Re: [PATCH] rebase -i: handle fixup of root commit correctly

From: Chris Webb <chris@arachsys.com>
Date: Jul 31, 2012, 11:19 UTC
Message-ID: <20120731111938.GD19416@arachsys.com>
In-Reply-To: <5017A1E4.1070800@kdbg.org>

Johannes Sixt <j6t@kdbg.org> writes:

Show 29 quoted lines

> Am 24.07.2012 14:17, schrieb Chris Webb:
> >diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> >index bef7bc0..0d2056f 100644
> >--- a/git-rebase--interactive.sh
> >+++ b/git-rebase--interactive.sh
> >@@ -493,25 +493,28 @@ do_next () {
> >  		author_script_content=$(get_author_ident_from_commit HEAD)
> >  		echo "$author_script_content" >"$author_script"
> >  		eval "$author_script_content"
> >-		output git reset --soft HEAD^
> >-		pick_one -n $sha1 || die_failed_squash $sha1 "$rest"
> >+		if ! pick_one -n $sha1
> >+		then
> >+			git rev-parse --verify HEAD >"$amend"
> >+			die_failed_squash $sha1 "$rest"
> >+		fi
> >  		case "$(peek_next_command)" in
> >  		squash|s|fixup|f)
> >  			# This is an intermediate commit; its message will only be
> >  			# used in case of trouble.  So use the long version:
> >-			do_with_author output git commit --no-verify -F "$squash_msg" ||
> >+			do_with_author output git commit --amend --no-verify -F "$squash_msg" ||
> >  				die_failed_squash $sha1 "$rest"
> 
> This new sequence looks *VERY* suspicious. It makes a HUGE
> difference in what is left behind if the cherry-pick fails. Did you
> think about what happens when the cherry-pick fails in a
> squash+squash+fixup+fixup sequence (or any combination thereof) and
> then the rebase is continued (after a manual resolution)?

I had to deal with the case where there's a conflict while picking the squash/fixup, and we have to ensure we commit --amend in rebase --continue. This is why I've written

  git rev-parse --verify HEAD >"$amend"

in the above, to use the pre-existing support for amending the HEAD commit in rebase --continue. (We test for this fixup-conflict case in various ways in t3404 and not doing an amend there would result in double commits and spectacular test breakage.)

Is this the issue you mean here, or is it something more subtle which I'm not properly following?

If we have a conflict in the middle of a chain of fixup/squashes, as far as I can see, we have a HEAD with all the previous successful fixups applied, conflict markers for the current failed pick, and when the conflict has been resolved, git rebase --continue will commit --amend the resolution and continue? Isn't that the correct behaviour here?

Cheers,

Chris.