Re: [PATCH 2/2] rebase -i: be careful to wrap up fixup/squash chains
Hi Junio,
On Tue, 4 Sep 2018, Junio C Hamano wrote:
> > diff --git a/sequencer.c b/sequencer.c
> > index 84bf598c3e..ac5c805c14 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -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(, _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(, _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(>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
s/not yet zero/still zero/
> 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.
> 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.
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?"
> 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
>
> > + } else {
> > + /* we are in a fixup/squash chain */
> > const char *p = opts->current_fixups.buf;
> > int len = opts->current_fixups.len;
> >
> > diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
> > index 7d5ea340b3..13f5688135 100755
> > --- a/t/t3415-rebase-autosquash.sh
> > +++ b/t/t3415-rebase-autosquash.sh
> > @@ -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 2/2] rebase -i: be careful to wrap up fixup/squash chains
> diff --git a/sequencer.c b/sequencer.c
> index 84bf598c3e..ac5c805c14 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -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(, _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(, _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(>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.
> + } else {
> + /* we are in a fixup/squash chain */
> const char *p = opts->current_fixups.buf;
> int len = opts->current_fixups.len;
>
> diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
> index 7d5ea340b3..13f5688135 100755
> --- a/t/t3415-rebase-autosquash.sh
> +++ b/t/t3415-rebase-autosquash.sh
> @@ -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" &&
[PATCH 2/2] rebase -i: be careful to wrap up fixup/squash chains
From: Johannes Schindelin
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.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 17 ++---
t/t3415-rebase-autosquash.sh | 2 +-
2 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 84bf598c3e..ac5c805c14 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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(, _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(, _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(>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;
diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 7d5ea340b3..13f5688135 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -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" &&
--
gitgitgadget
