| Message ID | pull.818.git.1608337339246.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase -i: do leave commit message intact in fixup! chains | expand |
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
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
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