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

q should not abort the commit if it contains a written commit message. #552

Closed
ten3roberts opened this issue Jul 7, 2023 · 12 comments · Fixed by #607
Closed

q should not abort the commit if it contains a written commit message. #552

ten3roberts opened this issue Jul 7, 2023 · 12 comments · Fixed by #607

Comments

@ten3roberts
Copy link
Member

I've encountered this multiple times because I try :wq to save and quit to commit, but accidentally just hit q which makes me lose the commit message.

Having a prompt if there are unsaved changes would be preferred

@wrightjjw
Copy link

Been looking into this. Looks like the commit logic is handled in a BufUnload autocmd in the commit editor (buffers/commit_editor/init.lua starting at line 46), which includes the "Are you sure you want to commit?" prompt. Btw, saying no to that discards the commit too - maybe that should also keep the buffer open in case someone wants to reword a message at the last minute.

Anyone know of a way to cancel the autocmd's event during its execution? That would probably be the easiest way to manage this, but I'm not sure if it's possible... A different approach may require reworking the logic outside an autocmd, which could be a rather major task.

By the way, you can also use ZZ to commit, which afaik would be equivalent to :wq in this case. Would probably cut down on the typos! 😃

@CKolkey
Copy link
Member

CKolkey commented Jul 8, 2023

This is slightly tangential, but for the longest time I've had an autocmd set up thats essentially:

autocmd InsertLeave * update

So, you leave insert mode, and it write the buffer to disk. The reasoning behind this is pretty simple: if I made changes, I probably wanted to. And writes take a negligible amount of time.

I'm bringing this up because my experience is exactly what we're describing as the ideal - dont let a user accidentally close the buffer with an unwritten commit message, discarding the commit.. because it's almost never in an unwritten state.

It would be trivial to add such an autocmd thats buffer-local, and could even gate it behind a config flag.

@ten3roberts
Copy link
Member Author

I am trying to use ZZ for commiting, but :wq and q for closing are so deeply engrained in my muscle memory that I do it in our team's discord chat, and close.other popups with q.

I think I need a firmware update as well.

Good point that aborting the commit should not discard the message entirely, hadn't thought about it. What I usually do when I remember I forgot to stage a file when writing the commit, I commit anyway and then amend instead.

There are definitely some good questions and considerations regarding the workflow, and I am open to all suggestions.

@wrightjjw
Copy link

This is slightly tangential, but for the longest time I've had an autocmd set up thats essentially:

autocmd InsertLeave * update

So, you leave insert mode, and it write the buffer to disk. The reasoning behind this is pretty simple: if I made changes, I probably wanted to. And writes take a negligible amount of time.

I'm bringing this up because my experience is exactly what we're describing as the ideal - dont let a user accidentally close the buffer with an unwritten commit message, discarding the commit.. because it's almost never in an unwritten state.

It would be trivial to add such an autocmd thats buffer-local, and could even gate it behind a config flag.

This is an interesting idea. Do you need to still need to use :w when you edit text in normal mode? Unless there's an autocmd for all edits, the behavior seems inconsistent to me. If I want to touch up the message with some ds, I may forget to write if I'm used to insert mode automatically writing, and I probably wouldn't realize it until I look at the commit on remote.

Alternatively, it would also be pretty easy to modify the existing autocmd to do a write when the buffer is closing (after a confirmation, of course). But neither of these ideas solve the problem of wanting to keep the buffer open after accidentally :qing.

I am trying to use ZZ for commiting, but :wq and q for closing are so deeply engrained in my muscle memory that I do it in our team's discord chat, and close.other popups with q.

I get that. I got used to ZZ pretty early in my vim-life, and now the idea of using :wq for anything sounds so foreign to me!

@ten3roberts
Copy link
Member Author

I may be wrong, but doing update may cause the commit to be fired off anyway as git will use the file contents .git/COMMIT_MSG when the invoked GIT_EDITOR exits. This means that if the file is saved git will see that and use those contents. That is why we have to make sure to clear the file explicitely if discarding a commit

-- Clear the buffer, without filling the register
.

I.e; git has no way to tell from the invoked editor of what to do besides the contents of the COMMIT_MSG file or MERGE_MSG etc that git creates and gives to the file.

@wrightjjw
Copy link

I may be wrong, but doing update may cause the commit to be fired off anyway as git will use the file contents .git/COMMIT_MSG when the invoked GIT_EDITOR exits. This means that if the file is saved git will see that and use those contents. That is why we have to make sure to clear the file explicitely if discarding a commit

I think this is wrong. You can do a :update while writing the commit message, but it won't commit until the editor quits. Clearing the buffer and writing the empty commit file, then exiting the editor, will tell git to cancel the commit.

@ten3roberts
Copy link
Member Author

Yes, sorry. What I meant was that if the buffer wasn't cleared after the editor has exited.

If you were to save the message to disk you would have to change the filename as well, or else it would fire off a commit when the editor is closed unless the buffer is cleared (which would defeat the purpose of saving it to the disk).

@wrightjjw
Copy link

Ah, I see what you mean.

Yeah, I don't think we would have a way of telling if the commit should be discarded or not if we're auto-writing during editing.

We could add a special keybind to the commit buffer to let neogit know to clear the buffer. But that would be un-git-like, and indirectly change the meaning of :q. I don't think that's a good idea.

@ten3roberts
Copy link
Member Author

Yes. I think that the problem is eerily similar to :term <cmd> buffers, which close after hitting enter when the process has completed. What I do in that case is just :w output.log to be able to go back to that very rare crash.

Maybe something similar where the commit buffer or another in progress message is saved under e.g ~/.local/cache/nvim/neogit/project%path/COMMIT_MSG so that it can be restored if closing and re-commiting without putting it in the outgoing chute of git

@CKolkey
Copy link
Member

CKolkey commented Jul 8, 2023

We should probably handle this like Magit does: c-c c-c commits, c-c c-x aborts. Not that we need to use the same chords, and we can of course keep the current behaviour, but a dedicated 'abort' command will probably be the clearest.

#492 could be added, for example

@wrightjjw
Copy link

I think in a perfect world, :q (or other such window-closing) should still abort the commit, even if a dedicated command is added. That's what it does when opening a vim from the git cli, so I don't think it's a good idea to change that.

@CKolkey
Copy link
Member

CKolkey commented Jul 8, 2023

I don't think thats possible - putting aside neogit for a second, when you're using git on the command line and you make a commit, it will open up .git/COMMIT_MSG with $EDITOR, and, importantly, will wait for that editor to close. If the file is empty, the commit is aborted. If it isn't empty, the commit goes through.

Neogit basically hijacks this workflow, and when the commit editor view opens, you're actually communicating via RPC with a separate instance of neovim. To be honest, I haven't wrapped my head entirely around how it works, but it's implemented here, primarily: https://github.com/NeogitOrg/neogit/blob/master/lua/neogit/client.lua

But, I haven't really looked into this system yet, so who knows :)

@ten3roberts ten3roberts added bug Something isn't working and removed bug Something isn't working labels Jul 11, 2023
PriceHiller added a commit to PriceHiller/neogit that referenced this issue Jul 15, 2023
PriceHiller added a commit to PriceHiller/neogit that referenced this issue Jul 15, 2023
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 a pull request may close this issue.

3 participants