Re: [PATCH 3/3] rebase --skip: clean up commit message after a failedfixup/squash

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Phillip,

On Sat, 21 Apr 2018, Phillip Wood wrote:

> 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.

> > 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.

> > +			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.

> > +			}
> > +		}
> >   
> >     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.

> 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