rebase -i: do leave commit message intact in fixup! chains

Commit Message

Johannes Schindelin Dec. 19, 2020, 12:22 a.m. UTC

From: Johannes Schindelin <[e-mail]>

In 6e98de72c03 (sequencer (rebase -i): add support for the 'fixup' and
'squash' commands, 2017-01-02), this developer introduced a change of
behavior by mistake: when encountering a `fixup!` commit (or multiple
`fixup!` commits) without any `squash!` commit thrown in, the final `git
commit` was invoked with `--cleanup=strip`. Prior to that commit, the
commit command had been called without that `--cleanup` option.

Since we explicitly read the original commit message from a file in that
case, there is really no sense in forcing that clean-up.

Reported-by: Vojtěch Knyttl <[e-mail]>
Signed-off-by: Johannes Schindelin <[e-mail]>
---
    Fix bug in interactive rebases where fixup! cleans up the commit message

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-818%2Fdscho%2Fautosquash-without-scissors-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-818/dscho/autosquash-without-scissors-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/818

 sequencer.c                  | 5 ++---
 t/t3415-rebase-autosquash.sh | 8 ++++++++
 2 files changed, 10 insertions(+), 3 deletions(-)


base-commit: ba2aa15129e59f248d8cdd30404bc78b5178f61d

Comments

Martin Ågren Dec. 19, 2020, 2:47 p.m. UTC | #1

Hi Dscho,

On Sat, 19 Dec 2020 at 01:25, Johannes Schindelin via GitGitGadget
<[e-mail]> wrote:
>
> From: Johannes Schindelin <[e-mail]>
>
> In 6e98de72c03 (sequencer (rebase -i): add support for the 'fixup' and
> 'squash' commands, 2017-01-02), this developer introduced a change of
> behavior by mistake: when encountering a `fixup!` commit (or multiple
> `fixup!` commits) without any `squash!` commit thrown in, the final `git
> commit` was invoked with `--cleanup=strip`. Prior to that commit, the
> commit command had been called without that `--cleanup` option.
>
> Since we explicitly read the original commit message from a file in that
> case, there is really no sense in forcing that clean-up.

>                 if (!final_fixup)
>                         msg_file = rebase_path_squash_msg();
> -               else if (file_exists(rebase_path_fixup_msg())) {
> -                       flags |= CLEANUP_MSG;
> +               else if (file_exists(rebase_path_fixup_msg()))
>                         msg_file = rebase_path_fixup_msg();
> -               } else {
> +               else {

I see. The bug survived your 789b3effec ("sequencer: make commit
options more extensible", 2017-03-23). Which isn't surprising for such a
mechanical change.

Nit: The "else" still needs braces, so if we follow the coding
guidelines, the "else if" should also use them. And even the "if",
FWIW. So it would arguably be more in line with CodingGuidelines to have
this diff just drop a single line, no additions needed.

So what this does in the end is, it stops adding `--cleanup=strip` and
it doesn't do anything instead, i.e., not even `--cleanup=whitespace`.
OK, we want to use the exact original message. But what if
`commit.cleanup` happens to be "strip"?

> +test_expect_success 'fixup does not clean up commit message' '
> +       oneline="#818" &&
> +       git commit --allow-empty -m "$oneline" &&
> +       git commit --fixup HEAD --allow-empty &&
> +       git rebase -ki --autosquash HEAD~2 &&
> +       test "$oneline" = "$(git show -s --format=%s)"
> +'

I changed your test to use

  git -c commit.cleanup=strip rebase ...

and it started failing. Maybe `run_git_command()` in sequencer.c could
learn to pass `--cleanup=verbatim` or in some other way make sure to
override any user configuration here? I couldn't figure out how to get
this to actually work, though...

Looking around for `CLEANUP_MSG`, I spotted the logic added by
15ef69314d ("rebase --skip: clean up commit message after a failed
fixup/squash", 2018-04-27). It seems like it has the same problem, but
that this proposed patch misses it. I did some testing that seemed to
confirm it:

Adding a commit with some "#message", then adding a fixup and then
adding a fixup that will conflict, then running the rebase and skipping
the conflicting fixup, I end up with a commit with the empty log
message. That's both before and after this proposed patch.

Martin

Johannes Schindelin Dec. 19, 2020, 3 p.m. UTC | #2

Hi Martin,

On Sat, 19 Dec 2020, Martin Ågren wrote:

> On Sat, 19 Dec 2020 at 01:25, Johannes Schindelin via GitGitGadget
> <[e-mail]> wrote:
> >
> > From: Johannes Schindelin <[e-mail]>
> >
> > In 6e98de72c03 (sequencer (rebase -i): add support for the 'fixup' and
> > 'squash' commands, 2017-01-02), this developer introduced a change of
> > behavior by mistake: when encountering a `fixup!` commit (or multiple
> > `fixup!` commits) without any `squash!` commit thrown in, the final `git
> > commit` was invoked with `--cleanup=strip`. Prior to that commit, the
> > commit command had been called without that `--cleanup` option.
> >
> > Since we explicitly read the original commit message from a file in that
> > case, there is really no sense in forcing that clean-up.
>
> >                 if (!final_fixup)
> >                         msg_file = rebase_path_squash_msg();
> > -               else if (file_exists(rebase_path_fixup_msg())) {
> > -                       flags |= CLEANUP_MSG;
> > +               else if (file_exists(rebase_path_fixup_msg()))
> >                         msg_file = rebase_path_fixup_msg();
> > -               } else {
> > +               else {
>
> I see. The bug survived your 789b3effec ("sequencer: make commit
> options more extensible", 2017-03-23). Which isn't surprising for such a
> mechanical change.
>
> Nit: The "else" still needs braces, so if we follow the coding
> guidelines, the "else if" should also use them. And even the "if",
> FWIW. So it would arguably be more in line with CodingGuidelines to have
> this diff just drop a single line, no additions needed.
>
> So what this does in the end is, it stops adding `--cleanup=strip` and
> it doesn't do anything instead, i.e., not even `--cleanup=whitespace`.
> OK, we want to use the exact original message. But what if
> `commit.cleanup` happens to be "strip"?
>
> > +test_expect_success 'fixup does not clean up commit message' '
> > +       oneline="#818" &&
> > +       git commit --allow-empty -m "$oneline" &&
> > +       git commit --fixup HEAD --allow-empty &&
> > +       git rebase -ki --autosquash HEAD~2 &&
> > +       test "$oneline" = "$(git show -s --format=%s)"
> > +'
>
> I changed your test to use
>
>   git -c commit.cleanup=strip rebase ...
>
> and it started failing. Maybe `run_git_command()` in sequencer.c could
> learn to pass `--cleanup=verbatim` or in some other way make sure to
> override any user configuration here? I couldn't figure out how to get
> this to actually work, though...
>
> Looking around for `CLEANUP_MSG`, I spotted the logic added by
> 15ef69314d ("rebase --skip: clean up commit message after a failed
> fixup/squash", 2018-04-27). It seems like it has the same problem, but
> that this proposed patch misses it. I did some testing that seemed to
> confirm it:
>
> Adding a commit with some "#message", then adding a fixup and then
> adding a fixup that will conflict, then running the rebase and skipping
> the conflicting fixup, I end up with a commit with the empty log
> message. That's both before and after this proposed patch.

Thank you so much for this detailed feedback. I will take care of it next
year, after taking a semi-vacation interrupted only by releasing -rc1
(check), -rc2 (TBD) and v2.30.0 final (TBD).

Ciao,
Dscho

Patch

diff --git a/sequencer.c b/sequencer.c
index 8909a467700..749bddd7a1f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2001,10 +2001,9 @@  static int do_pick_commit(struct repository *r,
 		flags |= AMEND_MSG;
 		if (!final_fixup)
 			msg_file = rebase_path_squash_msg();
-		else if (file_exists(rebase_path_fixup_msg())) {
-			flags |= CLEANUP_MSG;
+		else if (file_exists(rebase_path_fixup_msg()))
 			msg_file = rebase_path_fixup_msg();
-		} else {
+		else {
 			const char *dest = git_path_squash_msg(r);
 			unlink(dest);
 			if (copy_file(dest, rebase_path_squash_msg(), 0666))
diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 7bab6000dc7..4c83c98b3fc 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -440,4 +440,12 @@  test_expect_success 'fixup a fixup' '
 	test XZWY = $(git show | tr -cd W-Z)
 '
 
+test_expect_success 'fixup does not clean up commit message' '
+	oneline="#818" &&
+	git commit --allow-empty -m "$oneline" &&
+	git commit --fixup HEAD --allow-empty &&
+	git rebase -ki --autosquash HEAD~2 &&
+	test "$oneline" = "$(git show -s --format=%s)"
+'
+
 test_done