threads / patch / 31084

patchrebase -i: handle fixup of root commit correctly

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

## tl;dr

7 messages between Jul 24, 2012 and Jul 31, 2012. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Chris Webb· Jul 24, 2012, 12:17 UTC · lore

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(-)
Show changes to 2 files +21 −12

git-rebase--interactive.sh, t/t3404-rebase-interactive.sh

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
Junio C Hamano· Jul 24, 2012, 19:22 UTC · re: Chris Webb · lore

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

Chris Webb <chris@arachsys.com> writes:
Show 16 quoted lines
> 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.
Show 87 quoted lines
>
>  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
Johannes Sixt· Jul 31, 2012, 09:14 UTC · re: Chris Webb · lore

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

Am 24.07.2012 14:17, schrieb Chris Webb:
Show 43 quoted lines
> 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)?

Show 60 quoted lines
>   			;;
>   		*)
>   			# 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
Chris Webb· Jul 31, 2012, 11:19 UTC · re: Johannes Sixt · lore

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

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.
Chris Webb· Jul 31, 2012, 12:48 UTC · re: Chris Webb · lore

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

Chris Webb <chris@arachsys.com> writes:
Show 5 quoted lines
> 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.
Johannes Sixt· Jul 31, 2012, 20:04 UTC · re: Chris Webb · lore

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

Am 31.07.2012 14:48, schrieb Chris Webb:
Show 15 quoted lines
> 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
Chris Webb· Jul 31, 2012, 22:47 UTC · re: Johannes Sixt · lore

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

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.

← back to recent threads