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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 3405 bytes --]

Hi Martin,

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

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

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