[v2,1/2] sequencer: fix gpg option passed to merge subcommand

Commit Message

Samuel Čavoj Oct. 12, 2020, 11:49 p.m. UTC

From: Johannes Schindelin <[e-mail]>

When performing a rebase with --rebase-merges using either a custom
strategy specified with -s or an octopus merge, and at the same time
having gpgsign enabled (either rebase -S or config commit.gpgsign), the
operation would fail on making the merge commit. 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.

Fix the issue and add a test case as suggested by Johannes Schindelin.

Signed-off-by: Samuel Čavoj <[e-mail]>
---
changed v1 -> v2:
    added test case
---
 sequencer.c                | 2 +-
 t/t3435-rebase-gpg-sign.sh | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

Comments

Samuel Čavoj Oct. 12, 2020, 11:52 p.m. UTC | #1

On 13.10.2020 01:49, Samuel Čavoj wrote:
> From: Johannes Schindelin <[e-mail]>

Oh boy, I left in a line by accident. Please remove this before
applying, unless a v3 comes around. Sorry about that.

> 
> When performing a rebase with --rebase-merges using either a custom
> strategy specified with -s or an octopus merge, and at the same time
> having gpgsign enabled (either rebase -S or config commit.gpgsign), the
> operation would fail on making the merge commit. 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.
> 
> Fix the issue and add a test case as suggested by Johannes Schindelin.
> 
> Signed-off-by: Samuel Čavoj <[e-mail]>
> ---
> changed v1 -> v2:
>     added test case
> ---
>  sequencer.c                | 2 +-
>  t/t3435-rebase-gpg-sign.sh | 6 ++++++
>  2 files changed, 7 insertions(+), 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)
> diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh
> index b47c59c190..f70b280f5c 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
> -- 
> 2.28.0
>

Phillip Wood Oct. 13, 2020, 9:55 a.m. UTC | #2

Hi Samuel

Thanks for re-rolling this

On 13/10/2020 00:49, Samuel Čavoj wrote:
> From: Johannes Schindelin <[e-mail]>
> 
> When performing a rebase with --rebase-merges using either a custom
> strategy specified with -s or an octopus merge, and at the same time
> having gpgsign enabled (either rebase -S or config commit.gpgsign), the
> operation would fail on making the merge commit.

Small nit-pick: I think it worked fine with if commit.gpgsign was set 
and the user did not pass -S or --no-gpg-sign because merge would sign 
the commits as commit.gpgsign was set, it was only if the user passed a 
gpg signing option to rebase that we had problems. I'm not sure it's 
worth a re-roll just for that though

Best Wishes

Phillip

> 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.
> 
> Fix the issue and add a test case as suggested by Johannes Schindelin.
> 
> Signed-off-by: Samuel Čavoj <[e-mail]>
> ---
> changed v1 -> v2:
>      added test case
> ---
>   sequencer.c                | 2 +-
>   t/t3435-rebase-gpg-sign.sh | 6 ++++++
>   2 files changed, 7 insertions(+), 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)
> diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh
> index b47c59c190..f70b280f5c 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
>

Phillip Wood Oct. 13, 2020, 10:02 a.m. UTC | #3

Hi Samuel

On 13/10/2020 00:49, Samuel Čavoj wrote:
> From: Johannes Schindelin <[e-mail]>
> 
> When performing a rebase with --rebase-merges using either a custom
> strategy specified with -s or an octopus merge, and at the same time
> having gpgsign enabled (either rebase -S or config commit.gpgsign), the
> operation would fail on making the merge commit. 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.
> 
> Fix the issue and add a test case as suggested by Johannes Schindelin.
> 
> Signed-off-by: Samuel Čavoj <[e-mail]>
> ---
> changed v1 -> v2:
>      added test case
> ---
>   sequencer.c                | 2 +-
>   t/t3435-rebase-gpg-sign.sh | 6 ++++++
>   2 files changed, 7 insertions(+), 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)
> diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh
> index b47c59c190..f70b280f5c 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
> +'

Unfortunately I've just noticed that the test above runs

	git config commit.gpgsign true

So this test would pass anyway if merge picks up that config setting. 
The previous test needs to be changed to

	test_config commit.gpgsign true

so that the config setting is cleared when that test finishes. That 
previous test would then be a good template for testing `rebase -r 
--no-gpg-sign` if you wanted to add a test for that to the next patch as 
Junio suggested.

Best Wishes

Phillip

>   test_done
>

Samuel Čavoj Oct. 13, 2020, 10:43 a.m. UTC | #4

Hi Phillip,

On 13.10.2020 10:55, Phillip Wood wrote:
> Hi Samuel
> 
> Thanks for re-rolling this
> 
> On 13/10/2020 00:49, Samuel Čavoj wrote:
> > From: Johannes Schindelin <[e-mail]>
> > 
> > When performing a rebase with --rebase-merges using either a custom
> > strategy specified with -s or an octopus merge, and at the same time
> > having gpgsign enabled (either rebase -S or config commit.gpgsign), the
> > operation would fail on making the merge commit.
> 
> Small nit-pick: I think it worked fine with if commit.gpgsign was set and
> the user did not pass -S or --no-gpg-sign because merge would sign the
> commits as commit.gpgsign was set, it was only if the user passed a gpg
> signing option to rebase that we had problems. I'm not sure it's worth a
> re-roll just for that though

This is not the case. That's how I encountered the problem initially, I
have commit.gpgsign set to true globally. I ran a rebase -ir, over an
octopus merge and then it would fail with an error in the lines of 'not
something we can merge'. I later found out it didn't happen on my
laptop, where gpgsign is not set, so that's what gave it away. In either
case, I did not pass neither -S nor --no-gpg-sign to rebase.

Yes, _if_ the merge command went through, it would have choosen the
correct signing behaviour (i.e. the default), but the merge died,
because an empty string was being passed to it as one of the commits to
merge.

off-topic p.s.:
My mail server does not currently have a proper rDNS record (yeah yeah,
I know) and for this reason, gmx.net drops my emails unconditionally. As
such, I am unable to send emails directly to [e-mail],
without major hackery, anyway. I am dropping him from Cc, as to prevent
sad mailservers. I hope these messages reach him via the mailing list.

Regards,
Samuel

> 
> Best Wishes
> 
> Phillip
> 
> > 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.
> > 
> > Fix the issue and add a test case as suggested by Johannes Schindelin.
> > 
> > Signed-off-by: Samuel Čavoj <[e-mail]>
> > ---
> > changed v1 -> v2:
> >      added test case
> > ---
> >   sequencer.c                | 2 +-
> >   t/t3435-rebase-gpg-sign.sh | 6 ++++++
> >   2 files changed, 7 insertions(+), 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)
> > diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh
> > index b47c59c190..f70b280f5c 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
> > 
>

Samuel Čavoj Oct. 13, 2020, 10:51 a.m. UTC | #5

Hi Phillip,

thank you very much for the feedback.

On 13.10.2020 11:02, Phillip Wood wrote:
> Hi Samuel
> 
> On 13/10/2020 00:49, Samuel Čavoj wrote:
> > From: Johannes Schindelin <[e-mail]>
> > 
> > When performing a rebase with --rebase-merges using either a custom
> > strategy specified with -s or an octopus merge, and at the same time
> > having gpgsign enabled (either rebase -S or config commit.gpgsign), the
> > operation would fail on making the merge commit. 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.
> > 
> > Fix the issue and add a test case as suggested by Johannes Schindelin.
> > 
> > Signed-off-by: Samuel Čavoj <[e-mail]>
> > ---
> > changed v1 -> v2:
> >      added test case
> > ---
> >   sequencer.c                | 2 +-
> >   t/t3435-rebase-gpg-sign.sh | 6 ++++++
> >   2 files changed, 7 insertions(+), 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)
> > diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh
> > index b47c59c190..f70b280f5c 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
> > +'
> 
> Unfortunately I've just noticed that the test above runs
> 
> 	git config commit.gpgsign true
> 
> So this test would pass anyway if merge picks up that config setting. The
> previous test needs to be changed to
> 
> 	test_config commit.gpgsign true

Should I do that now, maybe as a separate part of the patch series? Or
just override the config with a `test_config commit.gpgsign false` in
the test I added?

There is another usage of `git config` in the file, in the
test_rebase_gpg_sign function. 

> 
> so that the config setting is cleared when that test finishes. That previous
> test would then be a good template for testing `rebase -r --no-gpg-sign` if
> you wanted to add a test for that to the next patch as Junio suggested.

Yes, I will definitely do that in v3. Also, the previous test is expect_failure,
that means its a known bug?

Regards,
Samuel

> 
> Best Wishes
> 
> Phillip
> 
> >   test_done
> > 
>

Phillip Wood Oct. 13, 2020, 1:15 p.m. UTC | #6

Hi Samuel

On 13/10/2020 11:43, Samuel Čavoj wrote:
> Hi Phillip,
> 
> On 13.10.2020 10:55, Phillip Wood wrote:
>> Hi Samuel
>>
>> Thanks for re-rolling this
>>
>> On 13/10/2020 00:49, Samuel Čavoj wrote:
>>> From: Johannes Schindelin <[e-mail]>
>>>
>>> When performing a rebase with --rebase-merges using either a custom
>>> strategy specified with -s or an octopus merge, and at the same time
>>> having gpgsign enabled (either rebase -S or config commit.gpgsign), the
>>> operation would fail on making the merge commit.
>>
>> Small nit-pick: I think it worked fine with if commit.gpgsign was set and
>> the user did not pass -S or --no-gpg-sign because merge would sign the
>> commits as commit.gpgsign was set, it was only if the user passed a gpg
>> signing option to rebase that we had problems. I'm not sure it's worth a
>> re-roll just for that though
> 
> This is not the case. That's how I encountered the problem initially, I
> have commit.gpgsign set to true globally. I ran a rebase -ir, over an
> octopus merge and then it would fail with an error in the lines of 'not
> something we can merge'. I later found out it didn't happen on my
> laptop, where gpgsign is not set, so that's what gave it away. In either
> case, I did not pass neither -S nor --no-gpg-sign to rebase.
> 
> Yes, _if_ the merge command went through, it would have choosen the
> correct signing behaviour (i.e. the default), but the merge died,
> because an empty string was being passed to it as one of the commits to
> merge.

Oh sorry for some reason I thought we just ignored opts->gpg_sign before 
your patch I'd forgotten we actually passed it without the '-S' - thanks 
for correcting me.

> off-topic p.s.:
> My mail server does not currently have a proper rDNS record (yeah yeah,
> I know) and for this reason, gmx.net drops my emails unconditionally.

It was dropping my emails sent from a gmail account a few weeks back

> As
> such, I am unable to send emails directly to [e-mail],
> without major hackery, anyway. I am dropping him from Cc, as to prevent
> sad mailservers. I hope these messages reach him via the mailing list.

He's subscribed to the list so hopefully will see them that way

Best Wishes

Phillip

> Regards,
> Samuel
> 
>>
>> Best Wishes
>>
>> Phillip
>>
>>> 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.
>>>
>>> Fix the issue and add a test case as suggested by Johannes Schindelin.
>>>
>>> Signed-off-by: Samuel Čavoj <[e-mail]>
>>> ---
>>> changed v1 -> v2:
>>>       added test case
>>> ---
>>>    sequencer.c                | 2 +-
>>>    t/t3435-rebase-gpg-sign.sh | 6 ++++++
>>>    2 files changed, 7 insertions(+), 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)
>>> diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh
>>> index b47c59c190..f70b280f5c 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
>>>
>>

Phillip Wood Oct. 13, 2020, 1:28 p.m. UTC | #7

Hi Samuel

On 13/10/2020 11:51, Samuel Čavoj wrote:
> Hi Phillip,
> 
> thank you very much for the feedback.
> 
> On 13.10.2020 11:02, Phillip Wood wrote:
>> Hi Samuel
>>
>> On 13/10/2020 00:49, Samuel Čavoj wrote:
>>> From: Johannes Schindelin <[e-mail]>
>>>
>>> When performing a rebase with --rebase-merges using either a custom
>>> strategy specified with -s or an octopus merge, and at the same time
>>> having gpgsign enabled (either rebase -S or config commit.gpgsign), the
>>> operation would fail on making the merge commit. 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.
>>>
>>> Fix the issue and add a test case as suggested by Johannes Schindelin.
>>>
>>> Signed-off-by: Samuel Čavoj <[e-mail]>
>>> ---
>>> changed v1 -> v2:
>>>       added test case
>>> ---
>>>    sequencer.c                | 2 +-
>>>    t/t3435-rebase-gpg-sign.sh | 6 ++++++
>>>    2 files changed, 7 insertions(+), 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)
>>> diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh
>>> index b47c59c190..f70b280f5c 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
>>> +'
>>
>> Unfortunately I've just noticed that the test above runs
>>
>> 	git config commit.gpgsign true
>>
>> So this test would pass anyway if merge picks up that config setting.

That's predicated on my misunderstanding of the behavior without your 
patch but it would be good to fix the test anyway

>> The
>> previous test needs to be changed to
>>
>> 	test_config commit.gpgsign true
> 
> Should I do that now, maybe as a separate part of the patch series? Or
> just override the config with a `test_config commit.gpgsign false` in
> the test I added?

As you've found another instance that needs changing it would probably 
be better as a separate patch

> There is another usage of `git config` in the file, in the
> test_rebase_gpg_sign function.

Well spotted, that should be changed as well

>> so that the config setting is cleared when that test finishes. That previous
>> test would then be a good template for testing `rebase -r --no-gpg-sign` if
>> you wanted to add a test for that to the next patch as Junio suggested.
> 
> Yes, I will definitely do that in v3.

Thanks

> Also, the previous test is expect_failure,
> that means its a known bug?

Yes, `rebase -p` is deprecated and it looks like it was skipped by the 
recent fix for `rebase --no-gpg-sign` in c241371c04 ("rebase.c: honour 
--no-gpg-sign", 2020-04-03)


Best Wishes

Phillip

> Regards,
> Samuel
> 
>>
>> Best Wishes
>>
>> Phillip
>>
>>>    test_done
>>>
>>

Junio C Hamano Oct. 13, 2020, 10:06 p.m. UTC | #8

Phillip Wood <[e-mail]> writes:

>>   +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
>> +'
>
> Unfortunately I've just noticed that the test above runs
>
> 	git config commit.gpgsign true
>
> So this test would pass anyway if merge picks up that config
> setting. The previous test needs to be changed to
>
> 	test_config commit.gpgsign true
>
> so that the config setting is cleared when that test finishes.

Thanks for a review, but I do not think that is a right way to go.

test_config has an inherent assumption that not having the config at
all is somehow the "natural" state, and if that holds true, that
would be OK.  But what is "natural" is subjective X-<.

The way each test is run by calling test_rebase_gpg_sign repeatedly
uses a different and more robust approach to ensure that previous
test does not affect the current one.  Each invocation of test
explicitly sets the configuration to the state the test wants to,
cancelling what the previous test did.

To blend in better with existing tests and match their robustness
expectations, the right fix is for this new test to explicitly use
"git config --set" or "git config --unset" to make the variable into
the desired state, regardless of what the previous tests did.  

If the test quoted at the beginning of this message wants to make
sure that --gpg-sign from the command line takes effect without
commit.gpgsign set, it should unset the variable explicitly.
Another combination worth testing is to ensure that --gpg-sign takes
effect when commit.gpgsign is explicitly set to false (not "left
unset").

Thanks.

Samuel Čavoj Oct. 13, 2020, 11:45 p.m. UTC | #9

Hi,

On 13.10.2020 15:06, Junio C Hamano wrote:
> Phillip Wood <[e-mail]> writes:
> 
> >>   +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
> >> +'
> >
> > Unfortunately I've just noticed that the test above runs
> >
> > 	git config commit.gpgsign true
> >
> > So this test would pass anyway if merge picks up that config
> > setting. The previous test needs to be changed to
> >
> > 	test_config commit.gpgsign true
> >
> > so that the config setting is cleared when that test finishes.
> 
> Thanks for a review, but I do not think that is a right way to go.
> 
> test_config has an inherent assumption that not having the config at
> all is somehow the "natural" state, and if that holds true, that
> would be OK.  But what is "natural" is subjective X-<.
> 
> The way each test is run by calling test_rebase_gpg_sign repeatedly
> uses a different and more robust approach to ensure that previous
> test does not affect the current one.  Each invocation of test
> explicitly sets the configuration to the state the test wants to,
> cancelling what the previous test did.
> 
> To blend in better with existing tests and match their robustness
> expectations, the right fix is for this new test to explicitly use
> "git config --set" or "git config --unset" to make the variable into
> the desired state, regardless of what the previous tests did.  

This is more or less what I did in v3, although not completely
comprehensively. Of course, the test_rebase_gpg_sign function could be
either duplicated or modified to be more generic in order to accommodate
all combinations for the new tests, though I considered this not worth
doing. I am open to it, however.

I'm unfamiliar with the git codebase and the testing practices, so a
little hand-holding might be needed, I hope that's not a bother for you.

In addition, I have replaced `git config` usages with `test_config`, not
sure if that should be reverted -- I see no harm in using it, and I
guess it is slightly more explicit.

Speaking of v3, I pulled the trigger a little too fast on that one,
should have waited a little longer for feedback. Sorry about that.

Though, I am not sure what has happened to it. I sent it out in the
usual manner, but it hasn't appeared in any of the mailing list
archives. Can you please confirm (or deny) that you have received it?
In case not, should I just send it again, with the same Message-Id's?
All the recipient mail servers responded with a 250 Ok from my postfix
logs.

Thank you for all the help.

Regards,
Samuel

> 
> If the test quoted at the beginning of this message wants to make
> sure that --gpg-sign from the command line takes effect without
> commit.gpgsign set, it should unset the variable explicitly.
> Another combination worth testing is to ensure that --gpg-sign takes
> effect when commit.gpgsign is explicitly set to false (not "left
> unset").
> 
> Thanks.
>

Junio C Hamano Oct. 14, 2020, 3:31 p.m. UTC | #10

Samuel Čavoj <[e-mail]> writes:

> Speaking of v3, I pulled the trigger a little too fast on that one,
> should have waited a little longer for feedback. Sorry about that.

Nothing to be sorry about---working asynchronously, mails crossing
and timings not coinciding are just expected parts of life.  It is
the kind of thing participants need to learn to adjust, as "normal
latency" is different from community to community.

> Though, I am not sure what has happened to it. I sent it out in the
> usual manner, but it hasn't appeared in any of the mailing list
> archives. Can you please confirm (or deny) that you have received it?
> In case not, should I just send it again, with the same Message-Id's?
> All the recipient mail servers responded with a 250 Ok from my postfix
> logs.

I see the message I am responding to has a timestamp that is about
16 hours ago (relative to the time I am typing this response), but I
am fairly sure that I didn't see it ten hours ago (i.e. my last
night's final e-mail scanning).

Perhaps there was congestion somewhere that held some messages.

Phillip Wood Oct. 16, 2020, 1:40 p.m. UTC | #11

Hi Junio

It seems to be that the config handling here is another example an 
interdependence between tests that makes life harder for contributors 
and reviewers.

On 13/10/2020 23:06, Junio C Hamano wrote:
> Phillip Wood <[e-mail]> writes:
> 
>>>    +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
>>> +'
>>
>> Unfortunately I've just noticed that the test above runs
>>
>> 	git config commit.gpgsign true
>>
>> So this test would pass anyway if merge picks up that config
>> setting. The previous test needs to be changed to
>>
>> 	test_config commit.gpgsign true
>>
>> so that the config setting is cleared when that test finishes.
> 
> Thanks for a review, but I do not think that is a right way to go.
> 
> test_config has an inherent assumption that not having the config at
> all is somehow the "natural" state, and if that holds true, that
> would be OK.  But what is "natural" is subjective X-<.

What is "natural" is subjective but what is the default config is not. 
If test scripts set random config variables and assumes that they will 
be cleared in later tests if they don't want them it makes it very hard 
for contributors and reviewers to check that new tests are sound as they 
have to analyze each existing test in the script. In this example I 
believe the new test was contributed by dscho who is an experienced 
contributor with an interest in the test suite. However the test did not 
clear the relevant config variable - if an experienced contributor did 
not realize that the variable needed to be cleared how are new 
contributors supposed to figure it out? If each test starts with the 
default config it is much easier to reason about it.

> The way each test is run by calling test_rebase_gpg_sign repeatedly
> uses a different and more robust approach to ensure that previous
> test does not affect the current one.  Each invocation of test
> explicitly sets the configuration to the state the test wants to,
> cancelling what the previous test did.

It is only robust if contributors and reviewers realize that is what is 
expected. Reviewers that only read the patch without loading up the test 
file in their editor have no indication that the test should be clearing 
the config variable.

Best Wishes

Phillip

> To blend in better with existing tests and match their robustness
> expectations, the right fix is for this new test to explicitly use
> "git config --set" or "git config --unset" to make the variable into
> the desired state, regardless of what the previous tests did.
> 
> If the test quoted at the beginning of this message wants to make
> sure that --gpg-sign from the command line takes effect without
> commit.gpgsign set, it should unset the variable explicitly.
> Another combination worth testing is to ensure that --gpg-sign takes
> effect when commit.gpgsign is explicitly set to false (not "left
> unset").
> 
> Thanks.
> 
> 
>

Junio C Hamano Oct. 16, 2020, 4:37 p.m. UTC | #12

Phillip Wood <[e-mail]> writes:

> ... Reviewers that only read the patch without loading up the
> test file in their editor have no indication that the test should be
> clearing the config variable.

It is not a review if the code being updated is not checked for
sanity in its own context, is it?

Phillip Wood Oct. 16, 2020, 5:25 p.m. UTC | #13

On 16/10/2020 17:37, Junio C Hamano wrote:
> Phillip Wood <[e-mail]> writes:
> 
>> ... Reviewers that only read the patch without loading up the
>> test file in their editor have no indication that the test should be
>> clearing the config variable.
> 
> It is not a review if the code being updated is not checked for
> sanity in its own context, is it?

It is disappointing that you have chosen to cut the original to those 
few lines and reply just to them. The original message made the broader 
point that having to check the previous tests in a file to figure out 
what the state is at the beginning of any new test that gets added makes 
life harder than it needs to be both for contributors and reviewers.

Phillip

Junio C Hamano Oct. 16, 2020, 8:15 p.m. UTC | #14

Phillip Wood <[e-mail]> writes:

> What is "natural" is subjective but what is the default config is
> not. If test scripts set random config variables and assumes that they
> will be ...
> ... cleared how are new contributors supposed to figure it out? If each
> test starts with the default config it is much easier to reason about
> it.

If this were a test script with many tests, all of which depend on
starting from clean slate wrt the configuration variable space, and
if you are adding just one or two tests that wants to run with
configuration variable in effect, the story would be quite
different.  For example, I would think it would make a lot of sense
in <[e-mail]> to use
test_config instead of "git config" as it is clear that leading 100+
lines of tests run with the default configuration, and it is sane to
expact that later tests that may be added in the future would be the
same.

Look at the test script in question again and notice that it is
about seeing behaviour with the single configuration variable set to
true or false.  Using test_config would still signal the test pieces
that use it do depend on the shown setting of the variable, but it
does not help at all the test pieces that wants to see what happens
when there is no configuration.  Explicitly using "git config" and
"test_unconfig" actually would _help_ reviewers who only looks
between the pre- and post- context lines that are affected by the
patch, as they do not have to know or assume that "normal state is
nothing configured" (which needs to be verified by seeing all the
existing tests that are not shown in the patch use test_config to
clear the setting when they are done).

All of the above would be obvious once you think about it, I'd
think.

Thanks.

Patch

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)
diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh
index b47c59c190..f70b280f5c 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