{"thread":{"id":"61263","subject":"[PATCH] sequencer: honor signoff opt in run_git_commit","startedAt":"2024-04-04T09:39:37Z","lastAt":"2024-04-05T11:50:44Z","messageCount":6,"participants":["David Bimmler via GitGitGadget","Junio C Hamano","Phillip Wood","David Bimmler"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"492228","messageId":"pull.1707.git.1712223572933.gitgitgadget@gmail.com","threadId":"61263","inReplyTo":null,"subject":"[PATCH] sequencer: honor signoff opt in run_git_commit","fromName":"David Bimmler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-04-04T09:39:32Z","receivedAt":"2024-04-04T09:39:37Z","isPatch":true,"sender":{"key":"david.bimmler@isovalent.com","avatar":null},"body":"From: David Bimmler <david.bimmler@isovalent.com>\n\nWhen rebasing interactively, --signoff would not take effect for commits\nwhich conflict. That is, commits applying cleanly would be signed off,\nbut commits requiring intervention would miss the sign off trailer.\n\nThe reason is that run_git_commit did not check for the signoff replay\nopt, and hence even though the option was picked up and passed\ncorrectly, the actual committing dropped the ball.\n\nThe patch adds a test specifically for this case, as well as amending a\nsquash test which codified the broken behaviour.\n\nSigned-off-by: David Bimmler <david.bimmler@isovalent.com>\n---\n    sequencer: honor signoff opt in run_git_commit\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1707%2Fbimmlerd%2Fsignoff-conflicting-commits-in-rebase-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1707/bimmlerd/signoff-conflicting-commits-in-rebase-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1707\n\n sequencer.c                     |  2 ++\n t/t3428-rebase-signoff.sh       | 33 +++++++++++++++++++++++++++++++++\n t/t3437/expected-squash-message |  2 ++\n 3 files changed, 37 insertions(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex fa838f264f5..16595e26a17 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1121,6 +1121,8 @@ static int run_git_commit(const char *defmsg,\n \t\tstrvec_pushf(&cmd.args, \"-S%s\", opts->gpg_sign);\n \telse\n \t\tstrvec_push(&cmd.args, \"--no-gpg-sign\");\n+\tif (opts->signoff)\n+\t\tstrvec_push(&cmd.args, \"--signoff\");\n \tif (defmsg)\n \t\tstrvec_pushl(&cmd.args, \"-F\", defmsg, NULL);\n \telse if (!(flags & EDIT_MSG))\ndiff --git a/t/t3428-rebase-signoff.sh b/t/t3428-rebase-signoff.sh\nindex e1b1e947647..fcecdf41978 100755\n--- a/t/t3428-rebase-signoff.sh\n+++ b/t/t3428-rebase-signoff.sh\n@@ -27,6 +27,13 @@ first\n Signed-off-by: $(git var GIT_COMMITTER_IDENT | sed -e \"s/>.*/>/\")\n EOF\n \n+# Expected signed off message after resolving the conflict\n+cat >expected-signed-after-conflict <<EOF\n+update file on side\n+\n+Signed-off-by: $(git var GIT_COMMITTER_IDENT | sed -e \"s/>.*/>/\")\n+EOF\n+\n # Expected commit message after rebase without --signoff (or with --no-signoff)\n cat >expected-unsigned <<EOF\n first\n@@ -82,4 +89,30 @@ test_expect_success 'rebase -m --signoff fails' '\n \tgit cat-file commit HEAD | sed -e \"1,/^\\$/d\" >actual &&\n \ttest_cmp expected-signed actual\n '\n+\n+test_expect_success 'rebase -i signs commits even if a conflict occurs' '\n+\tgit branch -M main &&\n+\n+\tgit branch side &&\n+\techo \"b\" >file &&\n+\tgit add file &&\n+\tgit commit -m\"update file\" &&\n+\ttest_tick &&\n+\n+\tgit checkout side &&\n+\techo \"side\" >file &&\n+\tgit add file &&\n+\tgit commit -m\"update file on side\" &&\n+\ttest_tick &&\n+\n+\ttest_must_fail git rebase -i --signoff main &&\n+\n+\techo \"merged\" >file &&\n+\tgit add file &&\n+\tgit rebase --continue &&\n+\n+\tgit cat-file commit HEAD | sed -e \"1,/^\\$/d\" >actual &&\n+\ttest_cmp expected-signed-after-conflict actual\n+'\n+\n test_done\ndiff --git a/t/t3437/expected-squash-message b/t/t3437/expected-squash-message\nindex ab2434f90ed..d74af0bcf6b 100644\n--- a/t/t3437/expected-squash-message\n+++ b/t/t3437/expected-squash-message\n@@ -48,4 +48,6 @@ edited 1\n edited 2\n \n edited 3\n+\n+Signed-off-by: Rebase Committer <rebase.committer@example.com>\n squashed\n\nbase-commit: 7774cfed6261ce2900c84e55906da708c711d601\n-- \ngitgitgadget\n"},{"id":"492250","messageId":"xmqqzfu914jc.fsf@gitster.g","threadId":"61263","inReplyTo":"pull.1707.git.1712223572933.gitgitgadget@gmail.com","subject":"Re: [PATCH] sequencer: honor signoff opt in run_git_commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-04T16:22:31Z","receivedAt":"2024-04-04T16:22:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"David Bimmler via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: David Bimmler <david.bimmler@isovalent.com>\n>\n> When rebasing interactively, --signoff would not take effect for commits\n> which conflict. That is, commits applying cleanly would be signed off,\n> but commits requiring intervention would miss the sign off trailer.\n\nGood finding.\n\n> The reason is that run_git_commit did not check for the signoff replay\n> opt, and hence even though the option was picked up and passed\n> correctly, the actual committing dropped the ball.\n\nWe record and read back the initial \"--signoff\" given from the\ncommand line (and options.signoff from the \"options sheet\" when not\nREPLAY_INTERACTIVE_REBASE) in opts->signoff, but that piece of\ninformation is used only to decide if we call append_signoff().\nWhen do_commit(), after try_to_commit() punts, calls \"git commit\",\nthe invocation lacks \"--signoff\" on the command line.\n\n    Side note: the \"let's avoid spawning 'git commit' and duplicate\n    it in-process\" codepath in try_to_commit() does not seem to pay\n    attention to opts->signoff at all, and it does not do the\n    append_signoff() to add the trailer.  Is this something this\n    patch needs to fix as well, or am I misreading the control flow\n    (e.g., perhaps opts->signoff is set we somehow refrain from\n    doing the \"let's do it in-process\" logic)?\n\nIf we forgot to tell \"git commit\" that we want the result\nsigned-off, of course it would not add the trailer.  Perhaps\n\n    -correctly, the actual committing dropped the ball\" \n    +correctly to run_git_commit(), it failed to pass it to\n    +underlying 'git commit'\n\nis less misleading.\n\n> The patch adds a test specifically for this case, as well as amending a\n> squash test which codified the broken behaviour.\n\n\"The patch adds\" -> \"Add\"\n\nI love this kind of test update that corrects a mistaken expectation\nafter a careful analysis of how the commands should behave.\n\nVery good job.  Nicely done.\n\n> diff --git a/sequencer.c b/sequencer.c\n> index fa838f264f5..16595e26a17 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -1121,6 +1121,8 @@ static int run_git_commit(const char *defmsg,\n>  \t\tstrvec_pushf(&cmd.args, \"-S%s\", opts->gpg_sign);\n>  \telse\n>  \t\tstrvec_push(&cmd.args, \"--no-gpg-sign\");\n> +\tif (opts->signoff)\n> +\t\tstrvec_push(&cmd.args, \"--signoff\");\n>  \tif (defmsg)\n>  \t\tstrvec_pushl(&cmd.args, \"-F\", defmsg, NULL);\n>  \telse if (!(flags & EDIT_MSG))\n\nThis is straight-forward and obviously correct.\n\n> diff --git a/t/t3428-rebase-signoff.sh b/t/t3428-rebase-signoff.sh\n> index e1b1e947647..fcecdf41978 100755\n> --- a/t/t3428-rebase-signoff.sh\n> +++ b/t/t3428-rebase-signoff.sh\n> @@ -27,6 +27,13 @@ first\n>  Signed-off-by: $(git var GIT_COMMITTER_IDENT | sed -e \"s/>.*/>/\")\n>  EOF\n>  \n> +# Expected signed off message after resolving the conflict\n> +cat >expected-signed-after-conflict <<EOF\n> +update file on side\n> +\n> +Signed-off-by: $(git var GIT_COMMITTER_IDENT | sed -e \"s/>.*/>/\")\n> +EOF\n> +\n\nThe t3428 script uses ancient style to prepare these expectations\noutside the test_expect_success blocks, and following the pattern\nis fine (until we clean-up the style of the whole script).  Cleaning\nit up is better done as a separate series, and when the dust settles\nafter this fix lands.  #leftoverbits \n\n>  # Expected commit message after rebase without --signoff (or with --no-signoff)\n>  cat >expected-unsigned <<EOF\n>  first\n> @@ -82,4 +89,30 @@ test_expect_success 'rebase -m --signoff fails' '\n>  \tgit cat-file commit HEAD | sed -e \"1,/^\\$/d\" >actual &&\n>  \ttest_cmp expected-signed actual\n>  '\n> +\n> +test_expect_success 'rebase -i signs commits even if a conflict occurs' '\n> +\tgit branch -M main &&\n> +\n> +\tgit branch side &&\n> +\techo \"b\" >file &&\n> +\tgit add file &&\n> +\tgit commit -m\"update file\" &&\n\nHave a SP between \"-m\" and its value, imitating existing tests.\n\n> +\ttest_tick &&\n\nWhy?  Does the rest of the test depend on commits to have different\ntimestamps?\n\nIf you want to use test_tick, the pattern to follow is to call it\n_before_ creating a commit, not after.  Up to this point in the\noriginal script nobody calls it (or test_commit that by default\ncalls it), so \"test_tick && git_commit\" would be OK.\n\nBut if there is no dependency on exact sequence of timestamps,\nletting the commit share the same initial timestamp (hardcoded in\ntest-lib.sh for better reproducibility) is more preferable, as use\nof test_tick sends a wrong message that the test results may be\naffected by the exact timestamps.\n\n> +\ttest_must_fail git rebase -i --signoff main &&\n> +\n> +\techo \"merged\" >file &&\n> +\tgit add file &&\n> +\tgit rebase --continue &&\n> +\n> +\tgit cat-file commit HEAD | sed -e \"1,/^\\$/d\" >actual &&\n> +\ttest_cmp expected-signed-after-conflict actual\n\nRunning any git command on the left hand side of a pipe is frowned\nupon, as it will hide exit status from it when it fails.\n\nIn this case, the primary thing we care about is that we have added\nthe sign off that did not exist in the original, so I wonder\n\n\tgit cat-file commit HEAD >actual &&\n\ttest_grep \"Signed-off-by: \" actual\n\nwould be sufficient?\n\n> +'\n> +\n>  test_done\n> diff --git a/t/t3437/expected-squash-message b/t/t3437/expected-squash-message\n> index ab2434f90ed..d74af0bcf6b 100644\n> --- a/t/t3437/expected-squash-message\n> +++ b/t/t3437/expected-squash-message\n> @@ -48,4 +48,6 @@ edited 1\n>  edited 2\n>  \n>  edited 3\n> +\n> +Signed-off-by: Rebase Committer <rebase.committer@example.com>\n>  squashed\n\nThis defines the expected outcome from\n\n    git rebase -ik --signoff A\n\nin t3437-rebase-fixup-options.sh; I am not sure if the location the\nsign-off is inserted is sensible, though.\n\nThanks.\n"},{"id":"492252","messageId":"xmqqr0fl13gh.fsf@gitster.g","threadId":"61263","inReplyTo":"xmqqzfu914jc.fsf@gitster.g","subject":"Re: [PATCH] sequencer: honor signoff opt in run_git_commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-04T16:45:50Z","receivedAt":"2024-04-04T16:45:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> +\tgit cat-file commit HEAD | sed -e \"1,/^\\$/d\" >actual &&\n>> +\ttest_cmp expected-signed-after-conflict actual\n>\n> Running any git command on the left hand side of a pipe is frowned\n> upon, as it will hide exit status from it when it fails.\n>\n> In this case, the primary thing we care about is that we have added\n> the sign off that did not exist in the original, so I wonder\n>\n> \tgit cat-file commit HEAD >actual &&\n> \ttest_grep \"Signed-off-by: \" actual\n>\n> would be sufficient?\n\nThe answer is No.  It is plausible that somebody else in a future\nmay think that a better fix is to always prepare the final commit\nmessage inside sequencer.c and call append_signoff(), and use it in\nboth the \"in-process commit\" codepath in try_to_commit() and in the\ncode that was fixed in the patch we are discussing.  If such a\nfuture update is done carelessly, we might end up adding duplicate\nsign off.  A \"at least one instance must be there\" test_grep would\nnot be a good tool to catch such a breakage.\n\n\tgit show -s --format=%B HEAD >actual &&\n\ttest_cmp expect actual\n\nmay be a good replacement.\n\nBut having said that, and then after having looked at the existing\ntests in the file, I see that it is littered with the same \"do not\nrun git on the upstream of the pipe\" violation.  So let's not worry\nabout this one.  The whole t3428 script needs to be cleaned up after\nthe dust settles.\n\nThanks.\n"},{"id":"492260","messageId":"a80a5aa9-34f4-4bfe-9e32-7b5e878554ba@gmail.com","threadId":"61263","inReplyTo":"xmqqzfu914jc.fsf@gitster.g","subject":"Re: [PATCH] sequencer: honor signoff opt in run_git_commit","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-04T19:21:49Z","receivedAt":"2024-04-04T19:21:52Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 04/04/2024 17:22, Junio C Hamano wrote:\n> \"David Bimmler via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> From: David Bimmler <david.bimmler@isovalent.com>\n>>\n>> When rebasing interactively, --signoff would not take effect for commits\n>> which conflict. That is, commits applying cleanly would be signed off,\n>> but commits requiring intervention would miss the sign off trailer.\n> \n> Good finding.\n\nYes, thanks for posting this patch.\n\n>> The reason is that run_git_commit did not check for the signoff replay\n>> opt, and hence even though the option was picked up and passed\n>> correctly, the actual committing dropped the ball.\n> \n> We record and read back the initial \"--signoff\" given from the\n> command line (and options.signoff from the \"options sheet\" when not\n> REPLAY_INTERACTIVE_REBASE) in opts->signoff, but that piece of\n> information is used only to decide if we call append_signoff().\n> When do_commit(), after try_to_commit() punts, calls \"git commit\",\n> the invocation lacks \"--signoff\" on the command line.\n> \n>      Side note: the \"let's avoid spawning 'git commit' and duplicate\n>      it in-process\" codepath in try_to_commit() does not seem to pay\n>      attention to opts->signoff at all, and it does not do the\n>      append_signoff() to add the trailer.  Is this something this\n>      patch needs to fix as well, or am I misreading the control flow\n>      (e.g., perhaps opts->signoff is set we somehow refrain from\n>      doing the \"let's do it in-process\" logic)?\n\nI think it is more complicated than that. We do not use the \"--signoff\" \noption of \"git commit\" because we do not want to append the \n\"Signed-off-by:\" trailer when processing \"fixup\" and \"squash\" commands. \nThe trailer is appended to the commit message by the sequencer in \ndo_pick_commits(). The problem is that when we commit a conflict \nresolution we end up using the original commit message rather than the \nfile containing the commit message that we would have used if we had not \nstopped for conflicts. I've got some patches which need their commit \nmessages cleaning up at \nhttps://github.com/phillipwood/git/commits/wip/fix-rebase-signoff-with-conflicts/ \nwhich use the correct file when committing conflict resolutions. I'll \ntry and clean them up next week.\n\nBest Wishes\n\nPhillip\n\n> If we forgot to tell \"git commit\" that we want the result\n> signed-off, of course it would not add the trailer.  Perhaps\n> \n>      -correctly, the actual committing dropped the ball\"\n>      +correctly to run_git_commit(), it failed to pass it to\n>      +underlying 'git commit'\n> \n> is less misleading.\n> \n>> The patch adds a test specifically for this case, as well as amending a\n>> squash test which codified the broken behaviour.\n> \n> \"The patch adds\" -> \"Add\"\n> \n> I love this kind of test update that corrects a mistaken expectation\n> after a careful analysis of how the commands should behave.\n> \n> Very good job.  Nicely done.\n> \n>> diff --git a/sequencer.c b/sequencer.c\n>> index fa838f264f5..16595e26a17 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -1121,6 +1121,8 @@ static int run_git_commit(const char *defmsg,\n>>   \t\tstrvec_pushf(&cmd.args, \"-S%s\", opts->gpg_sign);\n>>   \telse\n>>   \t\tstrvec_push(&cmd.args, \"--no-gpg-sign\");\n>> +\tif (opts->signoff)\n>> +\t\tstrvec_push(&cmd.args, \"--signoff\");\n>>   \tif (defmsg)\n>>   \t\tstrvec_pushl(&cmd.args, \"-F\", defmsg, NULL);\n>>   \telse if (!(flags & EDIT_MSG))\n> \n> This is straight-forward and obviously correct.\n> \n>> diff --git a/t/t3428-rebase-signoff.sh b/t/t3428-rebase-signoff.sh\n>> index e1b1e947647..fcecdf41978 100755\n>> --- a/t/t3428-rebase-signoff.sh\n>> +++ b/t/t3428-rebase-signoff.sh\n>> @@ -27,6 +27,13 @@ first\n>>   Signed-off-by: $(git var GIT_COMMITTER_IDENT | sed -e \"s/>.*/>/\")\n>>   EOF\n>>   \n>> +# Expected signed off message after resolving the conflict\n>> +cat >expected-signed-after-conflict <<EOF\n>> +update file on side\n>> +\n>> +Signed-off-by: $(git var GIT_COMMITTER_IDENT | sed -e \"s/>.*/>/\")\n>> +EOF\n>> +\n> \n> The t3428 script uses ancient style to prepare these expectations\n> outside the test_expect_success blocks, and following the pattern\n> is fine (until we clean-up the style of the whole script).  Cleaning\n> it up is better done as a separate series, and when the dust settles\n> after this fix lands.  #leftoverbits\n> \n>>   # Expected commit message after rebase without --signoff (or with --no-signoff)\n>>   cat >expected-unsigned <<EOF\n>>   first\n>> @@ -82,4 +89,30 @@ test_expect_success 'rebase -m --signoff fails' '\n>>   \tgit cat-file commit HEAD | sed -e \"1,/^\\$/d\" >actual &&\n>>   \ttest_cmp expected-signed actual\n>>   '\n>> +\n>> +test_expect_success 'rebase -i signs commits even if a conflict occurs' '\n>> +\tgit branch -M main &&\n>> +\n>> +\tgit branch side &&\n>> +\techo \"b\" >file &&\n>> +\tgit add file &&\n>> +\tgit commit -m\"update file\" &&\n> \n> Have a SP between \"-m\" and its value, imitating existing tests.\n> \n>> +\ttest_tick &&\n> \n> Why?  Does the rest of the test depend on commits to have different\n> timestamps?\n> \n> If you want to use test_tick, the pattern to follow is to call it\n> _before_ creating a commit, not after.  Up to this point in the\n> original script nobody calls it (or test_commit that by default\n> calls it), so \"test_tick && git_commit\" would be OK.\n> \n> But if there is no dependency on exact sequence of timestamps,\n> letting the commit share the same initial timestamp (hardcoded in\n> test-lib.sh for better reproducibility) is more preferable, as use\n> of test_tick sends a wrong message that the test results may be\n> affected by the exact timestamps.\n> \n>> +\ttest_must_fail git rebase -i --signoff main &&\n>> +\n>> +\techo \"merged\" >file &&\n>> +\tgit add file &&\n>> +\tgit rebase --continue &&\n>> +\n>> +\tgit cat-file commit HEAD | sed -e \"1,/^\\$/d\" >actual &&\n>> +\ttest_cmp expected-signed-after-conflict actual\n> \n> Running any git command on the left hand side of a pipe is frowned\n> upon, as it will hide exit status from it when it fails.\n> \n> In this case, the primary thing we care about is that we have added\n> the sign off that did not exist in the original, so I wonder\n> \n> \tgit cat-file commit HEAD >actual &&\n> \ttest_grep \"Signed-off-by: \" actual\n> \n> would be sufficient?\n> \n>> +'\n>> +\n>>   test_done\n>> diff --git a/t/t3437/expected-squash-message b/t/t3437/expected-squash-message\n>> index ab2434f90ed..d74af0bcf6b 100644\n>> --- a/t/t3437/expected-squash-message\n>> +++ b/t/t3437/expected-squash-message\n>> @@ -48,4 +48,6 @@ edited 1\n>>   edited 2\n>>   \n>>   edited 3\n>> +\n>> +Signed-off-by: Rebase Committer <rebase.committer@example.com>\n>>   squashed\n> \n> This defines the expected outcome from\n> \n>      git rebase -ik --signoff A\n> \n> in t3437-rebase-fixup-options.sh; I am not sure if the location the\n> sign-off is inserted is sensible, though.\n> \n> Thanks.\n> \n"},{"id":"492261","messageId":"54b91c03-5170-493a-8c0c-def2ac1f804a@gmail.com","threadId":"61263","inReplyTo":"xmqqr0fl13gh.fsf@gitster.g","subject":"Re: [PATCH] sequencer: honor signoff opt in run_git_commit","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-04T19:22:53Z","receivedAt":"2024-04-04T19:22:55Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 04/04/2024 17:45, Junio C Hamano wrote:\n> \tgit show -s --format=%B HEAD >actual &&\n> \ttest_cmp expect actual\n> \n> may be a good replacement.\n\nwe have test_commit_message to do that these days.\n\nBest Wishes\n\nPhillip\n\n> But having said that, and then after having looked at the existing\n> tests in the file, I see that it is littered with the same \"do not\n> run git on the upstream of the pipe\" violation.  So let's not worry\n> about this one.  The whole t3428 script needs to be cleaned up after\n> the dust settles.\n> \n> Thanks.\n> \n"},{"id":"492292","messageId":"6e40f19c-21c3-4a7b-876f-b8161a85e603@isovalent.com","threadId":"61263","inReplyTo":"a80a5aa9-34f4-4bfe-9e32-7b5e878554ba@gmail.com","subject":"Re: [PATCH] sequencer: honor signoff opt in run_git_commit","fromName":"David Bimmler","fromEmail":"david.bimmler@isovalent.com","sentAt":"2024-04-05T11:50:41Z","receivedAt":"2024-04-05T11:50:44Z","isPatch":true,"sender":{"key":"david.bimmler@isovalent.com","avatar":null},"body":"On 04.04.24 21:21, Phillip Wood wrote:\n\n> I think it is more complicated than that. We do not use the \"--signoff\" \n> option of \"git commit\" because we do not want to append the \n> \"Signed-off-by:\" trailer when processing \"fixup\" and \"squash\" commands. \n> The trailer is appended to the commit message by the sequencer in \n> do_pick_commits(). The problem is that when we commit a conflict \n> resolution we end up using the original commit message rather than the \n> file containing the commit message that we would have used if we had not \n> stopped for conflicts. I've got some patches which need their commit \n> messages cleaning up at \n> https://github.com/phillipwood/git/commits/wip/fix-rebase-signoff-with-conflicts/ which use the correct file when committing conflict resolutions. I'll try and clean them up next week.\n\nTook a look at that series, I agree that they fix the issue too. I \nwasn't aware of the intended behaviour for signoffs in squash and fixup, \nI'll thus not pursue this patch further.\n\nThanks for the context and review,\nDavid\n\n"}]}