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

There is a bug with git rebase -i --root when a fixup or squash line is
applied to the new root. We attempt to amend the commit onto which they
apply with git reset --soft HEAD^ followed by a normal commit. Unlike a
real commit --amend, this sequence will fail against a root commit as it
has no parent.

Fix rebase -i to use commit --amend for fixup and squash instead, and
add a test for the case of a fixup of the root commit.

Signed-off-by: Chris Webb <redacted>
---

Sorry, I should have spotted this issue when I did the original root-rebase
series. I've checked that this patch doesn't break any of the existing
tests, as well as satisfying the newly introduced check for the root-fixup
case.

 git-rebase--interactive.sh    | 25 +++++++++++++------------
 t/t3404-rebase-interactive.sh |  8 ++++++++
 2 files changed, 21 insertions(+), 12 deletions(-)

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 is the final command of this squash/fixup group
 			if test -f "$fixup_msg"
 			then
-				do_with_author git commit --no-verify -F "$fixup_msg" ||
+				do_with_author git commit --amend --no-verify -F "$fixup_msg" ||
 					die_failed_squash $sha1 "$rest"
 			else
 				cp "$squash_msg" "$GIT_DIR"/SQUASH_MSG || exit
 				rm -f "$GIT_DIR"/MERGE_MSG
-				do_with_author git commit --no-verify -e ||
+				do_with_author git commit --amend --no-verify -F "$GIT_DIR"/SQUASH_MSG -e ||
 					die_failed_squash $sha1 "$rest"
 			fi
 			rm -f "$squash_msg" "$fixup_msg"

@@ -748,7 +751,6 @@ In both case, once you're done, continue with:
 		fi
 		. "$author_script" ||
 			die "Error trying to find the author identity to amend commit"
-		current_head=
 		if test -f "$amend"
 		then
 			current_head=$(git rev-parse --verify HEAD)

@@ -756,13 +758,12 @@ In both case, once you're done, continue with:
 			die "\
 You have uncommitted changes in your working tree. Please, commit them
 first and then run 'git rebase --continue' again."
-			git reset --soft HEAD^ ||
-			die "Cannot rewind the HEAD"
+			do_with_author git commit --amend --no-verify -F "$msg" -e ||
+				die "Could not commit staged changes."
+		else
+			do_with_author git commit --no-verify -F "$msg" -e ||
+				die "Could not commit staged changes."
 		fi
-		do_with_author git commit --no-verify -F "$msg" -e || {
-			test -n "$current_head" && git reset --soft $current_head
-			die "Could not commit staged changes."
-		}
 	fi
 
 	record_in_rewritten "$(cat "$state_dir"/stopped-sha)"

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 8078db6..3f75d32 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh

@@ -903,4 +903,12 @@ test_expect_success 'rebase -i --root temporary sentinel commit' '
 	git rebase --abort
 '
 
+test_expect_success 'rebase -i --root fixup root commit' '
+	git checkout B &&
+	FAKE_LINES="1 fixup 2" git rebase -i --root &&
+	test A = $(git cat-file commit HEAD | sed -ne \$p) &&
+	test B = $(git show HEAD:file1) &&
+	test 0 = $(git cat-file commit HEAD | grep -c ^parent\ )
+'
+
 test_done

-- 
1.7.11.2.251.g6a928a6

Am 24.07.2012 14:17, schrieb Chris Webb:

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)?

-- Hannes

Johannes Sixt [off-list ref] writes:

Am 24.07.2012 14:17, schrieb Chris Webb:

quoted

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.

Chris Webb [off-list ref] writes:

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?

As an explicit test, I've just tried a chain of four squashed commits, each
of which deliberately resulted in a conflict to manually resolve. For each
squash, I was left with conflict markers on top of what had already been
squashed in the expected way, and when I continued after resolving these,
the resolution was 'commit --amend'ed in the expected way, with the same
behaviour and resulting commit at the end of the rebase -i as I get with a
copy of git without this patch.

Cheers,

Chris.

Am 31.07.2012 14:48, schrieb Chris Webb:

Chris Webb[off-list ref]  writes:

quoted

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?

As an explicit test, I've just tried a chain of four squashed commits, each
of which deliberately resulted in a conflict to manually resolve. For each
squash, I was left with conflict markers on top of what had already been
squashed in the expected way, and when I continued after resolving these,
the resolution was 'commit --amend'ed in the expected way, with the same
behaviour and resulting commit at the end of the rebase -i as I get with a
copy of git without this patch.

OK, good. One subtlety to watch out for is when commit messages are 
edited. That is, if you edit the proposed message at 'rebase --continue' 
after the first squash failed, is the new text preserved until the last 
squash? I *think* that previously that was the case.

That said, I do appreciate the new modus operandi. The state when a rebase 
is interrupted is much clearer than earlier: now HEAD contains everything 
that was successfully replayed so far, and the index anything that failed.

-- Hannes

Johannes Sixt [off-list ref] writes:

One subtlety to watch out for is when commit messages are edited. That is,
if you edit the proposed message at 'rebase --continue' after the first
squash failed, is the new text preserved until the last squash? I *think*
that previously that was the case.

Hi. Yes, doing this seems to work fine both in the original code, and after
my patch. I've just checked to be certain using my previous test case of
four conflicting squashes again, editing the message at each stage and
ensuring the edits are all retained in the final commit.

Best wishes,

Chris.