{"thread":{"id":"54850","subject":"[PATCH] rebase -i: do leave commit message intact in fixup! chains","startedAt":"2020-12-19T00:23:10Z","lastAt":"2021-01-28T20:20:01Z","messageCount":10,"participants":["Johannes Schindelin via GitGitGadget","Martin Ågren","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"412611","messageId":"pull.818.git.1608337339246.gitgitgadget@gmail.com","threadId":"54850","inReplyTo":null,"subject":"[PATCH] rebase -i: do leave commit message intact in fixup! chains","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-19T00:22:18Z","receivedAt":"2020-12-19T00:23:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 6e98de72c03 (sequencer (rebase -i): add support for the 'fixup' and\n'squash' commands, 2017-01-02), this developer introduced a change of\nbehavior by mistake: when encountering a `fixup!` commit (or multiple\n`fixup!` commits) without any `squash!` commit thrown in, the final `git\ncommit` was invoked with `--cleanup=strip`. Prior to that commit, the\ncommit command had been called without that `--cleanup` option.\n\nSince we explicitly read the original commit message from a file in that\ncase, there is really no sense in forcing that clean-up.\n\nReported-by: Vojtěch Knyttl \nSigned-off-by: Johannes Schindelin \n---\n Fix bug in interactive rebases where fixup! cleans up the commit message\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-818%2Fdscho%2Fautosquash-without-scissors-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-818/dscho/autosquash-without-scissors-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/818\n\n sequencer.c | 5 ++---\n t/t3415-rebase-autosquash.sh | 8 ++++++++\n 2 files changed, 10 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 8909a467700..749bddd7a1f 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2001,10 +2001,9 @@ static int do_pick_commit(struct repository *r,\n \t\tflags |= AMEND_MSG;\n \t\tif (!final_fixup)\n \t\t\tmsg_file = rebase_path_squash_msg();\n-\t\telse if (file_exists(rebase_path_fixup_msg())) {\n-\t\t\tflags |= CLEANUP_MSG;\n+\t\telse if (file_exists(rebase_path_fixup_msg()))\n \t\t\tmsg_file = rebase_path_fixup_msg();\n-\t\t} else {\n+\t\telse {\n \t\t\tconst char *dest = git_path_squash_msg(r);\n \t\t\tunlink(dest);\n \t\t\tif (copy_file(dest, rebase_path_squash_msg(), 0666))\ndiff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh\nindex 7bab6000dc7..4c83c98b3fc 100755\n--- a/t/t3415-rebase-autosquash.sh\n+++ b/t/t3415-rebase-autosquash.sh\n@@ -440,4 +440,12 @@ test_expect_success 'fixup a fixup' '\n \ttest XZWY = $(git show | tr -cd W-Z)\n '\n \n+test_expect_success 'fixup does not clean up commit message' '\n+\toneline=\"#818\" &&\n+\tgit commit --allow-empty -m \"$oneline\" &&\n+\tgit commit --fixup HEAD --allow-empty &&\n+\tgit rebase -ki --autosquash HEAD~2 &&\n+\ttest \"$oneline\" = \"$(git show -s --format=%s)\"\n+'\n+\n test_done\n\nbase-commit: ba2aa15129e59f248d8cdd30404bc78b5178f61d\n-- \ngitgitgadget\n"},{"id":"412643","messageId":"CAN0heSpEnZGN9DBCabYg8JPWdSzRGh5YQ6CdrN4ubLqG-JkN8A@mail.gmail.com","threadId":"54850","inReplyTo":"pull.818.git.1608337339246.gitgitgadget@gmail.com","subject":"Re: [PATCH] rebase -i: do leave commit message intact in fixup! chains","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-12-19T14:47:09Z","receivedAt":"2020-12-19T14:48:14Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Hi Dscho,\n\nOn Sat, 19 Dec 2020 at 01:25, Johannes Schindelin via GitGitGadget\n wrote:\n>\n> From: Johannes Schindelin \n>\n> In 6e98de72c03 (sequencer (rebase -i): add support for the 'fixup' and\n> 'squash' commands, 2017-01-02), this developer introduced a change of\n> behavior by mistake: when encountering a `fixup!` commit (or multiple\n> `fixup!` commits) without any `squash!` commit thrown in, the final `git\n> commit` was invoked with `--cleanup=strip`. Prior to that commit, the\n> commit command had been called without that `--cleanup` option.\n>\n> Since we explicitly read the original commit message from a file in that\n> case, there is really no sense in forcing that clean-up.\n\n> if (!final_fixup)\n> msg_file = rebase_path_squash_msg();\n> - else if (file_exists(rebase_path_fixup_msg())) {\n> - flags |= CLEANUP_MSG;\n> + else if (file_exists(rebase_path_fixup_msg()))\n> msg_file = rebase_path_fixup_msg();\n> - } else {\n> + else {\n\nI see. The bug survived your 789b3effec (\"sequencer: make commit\noptions more extensible\", 2017-03-23). Which isn't surprising for such a\nmechanical change.\n\nNit: The \"else\" still needs braces, so if we follow the coding\nguidelines, the \"else if\" should also use them. And even the \"if\",\nFWIW. So it would arguably be more in line with CodingGuidelines to have\nthis diff just drop a single line, no additions needed.\n\nSo what this does in the end is, it stops adding `--cleanup=strip` and\nit doesn't do anything instead, i.e., not even `--cleanup=whitespace`.\nOK, we want to use the exact original message. But what if\n`commit.cleanup` happens to be \"strip\"?\n\n> +test_expect_success 'fixup does not clean up commit message' '\n> + oneline=\"#818\" &&\n> + git commit --allow-empty -m \"$oneline\" &&\n> + git commit --fixup HEAD --allow-empty &&\n> + git rebase -ki --autosquash HEAD~2 &&\n> + test \"$oneline\" = \"$(git show -s --format=%s)\"\n> +'\n\nI changed your test to use\n\n git -c commit.cleanup=strip rebase ...\n\nand it started failing. Maybe `run_git_command()` in sequencer.c could\nlearn to pass `--cleanup=verbatim` or in some other way make sure to\noverride any user configuration here? I couldn't figure out how to get\nthis to actually work, though...\n\nLooking around for `CLEANUP_MSG`, I spotted the logic added by\n15ef69314d (\"rebase --skip: clean up commit message after a failed\nfixup/squash\", 2018-04-27). It seems like it has the same problem, but\nthat this proposed patch misses it. I did some testing that seemed to\nconfirm it:\n\nAdding a commit with some \"#message\", then adding a fixup and then\nadding a fixup that will conflict, then running the rebase and skipping\nthe conflicting fixup, I end up with a commit with the empty log\nmessage. That's both before and after this proposed patch.\n\nMartin\n"},{"id":"412646","messageId":"nycvar.QRO.7.76.6.2012191559180.56@tvgsbejvaqbjf.bet","threadId":"54850","inReplyTo":"CAN0heSpEnZGN9DBCabYg8JPWdSzRGh5YQ6CdrN4ubLqG-JkN8A@mail.gmail.com","subject":"Re: [PATCH] rebase -i: do leave commit message intact in fixup! chains","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-12-19T15:00:27Z","receivedAt":"2020-12-19T15:02:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Martin,\n\nOn Sat, 19 Dec 2020, Martin Ågren wrote:\n\n> On Sat, 19 Dec 2020 at 01:25, Johannes Schindelin via GitGitGadget\n> wrote:\n> >\n> > From: Johannes Schindelin \n> >\n> > In 6e98de72c03 (sequencer (rebase -i): add support for the 'fixup' and\n> > 'squash' commands, 2017-01-02), this developer introduced a change of\n> > behavior by mistake: when encountering a `fixup!` commit (or multiple\n> > `fixup!` commits) without any `squash!` commit thrown in, the final `git\n> > commit` was invoked with `--cleanup=strip`. Prior to that commit, the\n> > commit command had been called without that `--cleanup` option.\n> >\n> > Since we explicitly read the original commit message from a file in that\n> > case, there is really no sense in forcing that clean-up.\n>\n> > if (!final_fixup)\n> > msg_file = rebase_path_squash_msg();\n> > - else if (file_exists(rebase_path_fixup_msg())) {\n> > - flags |= CLEANUP_MSG;\n> > + else if (file_exists(rebase_path_fixup_msg()))\n> > msg_file = rebase_path_fixup_msg();\n> > - } else {\n> > + else {\n>\n> I see. The bug survived your 789b3effec (\"sequencer: make commit\n> options more extensible\", 2017-03-23). Which isn't surprising for such a\n> mechanical change.\n>\n> Nit: The \"else\" still needs braces, so if we follow the coding\n> guidelines, the \"else if\" should also use them. And even the \"if\",\n> FWIW. So it would arguably be more in line with CodingGuidelines to have\n> this diff just drop a single line, no additions needed.\n>\n> So what this does in the end is, it stops adding `--cleanup=strip` and\n> it doesn't do anything instead, i.e., not even `--cleanup=whitespace`.\n> OK, we want to use the exact original message. But what if\n> `commit.cleanup` happens to be \"strip\"?\n>\n> > +test_expect_success 'fixup does not clean up commit message' '\n> > + oneline=\"#818\" &&\n> > + git commit --allow-empty -m \"$oneline\" &&\n> > + git commit --fixup HEAD --allow-empty &&\n> > + git rebase -ki --autosquash HEAD~2 &&\n> > + test \"$oneline\" = \"$(git show -s --format=%s)\"\n> > +'\n>\n> I changed your test to use\n>\n> git -c commit.cleanup=strip rebase ...\n>\n> and it started failing. Maybe `run_git_command()` in sequencer.c could\n> learn to pass `--cleanup=verbatim` or in some other way make sure to\n> override any user configuration here? I couldn't figure out how to get\n> this to actually work, though...\n>\n> Looking around for `CLEANUP_MSG`, I spotted the logic added by\n> 15ef69314d (\"rebase --skip: clean up commit message after a failed\n> fixup/squash\", 2018-04-27). It seems like it has the same problem, but\n> that this proposed patch misses it. I did some testing that seemed to\n> confirm it:\n>\n> Adding a commit with some \"#message\", then adding a fixup and then\n> adding a fixup that will conflict, then running the rebase and skipping\n> the conflicting fixup, I end up with a commit with the empty log\n> message. That's both before and after this proposed patch.\n\nThank you so much for this detailed feedback. I will take care of it next\nyear, after taking a semi-vacation interrupted only by releasing -rc1\n(check), -rc2 (TBD) and v2.30.0 final (TBD).\n\nCiao,\nDscho\n"},{"id":"413828","messageId":"pull.818.v2.git.1610123298764.gitgitgadget@gmail.com","threadId":"54850","inReplyTo":"pull.818.git.1608337339246.gitgitgadget@gmail.com","subject":"[PATCH v2] rebase -i: do leave commit message intact in fixup! chains","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-08T16:28:18Z","receivedAt":"2021-01-08T16:29:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 6e98de72c03 (sequencer (rebase -i): add support for the 'fixup' and\n'squash' commands, 2017-01-02), this developer introduced a change of\nbehavior by mistake: when encountering a `fixup!` commit (or multiple\n`fixup!` commits) without any `squash!` commit thrown in, the final `git\ncommit` was invoked with `--cleanup=strip`. Prior to that commit, the\ncommit command had been called without that `--cleanup` option.\n\nSince we explicitly read the original commit message from a file in that\ncase, there is really no sense in forcing that clean-up.\n\nWe actually need to actively suppress that clean-up lest a configured\n`commit.cleanup` may interfere with what we want to do: leave the commit\nmessage unchanged.\n\nReported-by: Vojtěch Knyttl \nHelped-by: Martin Ågren \nSigned-off-by: Johannes Schindelin \n---\n Fix bug in interactive rebases where fixup! cleans up the commit message\n \n Original report here:\n https://lore.kernel.org/git/CANVGpwZGbzYLMeMze64e_OU9p3bjyEgzC5thmNBr6LttBt%2BYGw%40mail.gmail.com/t\n \n Changes since v1:\n \n * The fix now works even if commit.cleanup = commit\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-818%2Fdscho%2Fautosquash-without-scissors-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-818/dscho/autosquash-without-scissors-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/818\n\nRange-diff vs v1:\n\n 1: bfade3b146f ! 1: c760d6cd203 rebase -i: do leave commit message intact in fixup! chains\n @@ Commit message\n Since we explicitly read the original commit message from a file in that\n case, there is really no sense in forcing that clean-up.\n \n + We actually need to actively suppress that clean-up lest a configured\n + `commit.cleanup` may interfere with what we want to do: leave the commit\n + message unchanged.\n +\n Reported-by: Vojtěch Knyttl \n + Helped-by: Martin Ågren \n Signed-off-by: Johannes Schindelin \n \n ## sequencer.c ##\n +@@ sequencer.c: N_(\"you have staged changes in your working tree\\n\"\n + #define CLEANUP_MSG (1<<3)\n + #define VERIFY_MSG (1<<4)\n + #define CREATE_ROOT_COMMIT (1<<5)\n ++#define VERBATIM_MSG (1<<6)\n + \n + static int run_command_silent_on_success(struct child_process *cmd)\n + {\n +@@ sequencer.c: static int run_git_commit(const char *defmsg,\n + \t\tstrvec_pushl(&cmd.args, \"-C\", \"HEAD\", NULL);\n + \tif ((flags & CLEANUP_MSG))\n + \t\tstrvec_push(&cmd.args, \"--cleanup=strip\");\n ++\tif ((flags & VERBATIM_MSG))\n ++\t\tstrvec_push(&cmd.args, \"--cleanup=verbatim\");\n + \tif ((flags & EDIT_MSG))\n + \t\tstrvec_push(&cmd.args, \"-e\");\n + \telse if (!(flags & CLEANUP_MSG) &&\n +@@ sequencer.c: static int try_to_commit(struct repository *r,\n + \n + \tif (flags & CLEANUP_MSG)\n + \t\tcleanup = COMMIT_MSG_CLEANUP_ALL;\n ++\telse if (flags & VERBATIM_MSG)\n ++\t\tcleanup = COMMIT_MSG_CLEANUP_NONE;\n + \telse if ((opts->signoff || opts->record_origin) &&\n + \t\t !opts->explicit_cleanup)\n + \t\tcleanup = COMMIT_MSG_CLEANUP_SPACE;\n @@ sequencer.c: static int do_pick_commit(struct repository *r,\n - \t\tflags |= AMEND_MSG;\n \t\tif (!final_fixup)\n \t\t\tmsg_file = rebase_path_squash_msg();\n --\t\telse if (file_exists(rebase_path_fixup_msg())) {\n + \t\telse if (file_exists(rebase_path_fixup_msg())) {\n -\t\t\tflags |= CLEANUP_MSG;\n -+\t\telse if (file_exists(rebase_path_fixup_msg()))\n ++\t\t\tflags |= VERBATIM_MSG;\n \t\t\tmsg_file = rebase_path_fixup_msg();\n --\t\t} else {\n -+\t\telse {\n + \t\t} else {\n \t\t\tconst char *dest = git_path_squash_msg(r);\n - \t\t\tunlink(dest);\n - \t\t\tif (copy_file(dest, rebase_path_squash_msg(), 0666))\n \n ## t/t3415-rebase-autosquash.sh ##\n @@ t/t3415-rebase-autosquash.sh: test_expect_success 'fixup a fixup' '\n @@ t/t3415-rebase-autosquash.sh: test_expect_success 'fixup a fixup' '\n +\toneline=\"#818\" &&\n +\tgit commit --allow-empty -m \"$oneline\" &&\n +\tgit commit --fixup HEAD --allow-empty &&\n -+\tgit rebase -ki --autosquash HEAD~2 &&\n ++\tgit -c commit.cleanup=strip rebase -ki --autosquash HEAD~2 &&\n +\ttest \"$oneline\" = \"$(git show -s --format=%s)\"\n +'\n +\n\n\n sequencer.c | 7 ++++++-\n t/t3415-rebase-autosquash.sh | 8 ++++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 8909a467700..092e7b811f0 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -943,6 +943,7 @@ N_(\"you have staged changes in your working tree\\n\"\n #define CLEANUP_MSG (1<<3)\n #define VERIFY_MSG (1<<4)\n #define CREATE_ROOT_COMMIT (1<<5)\n+#define VERBATIM_MSG (1<<6)\n \n static int run_command_silent_on_success(struct child_process *cmd)\n {\n@@ -1012,6 +1013,8 @@ static int run_git_commit(const char *defmsg,\n \t\tstrvec_pushl(&cmd.args, \"-C\", \"HEAD\", NULL);\n \tif ((flags & CLEANUP_MSG))\n \t\tstrvec_push(&cmd.args, \"--cleanup=strip\");\n+\tif ((flags & VERBATIM_MSG))\n+\t\tstrvec_push(&cmd.args, \"--cleanup=verbatim\");\n \tif ((flags & EDIT_MSG))\n \t\tstrvec_push(&cmd.args, \"-e\");\n \telse if (!(flags & CLEANUP_MSG) &&\n@@ -1454,6 +1457,8 @@ static int try_to_commit(struct repository *r,\n \n \tif (flags & CLEANUP_MSG)\n \t\tcleanup = COMMIT_MSG_CLEANUP_ALL;\n+\telse if (flags & VERBATIM_MSG)\n+\t\tcleanup = COMMIT_MSG_CLEANUP_NONE;\n \telse if ((opts->signoff || opts->record_origin) &&\n \t\t !opts->explicit_cleanup)\n \t\tcleanup = COMMIT_MSG_CLEANUP_SPACE;\n@@ -2002,7 +2007,7 @@ static int do_pick_commit(struct repository *r,\n \t\tif (!final_fixup)\n \t\t\tmsg_file = rebase_path_squash_msg();\n \t\telse if (file_exists(rebase_path_fixup_msg())) {\n-\t\t\tflags |= CLEANUP_MSG;\n+\t\t\tflags |= VERBATIM_MSG;\n \t\t\tmsg_file = rebase_path_fixup_msg();\n \t\t} else {\n \t\t\tconst char *dest = git_path_squash_msg(r);\ndiff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh\nindex 7bab6000dc7..88040bc4352 100755\n--- a/t/t3415-rebase-autosquash.sh\n+++ b/t/t3415-rebase-autosquash.sh\n@@ -440,4 +440,12 @@ test_expect_success 'fixup a fixup' '\n \ttest XZWY = $(git show | tr -cd W-Z)\n '\n \n+test_expect_success 'fixup does not clean up commit message' '\n+\toneline=\"#818\" &&\n+\tgit commit --allow-empty -m \"$oneline\" &&\n+\tgit commit --fixup HEAD --allow-empty &&\n+\tgit -c commit.cleanup=strip rebase -ki --autosquash HEAD~2 &&\n+\ttest \"$oneline\" = \"$(git show -s --format=%s)\"\n+'\n+\n test_done\n\nbase-commit: ba2aa15129e59f248d8cdd30404bc78b5178f61d\n-- \ngitgitgadget\n"},{"id":"414206","messageId":"20210112204939.1095-1-martin.agren@gmail.com","threadId":"54850","inReplyTo":"pull.818.v2.git.1610123298764.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] rebase -i: do leave commit message intact in fixup! chains","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2021-01-12T20:49:39Z","receivedAt":"2021-01-12T21:41:58Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Johannes Schindelin via GitGitGadget wrote:\n>\n> We actually need to actively suppress that clean-up lest a configured\n> `commit.cleanup` may interfere with what we want to do: leave the commit\n> message unchanged.\n\n>     Changes since v1:>\n>      * The fix now works even if commit.cleanup = commit\n\nIndeed. FWIW, this patch looks good to me.\n\nThere is the lone remaining user of `CLEANUP_MSG` where we're handling\nthe skipping of the final fixup in a chain and which is part of a larger\nblock of code to handle various cases like that around `git rebase\n--skip`. I wrote some tests on top of your patch to see what happens and\ntry to understand what's going on. The tests are below.\n\nI can't say I grok all that's going on in the implementation. By the\ntime we're finalizing the commit message, it seems to me that we've lost\ntrack of which of the lines that look like comments are indeed comment\nlines added by us and which actually originate in the commit messages\nwe're trying to rebase and which just happen to begin with a comment\ncharacter.\n\nMaybe these tests could be simplified a bit, or de-boilerplated to some\nextent, but I think they do correctly demonstrate a similar bug that\nremains after your fix.\n\nWhat do you think? Are these test failures useful at all? Would it be\nan easy fix? In any case, I don't think the presence of these bugs need\nto hold up the fix you've posted here. The bugs look like they're\nrelated, but they are in different parts of the code (to the best of my\nunderstanding).\n\nFWIW, this diff is Signed-off-by me if you want to run with it. Maybe\ninclude some part of it that you agree with in your commit?\n\n(Yes, this diff mentions \"master\" several times. This file already\nmentions that branch name. You have a patch in seen to address that,\nbut there would be a semantic conflict.)\n\nMartin\n\ndiff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh\nindex 88040bc435..f80318998f 100755\n--- a/t/t3415-rebase-autosquash.sh\n+++ b/t/t3415-rebase-autosquash.sh\n@@ -448,4 +448,63 @@ test_expect_success 'fixup does not clean up commit message' '\n \ttest \"$oneline\" = \"$(git show -s --format=%s)\"\n '\n \n+test_expect_failure 'fixup does not clean message (conflict in only fixup)' '\n+\ttest_when_finished \"git checkout master\" &&\n+\toneline=\"#ladder\" &&\n+\techo version-1 >file &&\n+\tgit add file &&\n+\tgit commit -m version-1 &&\n+\tgit checkout -B fixup-topic HEAD^ &&\n+\t# Now make two commits (oneline+fixup) that conflict\n+\t# in the fixup when we rebase them onto master.\n+\tgit commit --allow-empty -m \"$oneline\" &&\n+\techo conflict >file &&\n+\tgit add file &&\n+\tgit commit --fixup HEAD &&\n+\ttest_must_fail git -c commit.cleanup=strip rebase -ki \\\n+\t\t--autosquash --onto=master HEAD~2 &&\n+\tgit rebase --skip &&\n+\ttest \"$oneline\" = \"$(git show -s --format=%s)\"\n+'\n+\n+test_expect_failure 'fixup does not clean message (conflict in non-last fixup)' '\n+\ttest_when_finished \"git checkout master\" &&\n+\toneline=\"#not-a-comment\" &&\n+\techo version-2 >file &&\n+\tgit add file &&\n+\tgit commit -m version-2 &&\n+\tgit checkout -B fixup-topic HEAD^ &&\n+\t# Now make three commits (oneline+fixup+fixup) that conflict\n+\t# in the first fixup when we rebase them onto master.\n+\tgit commit --allow-empty -m \"$oneline\" &&\n+\techo conflict >file &&\n+\tgit add file &&\n+\tgit commit --fixup HEAD &&\n+\tgit commit --fixup HEAD --allow-empty &&\n+\ttest_must_fail git -c commit.cleanup=strip rebase -ki \\\n+\t\t--autosquash --onto=master HEAD~3 &&\n+\tgit rebase --skip &&\n+\ttest \"$oneline\" = \"$(git show -s --format=%s)\"\n+'\n+\n+test_expect_failure 'fixup does not clean message (conflict in last fixup)' '\n+\ttest_when_finished \"git checkout master\" &&\n+\toneline=\"#hash\" &&\n+\techo version-3 >file &&\n+\tgit add file &&\n+\tgit commit -m version-3 &&\n+\tgit checkout -B fixup-topic HEAD^ &&\n+\t# Now make three commits (oneline+fixup+fixup) that conflict\n+\t# in the second fixup when we rebase them onto master.\n+\tgit commit --allow-empty -m \"$oneline\" &&\n+\tgit commit --fixup HEAD --allow-empty &&\n+\techo conflict >file &&\n+\tgit add file &&\n+\tgit commit --fixup HEAD &&\n+\ttest_must_fail git -c commit.cleanup=strip rebase -ki \\\n+\t\t--autosquash --onto=master HEAD~3 &&\n+\tgit rebase --skip &&\n+\ttest \"$oneline\" = \"$(git show -s --format=%s)\"\n+'\n+\n test_done\n"},{"id":"414233","messageId":"xmqqwnwhzr54.fsf@gitster.c.googlers.com","threadId":"54850","inReplyTo":"pull.818.v2.git.1610123298764.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] rebase -i: do leave commit message intact in fixup! chains","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-12T23:12:23Z","receivedAt":"2021-01-12T23:13:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> We actually need to actively suppress that clean-up lest a configured\n> `commit.cleanup` may interfere with what we want to do: leave the commit\n> message unchanged.\n\nGood thinking.\n\n> Reported-by: Vojtěch Knyttl \n> Helped-by: Martin Ågren \n> Signed-off-by: Johannes Schindelin \n> ---\n> ...\n> diff --git a/sequencer.c b/sequencer.c\n> index 8909a467700..092e7b811f0 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -943,6 +943,7 @@ N_(\"you have staged changes in your working tree\\n\"\n> #define CLEANUP_MSG (1<<3)\n> #define VERIFY_MSG (1<<4)\n> #define CREATE_ROOT_COMMIT (1<<5)\n> +#define VERBATIM_MSG (1<<6)\n\nIt somewhat bothers me that these pretend to be orthogonal options\nthat can be mixed and matched, but CLEANUP and VERBATIM do not make\nsense to be used at the same time. As long as we have some safety\nto ensure that both bits are not used at the same time, i.e.e.g.\n\n\tif ((flags & (CLEANUP_MSG|VERBATIM_MSG)) == (CLEANUP_MSG|VERBATIM_MSG))\n\t\tBUG(\"cleanup and verbatim asked at the same time\");\n\nit would be OK, though.\n\nThanks.\n"},{"id":"415509","messageId":"pull.818.v3.git.1611850603359.gitgitgadget@gmail.com","threadId":"54850","inReplyTo":"pull.818.v2.git.1610123298764.gitgitgadget@gmail.com","subject":"[PATCH v3] rebase -i: do leave commit message intact in fixup! chains","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-28T16:16:42Z","receivedAt":"2021-01-28T16:18:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 6e98de72c03 (sequencer (rebase -i): add support for the 'fixup' and\n'squash' commands, 2017-01-02), this developer introduced a change of\nbehavior by mistake: when encountering a `fixup!` commit (or multiple\n`fixup!` commits) without any `squash!` commit thrown in, the final `git\ncommit` was invoked with `--cleanup=strip`. Prior to that commit, the\ncommit command had been called without that `--cleanup` option.\n\nSince we explicitly read the original commit message from a file in that\ncase, there is really no sense in forcing that clean-up.\n\nWe actually need to actively suppress that clean-up lest a configured\n`commit.cleanup` may interfere with what we want to do: leave the commit\nmessage unchanged.\n\nReported-by: Vojtěch Knyttl \nHelped-by: Martin Ågren \nSigned-off-by: Johannes Schindelin \n---\n Fix bug in interactive rebases where fixup! cleans up the commit message\n \n Original report here:\n https://lore.kernel.org/git/CANVGpwZGbzYLMeMze64e_OU9p3bjyEgzC5thmNBr6LttBt%2BYGw%40mail.gmail.com/t\n \n Changes since v2:\n \n * Added a safeguard so that CLEANUP_MSG and VERBATIM_MSG aren't used\n together by mistake.\n * Changes since v1:\n * The fix now works even if commit.cleanup = commit\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-818%2Fdscho%2Fautosquash-without-scissors-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-818/dscho/autosquash-without-scissors-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/818\n\nRange-diff vs v2:\n\n 1: c760d6cd203 ! 1: 8acb9b4562e rebase -i: do leave commit message intact in fixup! chains\n @@ sequencer.c: N_(\"you have staged changes in your working tree\\n\"\n \n static int run_command_silent_on_success(struct child_process *cmd)\n {\n +@@ sequencer.c: static int run_git_commit(const char *defmsg,\n + {\n + \tstruct child_process cmd = CHILD_PROCESS_INIT;\n + \n ++\tif ((flags & CLEANUP_MSG) && (flags & VERBATIM_MSG))\n ++\t\tBUG(\"CLEANUP_MSG and VERBATIM_MSG are mutually exclusive\");\n ++\n + \tcmd.git_cmd = 1;\n + \n + \tif (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {\n @@ sequencer.c: static int run_git_commit(const char *defmsg,\n \t\tstrvec_pushl(&cmd.args, \"-C\", \"HEAD\", NULL);\n \tif ((flags & CLEANUP_MSG))\n @@ sequencer.c: static int run_git_commit(const char *defmsg,\n \tif ((flags & EDIT_MSG))\n \t\tstrvec_push(&cmd.args, \"-e\");\n \telse if (!(flags & CLEANUP_MSG) &&\n +@@ sequencer.c: static int try_to_commit(struct repository *r,\n + \tenum commit_msg_cleanup_mode cleanup;\n + \tint res = 0;\n + \n ++\tif ((flags & CLEANUP_MSG) && (flags & VERBATIM_MSG))\n ++\t\tBUG(\"CLEANUP_MSG and VERBATIM_MSG are mutually exclusive\");\n ++\n + \tif (parse_head(r, ¤t_head))\n + \t\treturn -1;\n + \n @@ sequencer.c: static int try_to_commit(struct repository *r,\n \n \tif (flags & CLEANUP_MSG)\n\n\n sequencer.c | 13 ++++++++++++-\n t/t3415-rebase-autosquash.sh | 8 ++++++++\n 2 files changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 8909a467700..264d494a64c 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -943,6 +943,7 @@ N_(\"you have staged changes in your working tree\\n\"\n #define CLEANUP_MSG (1<<3)\n #define VERIFY_MSG (1<<4)\n #define CREATE_ROOT_COMMIT (1<<5)\n+#define VERBATIM_MSG (1<<6)\n \n static int run_command_silent_on_success(struct child_process *cmd)\n {\n@@ -979,6 +980,9 @@ static int run_git_commit(const char *defmsg,\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \n+\tif ((flags & CLEANUP_MSG) && (flags & VERBATIM_MSG))\n+\t\tBUG(\"CLEANUP_MSG and VERBATIM_MSG are mutually exclusive\");\n+\n \tcmd.git_cmd = 1;\n \n \tif (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {\n@@ -1012,6 +1016,8 @@ static int run_git_commit(const char *defmsg,\n \t\tstrvec_pushl(&cmd.args, \"-C\", \"HEAD\", NULL);\n \tif ((flags & CLEANUP_MSG))\n \t\tstrvec_push(&cmd.args, \"--cleanup=strip\");\n+\tif ((flags & VERBATIM_MSG))\n+\t\tstrvec_push(&cmd.args, \"--cleanup=verbatim\");\n \tif ((flags & EDIT_MSG))\n \t\tstrvec_push(&cmd.args, \"-e\");\n \telse if (!(flags & CLEANUP_MSG) &&\n@@ -1380,6 +1386,9 @@ static int try_to_commit(struct repository *r,\n \tenum commit_msg_cleanup_mode cleanup;\n \tint res = 0;\n \n+\tif ((flags & CLEANUP_MSG) && (flags & VERBATIM_MSG))\n+\t\tBUG(\"CLEANUP_MSG and VERBATIM_MSG are mutually exclusive\");\n+\n \tif (parse_head(r, ¤t_head))\n \t\treturn -1;\n \n@@ -1454,6 +1463,8 @@ static int try_to_commit(struct repository *r,\n \n \tif (flags & CLEANUP_MSG)\n \t\tcleanup = COMMIT_MSG_CLEANUP_ALL;\n+\telse if (flags & VERBATIM_MSG)\n+\t\tcleanup = COMMIT_MSG_CLEANUP_NONE;\n \telse if ((opts->signoff || opts->record_origin) &&\n \t\t !opts->explicit_cleanup)\n \t\tcleanup = COMMIT_MSG_CLEANUP_SPACE;\n@@ -2002,7 +2013,7 @@ static int do_pick_commit(struct repository *r,\n \t\tif (!final_fixup)\n \t\t\tmsg_file = rebase_path_squash_msg();\n \t\telse if (file_exists(rebase_path_fixup_msg())) {\n-\t\t\tflags |= CLEANUP_MSG;\n+\t\t\tflags |= VERBATIM_MSG;\n \t\t\tmsg_file = rebase_path_fixup_msg();\n \t\t} else {\n \t\t\tconst char *dest = git_path_squash_msg(r);\ndiff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh\nindex 7bab6000dc7..88040bc4352 100755\n--- a/t/t3415-rebase-autosquash.sh\n+++ b/t/t3415-rebase-autosquash.sh\n@@ -440,4 +440,12 @@ test_expect_success 'fixup a fixup' '\n \ttest XZWY = $(git show | tr -cd W-Z)\n '\n \n+test_expect_success 'fixup does not clean up commit message' '\n+\toneline=\"#818\" &&\n+\tgit commit --allow-empty -m \"$oneline\" &&\n+\tgit commit --fixup HEAD --allow-empty &&\n+\tgit -c commit.cleanup=strip rebase -ki --autosquash HEAD~2 &&\n+\ttest \"$oneline\" = \"$(git show -s --format=%s)\"\n+'\n+\n test_done\n\nbase-commit: ba2aa15129e59f248d8cdd30404bc78b5178f61d\n-- \ngitgitgadget\n"},{"id":"415510","messageId":"nycvar.QRO.7.76.6.2101281714240.54@tvgsbejvaqbjf.bet","threadId":"54850","inReplyTo":"20210112204939.1095-1-martin.agren@gmail.com","subject":"Re: [PATCH v2] rebase -i: do leave commit message intact in fixup! chains","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-01-28T16:19:19Z","receivedAt":"2021-01-28T16:21:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Martin,\n\nOn Tue, 12 Jan 2021, Martin Ågren wrote:\n\n> Johannes Schindelin via GitGitGadget wrote:\n> >\n> > We actually need to actively suppress that clean-up lest a configured\n> > `commit.cleanup` may interfere with what we want to do: leave the commit\n> > message unchanged.\n>\n> >     Changes since v1:>\n> >      * The fix now works even if commit.cleanup = commit\n>\n> Indeed. FWIW, this patch looks good to me.\n>\n> There is the lone remaining user of `CLEANUP_MSG` where we're handling\n> the skipping of the final fixup in a chain and which is part of a larger\n> block of code to handle various cases like that around `git rebase\n> --skip`. I wrote some tests on top of your patch to see what happens and\n> try to understand what's going on. The tests are below.\n>\n> I can't say I grok all that's going on in the implementation. By the\n> time we're finalizing the commit message, it seems to me that we've lost\n> track of which of the lines that look like comments are indeed comment\n> lines added by us and which actually originate in the commit messages\n> we're trying to rebase and which just happen to begin with a comment\n> character.\n\nIndeed, we lost that information. For what it's worth, that's totally in\nline with how `--amend` works:\n\n\tgit commit --allow-empty -m '#hash'\n\tgit commit --allow-empty --amend\n\nThis will interpret the `#hash` line as a comment.\n\nAnd since that's the case with `git commit`, I would contend that this is\na different issue than the one I am trying to address in this patch\nseries, and therefore should not be handled here (not even adding the\n`test_expect_failure` cases you generously provided).\n\nCiao,\nDscho\n"},{"id":"415515","messageId":"nycvar.QRO.7.76.6.2101281724130.54@tvgsbejvaqbjf.bet","threadId":"54850","inReplyTo":"xmqqwnwhzr54.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] rebase -i: do leave commit message intact in fixup! chains","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-01-28T16:24:31Z","receivedAt":"2021-01-28T16:26:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 12 Jan 2021, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > We actually need to actively suppress that clean-up lest a configured\n> > `commit.cleanup` may interfere with what we want to do: leave the commit\n> > message unchanged.\n>\n> Good thinking.\n>\n> > Reported-by: Vojtěch Knyttl \n> > Helped-by: Martin Ågren \n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > ...\n> > diff --git a/sequencer.c b/sequencer.c\n> > index 8909a467700..092e7b811f0 100644\n> > --- a/sequencer.c\n> > +++ b/sequencer.c\n> > @@ -943,6 +943,7 @@ N_(\"you have staged changes in your working tree\\n\"\n> > #define CLEANUP_MSG (1<<3)\n> > #define VERIFY_MSG (1<<4)\n> > #define CREATE_ROOT_COMMIT (1<<5)\n> > +#define VERBATIM_MSG (1<<6)\n>\n> It somewhat bothers me that these pretend to be orthogonal options\n> that can be mixed and matched, but CLEANUP and VERBATIM do not make\n> sense to be used at the same time. As long as we have some safety\n> to ensure that both bits are not used at the same time, i.e.e.g.\n>\n> \tif ((flags & (CLEANUP_MSG|VERBATIM_MSG)) == (CLEANUP_MSG|VERBATIM_MSG))\n> \t\tBUG(\"cleanup and verbatim asked at the same time\");\n>\n> it would be OK, though.\n\nI did something similar in spirit in the latest iteration.\n\nThanks,\nDscho\n"},{"id":"415523","messageId":"xmqq4kj0x1c3.fsf@gitster.c.googlers.com","threadId":"54850","inReplyTo":"nycvar.QRO.7.76.6.2101281724130.54@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2] rebase -i: do leave commit message intact in fixup! chains","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-28T20:18:04Z","receivedAt":"2021-01-28T20:20:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> > #define CLEANUP_MSG (1<<3)\n>> > #define VERIFY_MSG (1<<4)\n>> > #define CREATE_ROOT_COMMIT (1<<5)\n>> > +#define VERBATIM_MSG (1<<6)\n>>\n>> It somewhat bothers me that these pretend to be orthogonal options\n>> that can be mixed and matched, but ...\n>\n> I did something similar in spirit in the latest iteration.\n\nLooks good. We do not expect for the BUG() to trigger as long as\nprogrammers are careful, but it is better to be careful.\n\nAnother possibility would have been, as VERBATIM is what only we set\nprogrammatically when we do not want any clean-up to happen, make it\noverride CLEANUP, but what is written in the updated patch is safer,\nI would think.\n\nWill queue v3.\n\nThanks.\n\n"}]}