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

["Martin Ågren" <martin.agren@gmail.com>]

Hi Dscho,

On Sat, 19 Dec 2020 at 01:25, Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> 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.

>                 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