[PATCH] sequencer: fix gpg option passed to octopus merge

[PATCH] sequencer: fix gpg option passed to octopus merge

[Samuel Čavoj]

When performing octopus merges with interactive rebase with gpgsign
enabled (either using rebase -S or config commit.gpgsign), the operation
would fail on the merge. Instead of "-S%s" with the key id substituted,
only the bare key id would get passed to the underlying merge command,
which tried to interpret it as a ref.

Signed-off-by: Samuel Čavoj <samuel@cavoj.net>
---
It is unclear to me whether I should have based this off of maint or
master, master made more sense to me. I apologize if maint was the
correct one, please tell and I will resubmit.
---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 00acb12496..88ccff4838 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3677,7 +3677,7 @@ static int do_merge(struct repository *r,
 		strvec_push(&cmd.args, "-F");
 		strvec_push(&cmd.args, git_path_merge_msg(r));
 		if (opts->gpg_sign)
-			strvec_push(&cmd.args, opts->gpg_sign);
+			strvec_pushf(&cmd.args, "-S%s", opts->gpg_sign);
 
 		/* Add the tips to be merged */
 		for (j = to_merge; j; j = j->next)
-- 
2.28.0

Re: [PATCH] sequencer: fix gpg option passed to octopus merge

[brian m. carlson]

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

On 2020-10-11 at 22:48:04, Samuel Čavoj wrote:
> When performing octopus merges with interactive rebase with gpgsign
> enabled (either using rebase -S or config commit.gpgsign), the operation
> would fail on the merge. Instead of "-S%s" with the key id substituted,
> only the bare key id would get passed to the underlying merge command,
> which tried to interpret it as a ref.
> 
> Signed-off-by: Samuel Čavoj <samuel@cavoj.net>
> ---
> It is unclear to me whether I should have based this off of maint or
> master, master made more sense to me. I apologize if maint was the
> correct one, please tell and I will resubmit.
> ---
>  sequencer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 00acb12496..88ccff4838 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3677,7 +3677,7 @@ static int do_merge(struct repository *r,
>  		strvec_push(&cmd.args, "-F");
>  		strvec_push(&cmd.args, git_path_merge_msg(r));
>  		if (opts->gpg_sign)
> -			strvec_push(&cmd.args, opts->gpg_sign);
> +			strvec_pushf(&cmd.args, "-S%s", opts->gpg_sign);

Yeah, this seems obviously correct, and it's very similar to what we do
elsewhere in the file.  This will also handle the case where the option
is empty (because we want to do autodetection of the key to sign)
correctly as well.
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH] sequencer: fix gpg option passed to octopus merge

[Johannes Schindelin]

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

Hi,

On Sun, 11 Oct 2020, brian m. carlson wrote:

> On 2020-10-11 at 22:48:04, Samuel Čavoj wrote:
> > When performing octopus merges with interactive rebase with gpgsign
> > enabled (either using rebase -S or config commit.gpgsign), the operation
> > would fail on the merge. Instead of "-S%s" with the key id substituted,
> > only the bare key id would get passed to the underlying merge command,
> > which tried to interpret it as a ref.
> >
> > Signed-off-by: Samuel Čavoj <samuel@cavoj.net>
> > ---
> > It is unclear to me whether I should have based this off of maint or
> > master, master made more sense to me. I apologize if maint was the
> > correct one, please tell and I will resubmit.
> > ---
> >  sequencer.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/sequencer.c b/sequencer.c
> > index 00acb12496..88ccff4838 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -3677,7 +3677,7 @@ static int do_merge(struct repository *r,
> >  		strvec_push(&cmd.args, "-F");
> >  		strvec_push(&cmd.args, git_path_merge_msg(r));
> >  		if (opts->gpg_sign)
> > -			strvec_push(&cmd.args, opts->gpg_sign);
> > +			strvec_pushf(&cmd.args, "-S%s", opts->gpg_sign);
>
> Yeah, this seems obviously correct, and it's very similar to what we do
> elsewhere in the file.  This will also handle the case where the option
> is empty (because we want to do autodetection of the key to sign)
> correctly as well.

ACK

It is unclear to me whether we want to bother introducing a test case for
this; Octopus merges are somewhat rare...

Ciao,
Dscho

Re: [PATCH] sequencer: fix gpg option passed to octopus merge

[Phillip Wood]

Thanks for the patch Samuel

On 12/10/2020 11:34, Johannes Schindelin wrote:
> Hi,
> 
> On Sun, 11 Oct 2020, brian m. carlson wrote:
> 
>> On 2020-10-11 at 22:48:04, Samuel Čavoj wrote:
>>> When performing octopus merges with interactive rebase with gpgsign
>>> enabled (either using rebase -S or config commit.gpgsign), the operation
>>> would fail on the merge. Instead of "-S%s" with the key id substituted,
>>> only the bare key id would get passed to the underlying merge command,
>>> which tried to interpret it as a ref.
>>>
>>> Signed-off-by: Samuel Čavoj <samuel@cavoj.net>
>>> ---
>>> It is unclear to me whether I should have based this off of maint or
>>> master, master made more sense to me. I apologize if maint was the
>>> correct one, please tell and I will resubmit.
>>> ---
>>>   sequencer.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/sequencer.c b/sequencer.c
>>> index 00acb12496..88ccff4838 100644
>>> --- a/sequencer.c
>>> +++ b/sequencer.c
>>> @@ -3677,7 +3677,7 @@ static int do_merge(struct repository *r,
>>>   		strvec_push(&cmd.args, "-F");
>>>   		strvec_push(&cmd.args, git_path_merge_msg(r));
>>>   		if (opts->gpg_sign)
>>> -			strvec_push(&cmd.args, opts->gpg_sign);
>>> +			strvec_pushf(&cmd.args, "-S%s", opts->gpg_sign);
>>
>> Yeah, this seems obviously correct, and it's very similar to what we do
>> elsewhere in the file.
In run_git_commit() we do

	if (opts->gpg_sign)
		strvec_pushf(&cmd.args, "-S%s", opts->gpg_sign);
	else
		strvec_push(&cmd.args, "--no-gpg-sign");

I'm not immediately clear why we pass --no-gpg-sign when opts->gpg_sign 
isn't set but it makes me wonder if we should be doing that here as well

>  This will also handle the case where the option
>> is empty (because we want to do autodetection of the key to sign)
>> correctly as well.
> 
> ACK
> 
> It is unclear to me whether we want to bother introducing a test case for
> this; Octopus merges are somewhat rare...

This code path is used whenever the user specifies a merge strategy 
other than "recursive" or they pass merge strategy options to any merge 
strategy including "recursive" so while octopus merges may be rare the 
union of everything other than a plain recursive merge may not be.

Best Wishes

Phillip

> 
> Ciao,
> Dscho
> 

Re: [PATCH] sequencer: fix gpg option passed to octopus merge

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> In run_git_commit() we do
>
> 	if (opts->gpg_sign)
> 		strvec_pushf(&cmd.args, "-S%s", opts->gpg_sign);
> 	else
> 		strvec_push(&cmd.args, "--no-gpg-sign");
>
> I'm not immediately clear why we pass --no-gpg-sign when
> opts->gpg_sign isn't set ...

Isn't it because there is a configuration that the &cmd may honor
that forces gpg signing all the time?

> but it makes me wonder if we should be doing
> that here as well

Possibly.

Re: [PATCH] sequencer: fix gpg option passed to octopus merge

[Phillip Wood]

On 12/10/2020 17:56, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> In run_git_commit() we do
>>
>> 	if (opts->gpg_sign)
>> 		strvec_pushf(&cmd.args, "-S%s", opts->gpg_sign);
>> 	else
>> 		strvec_push(&cmd.args, "--no-gpg-sign");
>>
>> I'm not immediately clear why we pass --no-gpg-sign when
>> opts->gpg_sign isn't set ...
> 
> Isn't it because there is a configuration that the &cmd may honor
> that forces gpg signing all the time?

Yes you're right, so we should be passing --no-gpg-sign here if 
opts->gpg_sign == NULL, otherwise `git merge` will still sign commits 
when it is run by `git rebase --no-gpg-sign`

Best Wishes

Phillip

> 
>> but it makes me wonder if we should be doing
>> that here as well
> 
> Possibly.
> 

Re: [PATCH] sequencer: fix gpg option passed to octopus merge

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>> In run_git_commit() we do
>>
>> 	if (opts->gpg_sign)
>> 		strvec_pushf(&cmd.args, "-S%s", opts->gpg_sign);
>> 	else
>> 		strvec_push(&cmd.args, "--no-gpg-sign");
>>
>> I'm not immediately clear why we pass --no-gpg-sign when
>> opts->gpg_sign isn't set ...
>
> Isn't it because there is a configuration that the &cmd may honor
> that forces gpg signing all the time?
>
>> but it makes me wonder if we should be doing
>> that here as well

I was reacting only based on what I saw in these message, but it
turns out that cmd above is an internal invocation of "git merge";
as the command does honor the "commit.gpgsign" option, if "rebase"
or whatever command that invoked the sequencer turned off the
signing by setting opts->gpg_sign to false, I agree that the part
touched by the patch should have the else clause to explicitly
override the configuration option.

Re: [PATCH] sequencer: fix gpg option passed to octopus merge

[Junio C Hamano]

Samuel Čavoj <samuel@cavoj.net> writes:

> Subject: Re: [PATCH] sequencer: fix gpg option passed to octopus merge

Puzzling.  Why do you single out octopus merge like this?

sequencer.c::do_merge() is called from pick_commits() whenever we
see a "merge" insn, and not limited to an octopus merge.

Can we have a test to demonstrate the existing failure, so that we
can notice if this fix is broken in the future?

> When performing octopus merges with interactive rebase with gpgsign
> enabled (either using rebase -S or config commit.gpgsign), the operation
> would fail on the merge. Instead of "-S%s" with the key id substituted,
> only the bare key id would get passed to the underlying merge command,
> which tried to interpret it as a ref.
>
> Signed-off-by: Samuel Čavoj <samuel@cavoj.net>
> ---
> It is unclear to me whether I should have based this off of maint or
> master, master made more sense to me. I apologize if maint was the
> correct one, please tell and I will resubmit.
> ---
>  sequencer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 00acb12496..88ccff4838 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3677,7 +3677,7 @@ static int do_merge(struct repository *r,
>  		strvec_push(&cmd.args, "-F");
>  		strvec_push(&cmd.args, git_path_merge_msg(r));
>  		if (opts->gpg_sign)
> -			strvec_push(&cmd.args, opts->gpg_sign);
> +			strvec_pushf(&cmd.args, "-S%s", opts->gpg_sign);
>  
>  		/* Add the tips to be merged */
>  		for (j = to_merge; j; j = j->next)

Re: [PATCH] sequencer: fix gpg option passed to octopus merge

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>>
>>> In run_git_commit() we do
>>>
>>> 	if (opts->gpg_sign)
>>> 		strvec_pushf(&cmd.args, "-S%s", opts->gpg_sign);
>>> 	else
>>> 		strvec_push(&cmd.args, "--no-gpg-sign");
>>>
>>> I'm not immediately clear why we pass --no-gpg-sign when
>>> opts->gpg_sign isn't set ...
>>
>> Isn't it because there is a configuration that the &cmd may honor
>> that forces gpg signing all the time?
>>
>>> but it makes me wonder if we should be doing
>>> that here as well
>
> I was reacting only based on what I saw in these message, but it
> turns out that cmd above is an internal invocation of "git merge";
> as the command does honor the "commit.gpgsign" option, if "rebase"
> or whatever command that invoked the sequencer turned off the
> signing by setting opts->gpg_sign to false, I agree that the part
> touched by the patch should have the else clause to explicitly
> override the configuration option.

Forgot to say that it is perfectly fine to leave it outside Samuel's
patch.  It "fixes" the "intended" behaviour of the current design
where it somehow was deemed a good idea to disallow overriding
commit.gpgsign variable.  Fixing that design so that we can override
configured default from the command line can be left for a follow-up
patch.

Thanks.

Re: [PATCH] sequencer: fix gpg option passed to octopus merge

[Johannes Schindelin]

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

Hi,

On Mon, 12 Oct 2020, Junio C Hamano wrote:

> Samuel Čavoj <samuel@cavoj.net> writes:
>
> > Subject: Re: [PATCH] sequencer: fix gpg option passed to octopus merge
>
> Puzzling.  Why do you single out octopus merge like this?
>
> sequencer.c::do_merge() is called from pick_commits() whenever we
> see a "merge" insn, and not limited to an octopus merge.
>
> Can we have a test to demonstrate the existing failure, so that we
> can notice if this fix is broken in the future?

Yes, now that I understand that not only octopus merges are affected, I
would be very much in favor of adding a test case.

If I may suggest to add a new test case to t3435 based on t3430's
'--rebase-merges with strategies' and t3404's 'rebase -i
--gpg-sign=<key-id>'? Something along these lines:

-- snipsnap --
diff --git a/sequencer.c b/sequencer.c
index 00acb124962..88ccff4838d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3677,7 +3677,7 @@ static int do_merge(struct repository *r,
 		strvec_push(&cmd.args, "-F");
 		strvec_push(&cmd.args, git_path_merge_msg(r));
 		if (opts->gpg_sign)
-			strvec_push(&cmd.args, opts->gpg_sign);
+			strvec_pushf(&cmd.args, "-S%s", opts->gpg_sign);

 		/* Add the tips to be merged */
 		for (j = to_merge; j; j = j->next)
diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh
index b47c59c190b..f70b280f5c1 100755
--- a/t/t3435-rebase-gpg-sign.sh
+++ b/t/t3435-rebase-gpg-sign.sh
@@ -68,4 +68,10 @@ test_expect_failure 'rebase -p --no-gpg-sign override commit.gpgsign' '
 	test_must_fail git verify-commit HEAD
 '

+test_expect_success 'rebase -r, GPG and merge strategies' '
+	git reset --hard merged &&
+	git rebase -fr --gpg-sign -s resolve --root &&
+	git verify-commit HEAD
+'
+
 test_done

Re: [PATCH] sequencer: fix gpg option passed to octopus merge

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> Puzzling.  Why do you single out octopus merge like this?
>>
>> sequencer.c::do_merge() is called from pick_commits() whenever we
>> see a "merge" insn, and not limited to an octopus merge.
>>
>> Can we have a test to demonstrate the existing failure, so that we
>> can notice if this fix is broken in the future?
>
> Yes, now that I understand that not only octopus merges are affected, I
> would be very much in favor of adding a test case.

Ah, I see why the initial description was focused on octopus.  The
code special cases the two-parent merge using the recursive strategy
and uses completely separate codepath for it, which does not have
this problem but the codepath that handles octopus merges and merges
with the non-default strategy have the problem.

> If I may suggest to add a new test case to t3435 based on t3430's
> '--rebase-merges with strategies' and t3404's 'rebase -i
> --gpg-sign=<key-id>'? Something along these lines:

In addition, a test that passes --no-gpg-sign from the command line, 
because commit.gpgsign is set in the repository used in these tests,
would be necessary---we want to make sure the command line overrides
the configured default (which is the topic of [patch 2/1]).

>
> -- snipsnap --
> diff --git a/sequencer.c b/sequencer.c
> index 00acb124962..88ccff4838d 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3677,7 +3677,7 @@ static int do_merge(struct repository *r,
>  		strvec_push(&cmd.args, "-F");
>  		strvec_push(&cmd.args, git_path_merge_msg(r));
>  		if (opts->gpg_sign)
> -			strvec_push(&cmd.args, opts->gpg_sign);
> +			strvec_pushf(&cmd.args, "-S%s", opts->gpg_sign);
>
>  		/* Add the tips to be merged */
>  		for (j = to_merge; j; j = j->next)
> diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh
> index b47c59c190b..f70b280f5c1 100755
> --- a/t/t3435-rebase-gpg-sign.sh
> +++ b/t/t3435-rebase-gpg-sign.sh
> @@ -68,4 +68,10 @@ test_expect_failure 'rebase -p --no-gpg-sign override commit.gpgsign' '
>  	test_must_fail git verify-commit HEAD
>  '
>
> +test_expect_success 'rebase -r, GPG and merge strategies' '
> +	git reset --hard merged &&
> +	git rebase -fr --gpg-sign -s resolve --root &&
> +	git verify-commit HEAD
> +'
> +
>  test_done