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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux