{"thread":{"id":"54393","subject":"[PATCH] sequencer: fix gpg option passed to octopus merge","startedAt":"2020-10-11T22:51:06Z","lastAt":"2020-10-13T04:45:27Z","messageCount":11,"participants":["Samuel Čavoj","brian m. carlson","Johannes Schindelin","Phillip Wood","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"407311","messageId":"20201011224804.722607-1-samuel@cavoj.net","threadId":"54393","inReplyTo":null,"subject":"[PATCH] sequencer: fix gpg option passed to octopus merge","fromName":"Samuel Čavoj","fromEmail":"samuel@cavoj.net","sentAt":"2020-10-11T22:48:04Z","receivedAt":"2020-10-11T22:51:06Z","isPatch":true,"sender":{"key":"samuel@cavoj.net","avatar":"https://avatars.githubusercontent.com/u/703905?v=4"},"body":"When performing octopus merges with interactive rebase with gpgsign\nenabled (either using rebase -S or config commit.gpgsign), the operation\nwould fail on the merge. Instead of \"-S%s\" with the key id substituted,\nonly the bare key id would get passed to the underlying merge command,\nwhich tried to interpret it as a ref.\n\nSigned-off-by: Samuel Čavoj <samuel@cavoj.net>\n---\nIt is unclear to me whether I should have based this off of maint or\nmaster, master made more sense to me. I apologize if maint was the\ncorrect one, please tell and I will resubmit.\n---\n sequencer.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 00acb12496..88ccff4838 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3677,7 +3677,7 @@ static int do_merge(struct repository *r,\n \t\tstrvec_push(&cmd.args, \"-F\");\n \t\tstrvec_push(&cmd.args, git_path_merge_msg(r));\n \t\tif (opts->gpg_sign)\n-\t\t\tstrvec_push(&cmd.args, opts->gpg_sign);\n+\t\t\tstrvec_pushf(&cmd.args, \"-S%s\", opts->gpg_sign);\n \n \t\t/* Add the tips to be merged */\n \t\tfor (j = to_merge; j; j = j->next)\n-- \n2.28.0\n\n"},{"id":"407312","messageId":"20201011225850.GA490427@camp.crustytoothpaste.net","threadId":"54393","inReplyTo":"20201011224804.722607-1-samuel@cavoj.net","subject":"Re: [PATCH] sequencer: fix gpg option passed to octopus merge","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-10-11T22:58:50Z","receivedAt":"2020-10-11T22:59:00Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-10-11 at 22:48:04, Samuel Čavoj wrote:\n> When performing octopus merges with interactive rebase with gpgsign\n> enabled (either using rebase -S or config commit.gpgsign), the operation\n> would fail on the merge. Instead of \"-S%s\" with the key id substituted,\n> only the bare key id would get passed to the underlying merge command,\n> which tried to interpret it as a ref.\n> \n> Signed-off-by: Samuel Čavoj <samuel@cavoj.net>\n> ---\n> It is unclear to me whether I should have based this off of maint or\n> master, master made more sense to me. I apologize if maint was the\n> correct one, please tell and I will resubmit.\n> ---\n>  sequencer.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index 00acb12496..88ccff4838 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3677,7 +3677,7 @@ static int do_merge(struct repository *r,\n>  \t\tstrvec_push(&cmd.args, \"-F\");\n>  \t\tstrvec_push(&cmd.args, git_path_merge_msg(r));\n>  \t\tif (opts->gpg_sign)\n> -\t\t\tstrvec_push(&cmd.args, opts->gpg_sign);\n> +\t\t\tstrvec_pushf(&cmd.args, \"-S%s\", opts->gpg_sign);\n\nYeah, this seems obviously correct, and it's very similar to what we do\nelsewhere in the file.  This will also handle the case where the option\nis empty (because we want to do autodetection of the key to sign)\ncorrectly as well.\n-- \nbrian m. carlson (he/him or they/them)\nHouston, Texas, US\n"},{"id":"407322","messageId":"nycvar.QRO.7.76.6.2010121233590.50@tvgsbejvaqbjf.bet","threadId":"54393","inReplyTo":"20201011225850.GA490427@camp.crustytoothpaste.net","subject":"Re: [PATCH] sequencer: fix gpg option passed to octopus merge","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-12T10:34:32Z","receivedAt":"2020-10-12T10:34:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 11 Oct 2020, brian m. carlson wrote:\n\n> On 2020-10-11 at 22:48:04, Samuel Čavoj wrote:\n> > When performing octopus merges with interactive rebase with gpgsign\n> > enabled (either using rebase -S or config commit.gpgsign), the operation\n> > would fail on the merge. Instead of \"-S%s\" with the key id substituted,\n> > only the bare key id would get passed to the underlying merge command,\n> > which tried to interpret it as a ref.\n> >\n> > Signed-off-by: Samuel Čavoj <samuel@cavoj.net>\n> > ---\n> > It is unclear to me whether I should have based this off of maint or\n> > master, master made more sense to me. I apologize if maint was the\n> > correct one, please tell and I will resubmit.\n> > ---\n> >  sequencer.c | 2 +-\n> >  1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/sequencer.c b/sequencer.c\n> > index 00acb12496..88ccff4838 100644\n> > --- a/sequencer.c\n> > +++ b/sequencer.c\n> > @@ -3677,7 +3677,7 @@ static int do_merge(struct repository *r,\n> >  \t\tstrvec_push(&cmd.args, \"-F\");\n> >  \t\tstrvec_push(&cmd.args, git_path_merge_msg(r));\n> >  \t\tif (opts->gpg_sign)\n> > -\t\t\tstrvec_push(&cmd.args, opts->gpg_sign);\n> > +\t\t\tstrvec_pushf(&cmd.args, \"-S%s\", opts->gpg_sign);\n>\n> Yeah, this seems obviously correct, and it's very similar to what we do\n> elsewhere in the file.  This will also handle the case where the option\n> is empty (because we want to do autodetection of the key to sign)\n> correctly as well.\n\nACK\n\nIt is unclear to me whether we want to bother introducing a test case for\nthis; Octopus merges are somewhat rare...\n\nCiao,\nDscho\n"},{"id":"407332","messageId":"6f650960-6b32-4be5-b909-86685c6ad5ea@gmail.com","threadId":"54393","inReplyTo":"nycvar.QRO.7.76.6.2010121233590.50@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] sequencer: fix gpg option passed to octopus merge","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-10-12T13:15:09Z","receivedAt":"2020-10-12T13:15:17Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Thanks for the patch Samuel\n\nOn 12/10/2020 11:34, Johannes Schindelin wrote:\n> Hi,\n> \n> On Sun, 11 Oct 2020, brian m. carlson wrote:\n> \n>> On 2020-10-11 at 22:48:04, Samuel Čavoj wrote:\n>>> When performing octopus merges with interactive rebase with gpgsign\n>>> enabled (either using rebase -S or config commit.gpgsign), the operation\n>>> would fail on the merge. Instead of \"-S%s\" with the key id substituted,\n>>> only the bare key id would get passed to the underlying merge command,\n>>> which tried to interpret it as a ref.\n>>>\n>>> Signed-off-by: Samuel Čavoj <samuel@cavoj.net>\n>>> ---\n>>> It is unclear to me whether I should have based this off of maint or\n>>> master, master made more sense to me. I apologize if maint was the\n>>> correct one, please tell and I will resubmit.\n>>> ---\n>>>   sequencer.c | 2 +-\n>>>   1 file changed, 1 insertion(+), 1 deletion(-)\n>>>\n>>> diff --git a/sequencer.c b/sequencer.c\n>>> index 00acb12496..88ccff4838 100644\n>>> --- a/sequencer.c\n>>> +++ b/sequencer.c\n>>> @@ -3677,7 +3677,7 @@ static int do_merge(struct repository *r,\n>>>   \t\tstrvec_push(&cmd.args, \"-F\");\n>>>   \t\tstrvec_push(&cmd.args, git_path_merge_msg(r));\n>>>   \t\tif (opts->gpg_sign)\n>>> -\t\t\tstrvec_push(&cmd.args, opts->gpg_sign);\n>>> +\t\t\tstrvec_pushf(&cmd.args, \"-S%s\", opts->gpg_sign);\n>>\n>> Yeah, this seems obviously correct, and it's very similar to what we do\n>> elsewhere in the file.\nIn run_git_commit() we do\n\n\tif (opts->gpg_sign)\n\t\tstrvec_pushf(&cmd.args, \"-S%s\", opts->gpg_sign);\n\telse\n\t\tstrvec_push(&cmd.args, \"--no-gpg-sign\");\n\nI'm not immediately clear why we pass --no-gpg-sign when opts->gpg_sign \nisn't set but it makes me wonder if we should be doing that here as well\n\n>  This will also handle the case where the option\n>> is empty (because we want to do autodetection of the key to sign)\n>> correctly as well.\n> \n> ACK\n> \n> It is unclear to me whether we want to bother introducing a test case for\n> this; Octopus merges are somewhat rare...\n\nThis code path is used whenever the user specifies a merge strategy \nother than \"recursive\" or they pass merge strategy options to any merge \nstrategy including \"recursive\" so while octopus merges may be rare the \nunion of everything other than a plain recursive merge may not be.\n\nBest Wishes\n\nPhillip\n\n> \n> Ciao,\n> Dscho\n> \n\n"},{"id":"407348","messageId":"xmqq5z7fqt6k.fsf@gitster.c.googlers.com","threadId":"54393","inReplyTo":"6f650960-6b32-4be5-b909-86685c6ad5ea@gmail.com","subject":"Re: [PATCH] sequencer: fix gpg option passed to octopus merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-12T16:56:35Z","receivedAt":"2020-10-12T16:56:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> In run_git_commit() we do\n>\n> \tif (opts->gpg_sign)\n> \t\tstrvec_pushf(&cmd.args, \"-S%s\", opts->gpg_sign);\n> \telse\n> \t\tstrvec_push(&cmd.args, \"--no-gpg-sign\");\n>\n> I'm not immediately clear why we pass --no-gpg-sign when\n> opts->gpg_sign isn't set ...\n\nIsn't it because there is a configuration that the &cmd may honor\nthat forces gpg signing all the time?\n\n> but it makes me wonder if we should be doing\n> that here as well\n\nPossibly.\n"},{"id":"407370","messageId":"865afe23-3e83-59a0-76ce-fee91f3fb554@gmail.com","threadId":"54393","inReplyTo":"xmqq5z7fqt6k.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] sequencer: fix gpg option passed to octopus merge","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-10-12T19:26:57Z","receivedAt":"2020-10-12T19:27:04Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 12/10/2020 17:56, Junio C Hamano wrote:\n> Phillip Wood <phillip.wood123@gmail.com> writes:\n> \n>> In run_git_commit() we do\n>>\n>> \tif (opts->gpg_sign)\n>> \t\tstrvec_pushf(&cmd.args, \"-S%s\", opts->gpg_sign);\n>> \telse\n>> \t\tstrvec_push(&cmd.args, \"--no-gpg-sign\");\n>>\n>> I'm not immediately clear why we pass --no-gpg-sign when\n>> opts->gpg_sign isn't set ...\n> \n> Isn't it because there is a configuration that the &cmd may honor\n> that forces gpg signing all the time?\n\nYes you're right, so we should be passing --no-gpg-sign here if \nopts->gpg_sign == NULL, otherwise `git merge` will still sign commits \nwhen it is run by `git rebase --no-gpg-sign`\n\nBest Wishes\n\nPhillip\n\n> \n>> but it makes me wonder if we should be doing\n>> that here as well\n> \n> Possibly.\n> \n"},{"id":"407375","messageId":"xmqq362jp6yo.fsf@gitster.c.googlers.com","threadId":"54393","inReplyTo":"xmqq5z7fqt6k.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] sequencer: fix gpg option passed to octopus merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-12T19:41:51Z","receivedAt":"2020-10-12T19:41:56Z","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> Phillip Wood <phillip.wood123@gmail.com> writes:\n>\n>> In run_git_commit() we do\n>>\n>> \tif (opts->gpg_sign)\n>> \t\tstrvec_pushf(&cmd.args, \"-S%s\", opts->gpg_sign);\n>> \telse\n>> \t\tstrvec_push(&cmd.args, \"--no-gpg-sign\");\n>>\n>> I'm not immediately clear why we pass --no-gpg-sign when\n>> opts->gpg_sign isn't set ...\n>\n> Isn't it because there is a configuration that the &cmd may honor\n> that forces gpg signing all the time?\n>\n>> but it makes me wonder if we should be doing\n>> that here as well\n\nI was reacting only based on what I saw in these message, but it\nturns out that cmd above is an internal invocation of \"git merge\";\nas the command does honor the \"commit.gpgsign\" option, if \"rebase\"\nor whatever command that invoked the sequencer turned off the\nsigning by setting opts->gpg_sign to false, I agree that the part\ntouched by the patch should have the else clause to explicitly\noverride the configuration option.\n\n"},{"id":"407376","messageId":"xmqqy2kbns5l.fsf@gitster.c.googlers.com","threadId":"54393","inReplyTo":"20201011224804.722607-1-samuel@cavoj.net","subject":"Re: [PATCH] sequencer: fix gpg option passed to octopus merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-12T19:47:02Z","receivedAt":"2020-10-12T19:47:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Samuel Čavoj <samuel@cavoj.net> writes:\n\n> Subject: Re: [PATCH] sequencer: fix gpg option passed to octopus merge\n\nPuzzling.  Why do you single out octopus merge like this?\n\nsequencer.c::do_merge() is called from pick_commits() whenever we\nsee a \"merge\" insn, and not limited to an octopus merge.\n\nCan we have a test to demonstrate the existing failure, so that we\ncan notice if this fix is broken in the future?\n\n> When performing octopus merges with interactive rebase with gpgsign\n> enabled (either using rebase -S or config commit.gpgsign), the operation\n> would fail on the merge. Instead of \"-S%s\" with the key id substituted,\n> only the bare key id would get passed to the underlying merge command,\n> which tried to interpret it as a ref.\n>\n> Signed-off-by: Samuel Čavoj <samuel@cavoj.net>\n> ---\n> It is unclear to me whether I should have based this off of maint or\n> master, master made more sense to me. I apologize if maint was the\n> correct one, please tell and I will resubmit.\n> ---\n>  sequencer.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 00acb12496..88ccff4838 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3677,7 +3677,7 @@ static int do_merge(struct repository *r,\n>  \t\tstrvec_push(&cmd.args, \"-F\");\n>  \t\tstrvec_push(&cmd.args, git_path_merge_msg(r));\n>  \t\tif (opts->gpg_sign)\n> -\t\t\tstrvec_push(&cmd.args, opts->gpg_sign);\n> +\t\t\tstrvec_pushf(&cmd.args, \"-S%s\", opts->gpg_sign);\n>  \n>  \t\t/* Add the tips to be merged */\n>  \t\tfor (j = to_merge; j; j = j->next)\n"},{"id":"407382","messageId":"xmqqft6jnr0h.fsf@gitster.c.googlers.com","threadId":"54393","inReplyTo":"xmqq362jp6yo.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] sequencer: fix gpg option passed to octopus merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-12T20:11:42Z","receivedAt":"2020-10-12T20:11:49Z","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> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Phillip Wood <phillip.wood123@gmail.com> writes:\n>>\n>>> In run_git_commit() we do\n>>>\n>>> \tif (opts->gpg_sign)\n>>> \t\tstrvec_pushf(&cmd.args, \"-S%s\", opts->gpg_sign);\n>>> \telse\n>>> \t\tstrvec_push(&cmd.args, \"--no-gpg-sign\");\n>>>\n>>> I'm not immediately clear why we pass --no-gpg-sign when\n>>> opts->gpg_sign isn't set ...\n>>\n>> Isn't it because there is a configuration that the &cmd may honor\n>> that forces gpg signing all the time?\n>>\n>>> but it makes me wonder if we should be doing\n>>> that here as well\n>\n> I was reacting only based on what I saw in these message, but it\n> turns out that cmd above is an internal invocation of \"git merge\";\n> as the command does honor the \"commit.gpgsign\" option, if \"rebase\"\n> or whatever command that invoked the sequencer turned off the\n> signing by setting opts->gpg_sign to false, I agree that the part\n> touched by the patch should have the else clause to explicitly\n> override the configuration option.\n\nForgot to say that it is perfectly fine to leave it outside Samuel's\npatch.  It \"fixes\" the \"intended\" behaviour of the current design\nwhere it somehow was deemed a good idea to disallow overriding\ncommit.gpgsign variable.  Fixing that design so that we can override\nconfigured default from the command line can be left for a follow-up\npatch.\n\nThanks.\n"},{"id":"407392","messageId":"nycvar.QRO.7.76.6.2010122239430.50@tvgsbejvaqbjf.bet","threadId":"54393","inReplyTo":"xmqqy2kbns5l.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] sequencer: fix gpg option passed to octopus merge","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-12T20:55:42Z","receivedAt":"2020-10-12T20:56:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 12 Oct 2020, Junio C Hamano wrote:\n\n> Samuel Čavoj <samuel@cavoj.net> writes:\n>\n> > Subject: Re: [PATCH] sequencer: fix gpg option passed to octopus merge\n>\n> Puzzling.  Why do you single out octopus merge like this?\n>\n> sequencer.c::do_merge() is called from pick_commits() whenever we\n> see a \"merge\" insn, and not limited to an octopus merge.\n>\n> Can we have a test to demonstrate the existing failure, so that we\n> can notice if this fix is broken in the future?\n\nYes, now that I understand that not only octopus merges are affected, I\nwould be very much in favor of adding a test case.\n\nIf I may suggest to add a new test case to t3435 based on t3430's\n'--rebase-merges with strategies' and t3404's 'rebase -i\n--gpg-sign=<key-id>'? Something along these lines:\n\n-- snipsnap --\ndiff --git a/sequencer.c b/sequencer.c\nindex 00acb124962..88ccff4838d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3677,7 +3677,7 @@ static int do_merge(struct repository *r,\n \t\tstrvec_push(&cmd.args, \"-F\");\n \t\tstrvec_push(&cmd.args, git_path_merge_msg(r));\n \t\tif (opts->gpg_sign)\n-\t\t\tstrvec_push(&cmd.args, opts->gpg_sign);\n+\t\t\tstrvec_pushf(&cmd.args, \"-S%s\", opts->gpg_sign);\n\n \t\t/* Add the tips to be merged */\n \t\tfor (j = to_merge; j; j = j->next)\ndiff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh\nindex b47c59c190b..f70b280f5c1 100755\n--- a/t/t3435-rebase-gpg-sign.sh\n+++ b/t/t3435-rebase-gpg-sign.sh\n@@ -68,4 +68,10 @@ test_expect_failure 'rebase -p --no-gpg-sign override commit.gpgsign' '\n \ttest_must_fail git verify-commit HEAD\n '\n\n+test_expect_success 'rebase -r, GPG and merge strategies' '\n+\tgit reset --hard merged &&\n+\tgit rebase -fr --gpg-sign -s resolve --root &&\n+\tgit verify-commit HEAD\n+'\n+\n test_done\n"},{"id":"407420","messageId":"xmqq362in38e.fsf@gitster.c.googlers.com","threadId":"54393","inReplyTo":"nycvar.QRO.7.76.6.2010122239430.50@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] sequencer: fix gpg option passed to octopus merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-13T04:45:21Z","receivedAt":"2020-10-13T04:45:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> Puzzling.  Why do you single out octopus merge like this?\n>>\n>> sequencer.c::do_merge() is called from pick_commits() whenever we\n>> see a \"merge\" insn, and not limited to an octopus merge.\n>>\n>> Can we have a test to demonstrate the existing failure, so that we\n>> can notice if this fix is broken in the future?\n>\n> Yes, now that I understand that not only octopus merges are affected, I\n> would be very much in favor of adding a test case.\n\nAh, I see why the initial description was focused on octopus.  The\ncode special cases the two-parent merge using the recursive strategy\nand uses completely separate codepath for it, which does not have\nthis problem but the codepath that handles octopus merges and merges\nwith the non-default strategy have the problem.\n\n> If I may suggest to add a new test case to t3435 based on t3430's\n> '--rebase-merges with strategies' and t3404's 'rebase -i\n> --gpg-sign=<key-id>'? Something along these lines:\n\nIn addition, a test that passes --no-gpg-sign from the command line, \nbecause commit.gpgsign is set in the repository used in these tests,\nwould be necessary---we want to make sure the command line overrides\nthe configured default (which is the topic of [patch 2/1]).\n\n>\n> -- snipsnap --\n> diff --git a/sequencer.c b/sequencer.c\n> index 00acb124962..88ccff4838d 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3677,7 +3677,7 @@ static int do_merge(struct repository *r,\n>  \t\tstrvec_push(&cmd.args, \"-F\");\n>  \t\tstrvec_push(&cmd.args, git_path_merge_msg(r));\n>  \t\tif (opts->gpg_sign)\n> -\t\t\tstrvec_push(&cmd.args, opts->gpg_sign);\n> +\t\t\tstrvec_pushf(&cmd.args, \"-S%s\", opts->gpg_sign);\n>\n>  \t\t/* Add the tips to be merged */\n>  \t\tfor (j = to_merge; j; j = j->next)\n> diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh\n> index b47c59c190b..f70b280f5c1 100755\n> --- a/t/t3435-rebase-gpg-sign.sh\n> +++ b/t/t3435-rebase-gpg-sign.sh\n> @@ -68,4 +68,10 @@ test_expect_failure 'rebase -p --no-gpg-sign override commit.gpgsign' '\n>  \ttest_must_fail git verify-commit HEAD\n>  '\n>\n> +test_expect_success 'rebase -r, GPG and merge strategies' '\n> +\tgit reset --hard merged &&\n> +\tgit rebase -fr --gpg-sign -s resolve --root &&\n> +\tgit verify-commit HEAD\n> +'\n> +\n>  test_done\n"}]}