rebase --autosquash: handle manual "final fixups"
cover rebase --autosquash: handle manual "final fixups"
'tis bug fix season! I admit, though, that this bug might be a bit too off the trodden path to merit fast-tracking into v2.19.0.
While pairing with Jameson Miller to rebase Git for Windows to v2.19.0-rc1, we had to fix a couple of commits which had somehow lost their proper authorship (probably due to long fixed bugs in the interactive rebase). We did so by using empty squash! commits as reminders, so that we could interrupt the rebase by deleting the squash message, amend the commit appropriately, and then continue.
This exposed an (admittedly obscure) bug in the interactive rebase: when the last fixup or squash of a fixup/squash chain is aborted, and then the HEAD commit is amended, the rebase would not forget about the fixup/squash chain. It would hold onto the information about the current fixup count and the intermediate commit message. And upon processing another fixup/squash chain, that information would be reused!
This patch pair first introduces the test case to confirm the breakage, and then fixes it in the minimal way.
Johannes Schindelin (2):
rebase -i --autosquash: demonstrate a problem skipping the last squash rebase -i: be careful to wrap up fixup/squash chains
sequencer.c | 17 ++++++++++++++--- t/t3415-rebase-autosquash.sh | 19 +++++++++++++++++++ 2 files changed, 33 insertions(+), 3 deletions(-)
base-commit: 2f743933341f276111103550fbf383a34dfcfd38
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-30%2Fdscho%2Frebase-abort-last-squash-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-30/dscho/rebase-abort-last-squash-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/30
Signature · 1 line
gitgitgadget
PATCH 1/2 rebase -i --autosquash: demonstrate a problem skipping the last squash
From: Johannes Schindelin <[email protected]>
The git commit --squash command can be used not only to amend commit messages and changes, but also to record notes for an upcoming rebase.
For example, when the author information of a given commit is incorrect, a user might call git commit --allow-empty -m "Fix author" --squash <commit>, to remind them to fix that during the rebase. When the editor would pop up, the user would simply delete the commit message to abort the rebase at this stage, fix the author information, and continue with git rebase --skip. (This is a real-world example from the rebase of Git for Windows onto v2.19.0-rc1.)
However, there is a bug in git rebase that will cause the squash message *not* to be forgotten in this case. It will therefore be reused in the next fixup/squash chain (if any).
This patch adds a test case to demonstrate this breakage.
--- t/t3415-rebase-autosquash.sh | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
t/t3415-rebase-autosquash.sh +19 -0
@@ -330,4 +330,23 @@ test_expect_success 'wrapped original subject' ' test $base = $parent ' +test_expect_failure 'abort last squash' ' + test_when_finished "test_might_fail git rebase --abort" && + test_when_finished "git checkout master" && + + git checkout -b some-squashes && + git commit --allow-empty -m first && + git commit --allow-empty --squash HEAD && + git commit --allow-empty -m second && + git commit --allow-empty --squash HEAD && + + test_must_fail git -c core.editor="grep -q ^pick" \ + rebase -ki --autosquash HEAD~4 && + : do not finish the squash, but resolve it manually && + git commit --allow-empty --amend -m edited-first && + git rebase --skip && + git show >actual && + ! grep first actual +' + test_done
Signature · 1 line
gitgitgadget
PATCH 2/2 rebase -i: be careful to wrap up fixup/squash chains
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
When an interactive rebase was stopped at the end of a fixup/squash chain, the user might have edited the commit manually before continuing (with either git rebase --skip or git rebase --continue, it does not really matter which).
We need to be very careful to wrap up the fixup/squash chain also in this scenario: otherwise the next fixup/squash chain would try to pick up where the previous one was left.
--- sequencer.c | 17 ++++++++++++++--- t/t3415-rebase-autosquash.sh | 2 +- 2 files changed, 15 insertions(+), 4 deletions(-)
sequencer.c +14 -3
@@ -3578,9 +3578,20 @@ static int commit_staged_changes(struct replay_opts *opts,* the commit message and if there was a squash, let the user * edit it. */ -if (is_clean && !oidcmp (&head, &to_amend) && - opts->current_fixup_count >0 && -file_exists (rebase_path_stopped_sha ())) { +if (!is_clean || !opts->current_fixup_count) + ;/* this is not the final fixup */ +else if (oidcmp (&head, &to_amend) || + !file_exists (rebase_path_stopped_sha ())) { +/* was a final fixup or squash done manually? */ +if (!is_fixup (peek_command (todo_list,0 ))) { +unlink (rebase_path_fixup_msg ()); +unlink (rebase_path_squash_msg ()); +unlink (rebase_path_current_fixups ()); +strbuf_reset (&opts->current_fixups); + opts->current_fixup_count =0 ; + } + }else { +/* we are in a fixup/squash chain */ const char *p = opts->current_fixups.buf;int len = opts->current_fixups.len;
t/t3415-rebase-autosquash.sh +1 -1
@@ -330,7 +330,7 @@ test_expect_success 'wrapped original subject' ' test $base = $parent ' -test_expect_failure 'abort last squash' ' +test_expect_success 'abort last squash' ' test_when_finished "test_might_fail git rebase --abort" && test_when_finished "git checkout master" &&
Signature · 1 line
gitgitgadget
Re: [PATCH 2/2] rebase -i: be careful to wrap up fixup/squash chains
Quoted 13 lines
sequencer.c +2
-3@@ -3578,9 +3578,20 @@ static int commit_staged_changes(struct replay_opts *opts,* the commit message and if there was a squash, let the user * edit it. */ -if (is_clean && !oidcmp (&head, &to_amend) && - opts->current_fixup_count >0 && -file_exists (rebase_path_stopped_sha ())) { +if (!is_clean || !opts->current_fixup_count) + ;/* this is not the final fixup */
sequencer.c static int commit_staged_changes(struct replay_opts *opts, · line 3592
Quoted 10 lines
sequencer.c +10
-0+else if (oidcmp (&head, &to_amend) || + !file_exists (rebase_path_stopped_sha ())) { +/* was a final fixup or squash done manually? */ +if (!is_fixup (peek_command (todo_list,0 ))) { +unlink (rebase_path_fixup_msg ()); +unlink (rebase_path_squash_msg ()); +unlink (rebase_path_current_fixups ()); +strbuf_reset (&opts->current_fixups); + opts->current_fixup_count =0 ; + }
Let me see if the code is easily grokkable by (trying to) follow aloud.
We used to refrain from going into this big else clause that does the fixup-squash handling when is_clean is false, current-count is not yet zero, head and to-amend are different commits or stopped-sha file is missing. The updated code still refrains from going into the big else clause under exactly the same condition, but it learned to clean up the state, when the _next_ one is not a fix-up, i.e. when we are looking at the last fixup/squash in the current chain. And the lack of clean-up would have resulted in the next step misbehaving.
I see a few calls to is_fixup(peek_command()) and a local boolean variable final_fixup used in this function. I have to wonder if it makes the resulting code, especially the above part, easier to follow and understand, if the function peeked todo-list to check if we are dealing with the final fix-up in a chain very early just once, and used it to see "are we doing the final fixup/squash in the current chain?" throughout the rest of the function.
Side note: I actually think that the existing final_fixup boolean means something different (iow, final_fixup is not set inside the new "clean-up" code above, even though we dealt with the last one in the fix-up chain, and that is not a bug---which means that "final_fixup" does not mean "we are dealing with the last one in the fix-up chain"), which may want to be clarified a bit with in-code comment near where the variable is defined for the function to be readable.
In any case, thanks for fixing this, which seems to have appeared in Git 2.18. Let's fork a topic from maint, cook it in 'next' and aim for eventually merging it down for both 2.19 and 2.18 tracks.
Quoted 17 lines
sequencer.c +2
-0+ }else { +/* we are in a fixup/squash chain */ const char *p = opts->current_fixups.buf;int len = opts->current_fixups.len;t/t3415-rebase-autosquash.sh +1
-1@@ -330,7 +330,7 @@ test_expect_success 'wrapped original subject' ' test $base = $parent ' -test_expect_failure 'abort last squash' ' +test_expect_success 'abort last squash' ' test_when_finished "test_might_fail git rebase --abort" && test_when_finished "git checkout master" &&
Re*: [PATCH 1/2] rebase -i --autosquash: demonstrate a problem skipping the last squash
In reply to Johannes Schindelin via GitGitGadget"Johannes Schindelin via GitGitGadget" <[email protected]>
Review
Quoted 2 lines · writes:
(quoted diff) +2
-0+ test_must_fail git -c core.editor="grep -q ^pick" \ + rebase -ki --autosquash HEAD~4 &&
When merged to 'pu', this reveals that "git rebase" rewritten in C knows "--keep-empty" but not "-k".
I did a quick eyeballing between master:git-rebase.sh and pu:builtin/rebase.c and I think that it is the only one that is missed, but somebody may want to lend us a second set of eyes.
-- >8 --
Subject: rebase: re-add forgotten -k that stands for --keep-empty
630a70ea ("builtin rebase: support keep-empty option", 2018-08-08) forgot that the option comes with a short-and-sweet -k synonym. Add it back.
--- builtin/rebase.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/rebase.c +1 -1
@@ -847,7 +847,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) &options.allow_rerere_autoupdate, N_("allow rerere to update index with resolved " "conflict")), -OPT_BOOL (0 ,"keep-empty" , &options.keep_empty, +OPT_BOOL ('k' ,"keep-empty" , &options.keep_empty,N_ ("preserve empty commits during rebase" )), OPT_BOOL(0, "autosquash", &options.autosquash, N_("move commits that begin with "
Re: [PATCH 2/2] rebase -i: be careful to wrap up fixup/squash chains
In reply to Junio C HamanoHi Junio,
Quoted 27 lines · Junio C Hamano
Quoted 13 lines
sequencer.c +2
-3@@ -3578,9 +3578,20 @@ static int commit_staged_changes(struct replay_opts *opts,* the commit message and if there was a squash, let the user * edit it. */ -if (is_clean && !oidcmp (&head, &to_amend) && - opts->current_fixup_count >0 && -file_exists (rebase_path_stopped_sha ())) { +if (!is_clean || !opts->current_fixup_count) + ;/* this is not the final fixup */ sequencer.c static int commit_staged_changes(struct replay_opts *opts, · line 3592
Quoted 10 lines
sequencer.c +10
-0+else if (oidcmp (&head, &to_amend) || + !file_exists (rebase_path_stopped_sha ())) { +/* was a final fixup or squash done manually? */ +if (!is_fixup (peek_command (todo_list,0 ))) { +unlink (rebase_path_fixup_msg ()); +unlink (rebase_path_squash_msg ()); +unlink (rebase_path_current_fixups ()); +strbuf_reset (&opts->current_fixups); + opts->current_fixup_count =0 ; + }Let me see if the code is easily grokkable by (trying to) follow aloud.
Quoted 3 lines
We used to refrain from going into this big else clause that does the fixup-squash handling when
is_cleanis false, current-count is not yet zero, head and to-amend are different
s/not yet zero/still zero/
Quoted 6 lines
commits or stopped-sha file is missing. The updated code still refrains from going into the big else clause under exactly the same condition, but it learned to clean up the state, when the _next_ one is not a fix-up, i.e. when we are looking at the last fixup/squash in the current chain. And the lack of clean-up would have resulted in the next step misbehaving.
s/next step/ next fixup or squash chain, if any,/
You got the gist right.
Quoted 8 lines
I see a few calls to is_fixup(peek_command()) and a local boolean variable
final_fixupused in this function. I have to wonder if it makes the resulting code, especially the above part, easier to follow and understand, if the function peeked todo-list to check if we are dealing with the final fix-up in a chain very early just once, and used it to see "are we doing the final fixup/squash in the current chain?" throughout the rest of the function.
Quoted 8 lines
Side note: I actually think that the existing
final_fixupboolean means something different (iow,final_fixupis not set inside the new "clean-up" code above, even though we dealt with the last one in the fix-up chain, and that is not a bug---which means that "final_fixup" does not mean "we are dealing with the last one in the fix-up chain"), which may want to be clarified a bit with in-code comment near where the variable is defined for the function to be readable.
Indeed. The final_fixup name tries to convey "need to finalize the final fixup", as in: show the commit message in an editor if any squash! commits were included, and otherwise simply clean the commit message of all those commented-out lines.
So that's very different from "is the previously-run todo command a final fixup in a fixup/squash chain?"
Quoted 3 lines
In any case, thanks for fixing this, which seems to have appeared in Git 2.18. Let's fork a topic from maint, cook it in 'next' and aim for eventually merging it down for both 2.19 and 2.18 tracks.
Sounds good,
Dscho
Quoted 19 lines
Quoted 18 lines
sequencer.c +2
-0+ }else { +/* we are in a fixup/squash chain */ const char *p = opts->current_fixups.buf;int len = opts->current_fixups.len;t/t3415-rebase-autosquash.sh +1
-1@@ -330,7 +330,7 @@ test_expect_success 'wrapped original subject' ' test $base = $parent ' -test_expect_failure 'abort last squash' ' +test_expect_success 'abort last squash' ' test_when_finished "test_might_fail git rebase --abort" && test_when_finished "git checkout master" &&
Re: Re*: [PATCH 1/2] rebase -i --autosquash: demonstrate a problem skipping the last squash
In reply to Junio C HamanoHi Junio,
Quoted 8 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Review
Quoted 2 lines · writes:
(quoted diff) +2
-0+ test_must_fail git -c core.editor="grep -q ^pick" \ + rebase -ki --autosquash HEAD~4 &&When merged to 'pu', this reveals that "git rebase" rewritten in C knows "--keep-empty" but not "-k".
I had already reported this:
https://public-inbox.org/git/[email protected]/
Quoted 5 lines
I did a quick eyeballing between master:git-rebase.sh and pu:builtin/rebase.c and I think that it is the only one that is missed, but somebody may want to lend us a second set of eyes.
Quoted 8 lines
-- >8 --
Subject: rebase: re-add forgotten -k that stands for --keep-empty
630a70ea("builtin rebase: supportkeep-emptyoption", 2018-08-08) forgot that the option comes with a short-and-sweet -k synonym. Add it back.Signed-off-by Junio C Hamano <[email protected]>
Since this is not in next yet, let's do it the correct way. I had a fixup! sitting in my wip-rebase branch all the time, I just needed to find the time and motivation to figure out how to let GitGitGadget send a v2 when v1 was not sent via GitGitGadget.
Ciao,
Dscho
Quoted 19 lines
--- builtin/rebase.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)builtin/rebase.c +1
-1@@ -847,7 +847,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) &options.allow_rerere_autoupdate, N_("allow rerere to update index with resolved " "conflict")), -OPT_BOOL (0 ,"keep-empty" , &options.keep_empty, +OPT_BOOL ('k' ,"keep-empty" , &options.keep_empty,N_ ("preserve empty commits during rebase" )), OPT_BOOL(0, "autosquash", &options.autosquash, N_("move commits that begin with "
Re: Re*: [PATCH 1/2] rebase -i --autosquash: demonstrate a problem skipping the last squash
Quoted 11 lines · Johannes Schindelin
Quoted 8 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Review
Quoted 2 lines · writes:
(quoted diff) +2
-0+ test_must_fail git -c core.editor="grep -q ^pick" \ + rebase -ki --autosquash HEAD~4 &&When merged to 'pu', this reveals that "git rebase" rewritten in C knows "--keep-empty" but not "-k".
Quoted 3 lines
I had already reported this:
Good that you said there that you'll have it fixed in future updates.
I do not want to leave the project state with failing test at the tip of 'pu', and I do not want to get distracted in squashing the fix deep in large sequences of topics that we know will be rerolled once the prerelease freeze is over anyway, so in the meantime I'll queue this near the tip of 'pu' to keep the tree sane.
Thanks for eyeballing.