Support Grebase --continue/--edit-todo editor invocation

[rbong]

As discussed in #797

copen was decided as the method of feedback for the user on Grebase --continue because cwindow obfuscates user feedback.

The exact method for --edit-todo discussed was not used. You don't need to copy the file, edit it, and move it back. You just need to edit the file. The only difference between this and invoking --edit-todo via the command line is that the commit hashes are not contracted and expanded. This requires using git rebase--interactive --expand-ids and git rebase--interactive --shorten-ids, which are internal helpers and can't be relied on to remain publicly accessible.

Grebase --edit-todo does not yet support window mods but it easily can. Let me know if you think this is needed now.

I also believe that it would be useful to have --continue and --edit-todo rebase autocompletion. This is another thing that can easily be added if you think it should be part of this PR.

[rbong]

I've done some more testing and the code that adds informational rebase messages to the quickfix causes the commit window not to open. I'm pushing up a fix momentarily that updates the code which checks that there are no errors in the quickfix list by excluding informational lines.

[tpope]

This will result in a "problem with editor false" style error in the quickfix list on --edit-todo, correct? I think the check for it should be moved up to avoid that. Other than that looks good.

[tpope]

I'll deal with that later.

[rbong]

edit: whoops, didn't see your latest message. Glad it's in.

It will result in that error in the quickfix list, but the quickfix list will not be opened. However, we can avoid that error by simply changing the editor "true" rather than "false" when "--edit-todo" is passed in. I'm pushing that up now.

I am not entirely sure if this is the approach you were thinking of, but it preserves error checking by running the git rebase --edit-todo command while also keeping the "problem with editor false" error out of the quickfix window in case the user opens it.

Also, I want to make sure you know that the same behaviour will happen on "--continue". The editor error will be in the quickfix window, but the quickfix window will not automatically open if there is a commit message to edit. Causing an error using $GIT_EDITOR is the only way I know of to pause the rebase so we can edit commit messages. I assumed that was okay because %-Gerror:%.%#false was in the list of error formats, but if it's not okay and there's some way to get around actually having this error in the quickfix list let me know.

[tpope]

Yeah the error format should filter out the false message now that I think about it, but I don't see any reason to clobber the quickfix list at all in the successful --edit-todo case. If by "preserves error checking" you mean it prevents it from happening outside a rebase, I think the filereadable() check is fine for that. So it skips :make if we can edit the sequence file but runs it if we can't for the error message.

[tpope]

Any thoughts on how we might make :Grebase --continue invoke :Gcommit in the case of a resolved merge conflict?

[rbong]

I'll have to look into it more

[amadeus]

:Grebase --continue from a resolved merge conflict would be very nice :X kinda weird to have to jump out of the :Grebase flow when dealing with conflicts (a very common thing for me, heh)

[tpope]

It's driving me nuts. If someone can figure out the conditions of when we need to call git commit, then I can probably figure out the how. The answer should be in the git rebase --continue source code.

[rbong]

Rebase detects unresolved conflicts by looking for unstaged changes. Even if you are not in the middle of a merge conflict resolution, it will complain that you are if you have unstaged files.

Similarly, if you have any staged files and you run git rebase --continue, git enters into the commit editor whether you previously had a merge conflict or not. I cannot find the specific code for this but I've confirmed it through experimentation. If you need better than that, let me know. The flow is a little complex and the easiest way to find out what's going on is probably by compiling git with debug information and using gdb to trace it.

Minor note: if there are conflicts on git rebase --continue it will leave a MERGE_MSG... however, deleting this and running --continue results in the same behaviour (including the correct commit message), so it is probably not good to rely on this file at all.

[rbong]

My last post was correct but I have actually confirmed it in the source code now. There is only one minor special case to consider.

The main points about how it works are below, but more information is available here if you need to dig into it:

• just to show the flow up to the relevant point: when running git rebase --continue, git rebase--interactive --continue is called internally and drops into sequencer_continue, which then performs validation of the internal cache, the options, and the todo file. All of this validation can be put off by running rebase --continue after editing the commit as far as I can tell. The only relevant validation that occurs before getting to rebase--interactive is checking if there are unstaged changes, shown in my post above
• The sequencer immediately tries to commit any staged changes.
• The sequencer again checks for any unstaged changes and fails if they exist, although it should not get to this point
• If rebase-merge/amend exists, it goes down a long branching path. I haven't dug too deep into this because we're already handling the existence of this file.
• If there are no uncommitted changes and no errors handling the rebase-merge/amend file, the sequencer goes back to continuing the rebase
• At this point, the changes are committed and if there are any errors the sequencer fails. run_git_commit only checks if there is currently an interactive rebase, not for this special case, so it is the same as the other rebase cases we're currently handling
• Finally, any remaining interactive rebase state files are cleaned up and the normal continue workflow is returned to. Again, we're already handling these files and I haven't dug too deep into what happens when they're cleaned up

Main takeaways:

• As suspected, rebase checks to see if there was a merge conflict that needs resolving by looking at staged files. If there are unstaged files, it considers it an unresolved conflict, and if there are staged files, it commits them to resolve any conflicts. There doesn't appear to be any particularly special considerations
• There may be special considerations if files such as rebase-merge/amend exist already and there are staged files, but the way we're currently handling those files may be sufficient. Practical testing is needed before I go digging into that workflow

I hope there's something in there to help implement this. It's certainly beyond my capabilities to actually add this behaviour to the plugin without some serious investment.

[tpope]

I can't imagine I covered every use case but it's now working for the ones I bothered to test.