rebase -i: handle fixup of root commit correctly
PATCH 1/1 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.
---
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(-)
git-rebase--interactive.sh +13 -12
@@ -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)"
t/t3404-rebase-interactive.sh +8 -0
@@ -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
Signature · 1 line
1.7.11.2.251.g6a928a6
Quoted 9 lines · 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.
Quoted 7 lines
Signed-off-by Chris Webb <[email protected]>---
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.
Quoted 87 lines
git-rebase--interactive.sh | 25 +++++++++++++------------ t/t3404-rebase-interactive.sh | 8 ++++++++ 2 files changed, 21 insertions(+), 12 deletions(-)git-rebase--interactive.sh +13
-12@@ -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)"t/t3404-rebase-interactive.sh +8
-0@@ -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
Am 24.07.2012 14:17, schrieb Chris Webb:
Quoted 33 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<[email protected]>---
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(-)git-rebase--interactive.sh +2
-2@@ -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-rebase--interactive.sh do_next () { · line 506
Quoted 10 lines
git-rebase--interactive.sh +4
-1+ 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)?
Quoted 50 lines
git-rebase--interactive.sh +7
-9;; *) # 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)"t/t3404-rebase-interactive.sh +0
-0@@ -903,4 +903,12 @@ test_expect_success 'rebase -i --root temporary sentinel commit' ' git rebase --abort '
t/t3404-rebase-interactive.sh test_expect_success 'rebase -i --root temporary sentinel commit' ' · line 914
Quoted 10 lines
t/t3404-rebase-interactive.sh +8
-0+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
Quoted 21 lines · Johannes Sixt
Am 24.07.2012 14:17, schrieb Chris Webb:
Quoted 20 lines
git-rebase--interactive.sh +5
-3@@ -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" ||
Quoted 8 lines
git-rebase--interactive.sh do_next () { · line 506
Quoted 2 lines
git-rebase--interactive.sh +1
-0+ 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.
Quoted 5 lines · Chris Webb
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:
Quoted 8 lines
Quoted 5 lines · Chris Webb
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?
Quoted 7 lines
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
Quoted 4 lines · Johannes Sixt
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.