Re: [PATCH 3/3] rebase --skip: clean up commit message after a failedfixup/squash
Hi Phillip,
On Sat, 21 Apr 2018, Phillip Wood wrote:
Show 12 quoted lines
> On 20/04/18 13:18, Johannes Schindelin wrote:
> >
> > During a series of fixup/squash commands, the interactive rebase builds
> > up a commit message with comments. This will be presented to the user in
> > the editor if at least one of those commands was a `squash`.
> >
> > However, if the last of these fixup/squash commands fails with merge
> > conflicts, and if the user then decides to skip it (or resolve it to a
> > clean worktree and then continue the rebase), the current code fails to
> > clean up the commit message.
>
> Thanks for taking the time to track this down and fix it.
It was on my mind, but since I got caught twice by this bug within a week, I figured it was about time.
Show 38 quoted lines
> > diff --git a/sequencer.c b/sequencer.c
> > index a9c3bc26f84..f067b7b24c5 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -2781,17 +2781,12 @@ static int continue_single_pick(void)
> >
> > static int commit_staged_changes(struct replay_opts *opts)
> > {
> > - unsigned int flags = ALLOW_EMPTY | EDIT_MSG;
> > + unsigned int flags = ALLOW_EMPTY | EDIT_MSG, is_fixup = 0, is_clean;
> >
> > if (has_unstaged_changes(1))
> > return error(_("cannot rebase: You have unstaged changes."));
> > - if (!has_uncommitted_changes(0)) {
> > - const char *cherry_pick_head = git_path_cherry_pick_head();
> > - if (file_exists(cherry_pick_head) && unlink(cherry_pick_head))
> > - return error(_("could not remove CHERRY_PICK_HEAD"));
> > - return 0;
> > - }
> > + is_clean = !has_uncommitted_changes(0);
> >
> > if (file_exists(rebase_path_amend())) {
> > struct strbuf rev = STRBUF_INIT;
> > @@ -2804,16 +2799,41 @@ static int commit_staged_changes(struct replay_opts
> > *opts)
> > if (get_oid_hex(rev.buf, &to_amend))
> > return error(_("invalid contents: '%s'"),
> > rebase_path_amend());
> > - if (oidcmp(&head, &to_amend))
> > + if (!is_clean && oidcmp(&head, &to_amend))
> > return error(_("\nYou have uncommitted changes in your "
> > "working tree. Please, commit them\n"
> > "first and then run 'git rebase "
> > "--continue' again."));
> > + if (is_clean && !oidcmp(&head, &to_amend)) {
>
> Looking at pick_commits() it only writes to rebase_path_amend() if there are
> conflicts, not if the command has been rescheduled so this is safe.This is indeed the intent of that file.
Show 20 quoted lines
> > + strbuf_reset(&rev);
> > + /*
> > + * Clean tree, but we may need to finalize a
> > + * fixup/squash chain. A failed fixup/squash leaves
> > the
> > + * file amend-type in rebase-merge/; It is okay if
> > that
> > + * file is missing, in which case there is no such
> > + * chain to finalize.
> > + */
> > + read_oneliner(&rev, rebase_path_amend_type(), 0);
> > + if (!strcmp("squash", rev.buf))
> > + is_fixup = TODO_SQUASH;
> > + else if (!strcmp("fixup", rev.buf)) {
> > + is_fixup = TODO_FIXUP;
> > + flags = (flags & ~EDIT_MSG) | CLEANUP_MSG;
>
> I was going to say this should probably be (flags & ~(EDIT_MSG | VERIFY_MSG))
> but for some reason VERIFY_MSG isn't set here - I wonder if it should be as I
> think it's set elsewhere when we edit the message.As this patch series is purely about the bug fix where interrupted fixup/squash series can lead to incorrect commit messages, I would say that if this is a bug, it should be fixed in a separate patch series.
The name of that option is actually a little bit unfortunate: it bypasses the pre-commit/commit-msg hooks. I am not sure why they are bypassed in the commit_staged_changes() function.
*clickety-click*
It would appear that I simply copied this from
https://github.com/git/git/blob/v2.17.0/git-rebase--interactive.sh#L794-L808
So where does that come from? Let's use `git log -L 794,808:git-rebase--interactive.sh v2.17.0` to find out.
*clickety-click*
From that log, it looks as if this was added in 2147f844ed1 (rebase -i: handle fixup of root commit correctly, 2012-07-24). But that is incorrect: the --no-verify invocation was only split into two by said commit, and moved into the conditional. So we need to look a little further, with a larger line range (I extended it to 810 for the purpose of this analysis).
*clickety-click*
So it goes all the way back to c5b09feb786 (Avoid update hook during git-rebase --interactive, 2007-12-19). From that commit message, you can see that the rationale for the --no-verify flag was as following: the `git commit` might fail when continuing with staged changes, due to a check in a hook, and since there is inadequate error checking in the git-rebase--interactive.sh script, it would continue and squash the changes into the next commit.
From that description, it would appear that the proper fix would have been to 1) introduce proper error checking, and 2) offer a mode to bypass the hooks for *all* of the interactive rebase.
It is funny that 1) was addressed apparently 4 minutes later, in dbedf9729bd (Catch and handle git-commit failures in git-rebase --interactive, 2007-12-19).
As to 2): it seems to have been implemented in c44276563f9 (rebase --no-verify, 2008-10-06), but by that time it was probably safely forgotten that the --no-verify option was only introduced as a work-around for the absence of the `git rebase -i --no-verify` mode.
So I guess it would be time to undo c5b09feb786...
But as I said, I'd rather really not taint this here patch series by an unrelated bug fix. It has gone to the length of four iterations already even without such distractions.
Show 20 quoted lines
> > + }
> > + }
> >
> > strbuf_release(&rev);
> > flags |= AMEND_MSG;
> > }
> > + if (is_clean && !is_fixup) {
> > + const char *cherry_pick_head = git_path_cherry_pick_head();
> > +
> > + if (file_exists(cherry_pick_head) && unlink(cherry_pick_head))
> > + return error(_("could not remove CHERRY_PICK_HEAD"));
> > + return 0;
> > + }
> > +
> > if (run_git_commit(rebase_path_message(), opts, flags))
>
> If a squash command has been skipped, then rebase_path_message() still
> contains the message of the skipped commit. If it passed NULL instead
> then the user would get to edit the previous version of the squash
> message without the skipped commit message in it.True. One problem that I only caught late in my work on v4 is that run_git_commit() did not handle the absence of defmsg *and* EDIT_MSG well: in that case, it always opened the editor. I addressed that in the new 3/4. (The old 3/4 is no longer needed: it wrote the amend-type file whose purpose is now folded into current-fixups, introduced in the new 2/4.)
> Also I think we only want to re-commit if the skipped squash/fixup was
> preceded by another squash/fixup.
I thought so, too, at first. But I think that is still not quite the correct thing: we really only want to re-commit if we are in the middle of the final fixup/squash of the fixup/squash chain. And only if that final fixup/squash was not the only one in that chain.
Show 5 quoted lines
> If the user skips the first squash/fixup in a chain then HEAD has the
> commit message from the original pick so does not need amending. The
> first patch could perhaps avoid writing rebase_path_amend_type() in that
> case by reading the squash message and checking the message count is
> greater than two.
As I mentioned in another reply to you, I redid the whole shebang so that we always write a `current-fixups` file as soon as we encounter a fixup or squash. This file will build up the fixup/squash chain.
One thing that I had missed when I wrote that reply is that we sometimes do *not* skip a failed fixup/squash, but instead resolve the merge conflicts, stage the changes and call `git rebase --continue`. In this case, `git rebase` will commit the changes, *opening the commit message in an editor*. Therefore, the fixup/squash chain is broken at that point, and we have to treat the current fixup/squash as if it were the final in the current chain.
I imagine that it will take a while to review v4 in depth ;-)
Ciao, Dscho
Johannes Schindelin
