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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Oct 13, 2020, 13:28 UTC
Message-ID: <1bc5b455-68f7-eb7a-763d-d541086ddcdb@gmail.com>
In-Reply-To: <20201013105159.rfpczn563agvih5y@fastboi.localdomain>

Hi Samuel

On 13/10/2020 11:51, Samuel Čavoj wrote:

Show 58 quoted lines

> 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 <Johannes.Schindelin@gmx.de>
>>>
>>> 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 <samuel@cavoj.net>
>>> ---
>>> 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

Show 8 quoted lines

>> 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

Show 5 quoted lines

>> 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

Show 11 quoted lines

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