Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: do not close commit message with q until saved #607

Merged
merged 2 commits into from
Jul 18, 2023

Conversation

PriceHiller
Copy link
Contributor

Prior to this commit hitting q in normal mode in the commit editor would close it even if the text had not been saved. Also, prior to this commit, the Buffer:get_option in lib/buffer.lua never actually returned the option value 😅.

This PR pops up a warning when q is hit in normal mode and the buffer has unsaved modifications, we may want to change the message and the log level.
Currently the message is:

Commit message has not been saved! Try :w.

and the log level is set to WARN.

This PR also fixes the Buffer:get_option to actually return the value it got.

Let me know if any changes are desired to the message or log level, and secondly I have a question. Do y'all prefer me to force push or to create a separate commit for each change? Usually I try to avoid polluting the git history with a bunch of teeny tiny fix commits, but y'all may prefer that.

Closes #552

@CKolkey
Copy link
Member

CKolkey commented Jul 15, 2023

First off, hats off to you for all the recent work! And for fixing that buffer method. Oops.

Alright, lets get into this. I think you can combine your efforts with what I started here: #567

Essentially, instead of nudging a user to save, just ask if they want to abort the commit, because that should be allowed. But I like your way to check if the buffer is modified more, so definitely keep that.

And don't worry about the client.wrap() stuff - thats out of scope for this, and part of a bigger cleanup, so I'll do that in another branch.

As for the commits, thats up to you. A reasonably clean history is nice, without out too many "lint" commits, but... I'm also guilty of those sometimes, so don't worry about it too much.

@PriceHiller
Copy link
Contributor Author

  1. Thanks, I've got a bunch of time on my hands and I got tired of writing Rust in the last week.

  2. I didn't notice Add notice if closing commit without saving #567, are you sure you want confirmation dialogues for it?
    Would it be better to just inform the user to hit :q!? Dialogues block the editor and don't conform to standard keymaps. If the confirmation is preferred then I can certainly integrate it over here.

  3. Then I'll do my standard commit stuff of force pushing for tiny chunks and new commits for reasonably new stuff/significant modifications.

@PriceHiller
Copy link
Contributor Author

Oh wait, yeah a dialogue is definitely better. They're hitting q in normal mode, not doing anything funky. Will integrate.

@PriceHiller
Copy link
Contributor Author

PriceHiller commented Jul 15, 2023

@CKolkey

Alright, take a look again. I removed the write variable and check the modified status as well as getting user input. I also convert the buf given to the BufUnload autocmd into a buffer object so we can use the functionality that has been built for it.

As a heads up nvim_buf_get_option is deprecated in 0.10, so that's something we have to keep an eye out for. I don't want to write the shims for compatibility in this PR, but it will need to be done prior to 0.11 or 0.12, whenever they remove it.

@PriceHiller PriceHiller force-pushed the fix/commit-message-q branch 2 times, most recently from 764162a to 549da37 Compare July 15, 2023 19:20
Copy link
Member

@ten3roberts ten3roberts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the state of this so far, and the solution is really nice and goes out of your way without trying to do too much surprising side effects behind the scenes (such as saving somewhere)

lua/neogit/buffers/commit_editor/init.lua Outdated Show resolved Hide resolved
@CKolkey CKolkey merged commit 7240327 into NeogitOrg:master Jul 18, 2023
@PriceHiller PriceHiller deleted the fix/commit-message-q branch July 19, 2023 06:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

q should not abort the commit if it contains a written commit message.
3 participants