Re: [PATCH] rebase -i: do leave commit message intact in fixup! chains

From: Martin Ågren <martin.agren@gmail.com>
Date: Dec 19, 2020, 14:47 UTC
Message-ID: <CAN0heSpEnZGN9DBCabYg8JPWdSzRGh5YQ6CdrN4ubLqG-JkN8A@mail.gmail.com>
In-Reply-To: <pull.818.git.1608337339246.gitgitgadget@gmail.com>

Hi Dscho,

On Sat, 19 Dec 2020 at 01:25, Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 12 quoted lines

>
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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.

Show 8 quoted lines

>                 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"?

Show 7 quoted lines

> +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