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

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

[Chris Webb]

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 <chris@arachsys.com>
---

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

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

[Junio C Hamano]

Chris Webb <chris@arachsys.com> writes:

> 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 <chris@arachsys.com>
> ---
>
> 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.

OK, so instead of "reset --soft HEAD^ && pick -n && commit -F msg"
to back up one step and then build on top of it, the new sequence
"pick -n && commit --amend -F msg" modifies and then amends, whose
end result should be the same but the important difference is that
the latter would work even if the current commit is a root one.

Makes sense.  Thanks for catching and fixing it.

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

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

[Johannes Sixt]

Am 24.07.2012 14:17, schrieb Chris Webb:
> 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<chris@arachsys.com>
> ---
>
> 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 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)?

>   			;;
>   		*)
>   			# 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

-- Hannes

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

[Chris Webb]

Johannes Sixt <j6t@kdbg.org> writes:

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

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

[Chris Webb]

Chris Webb <chris@arachsys.com> 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.

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

[Johannes Sixt]

Am 31.07.2012 14:48, schrieb Chris Webb:
> Chris Webb<chris@arachsys.com>  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.

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

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

[Chris Webb]

Johannes Sixt <j6t@kdbg.org> 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.