git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Phillip Wood <phillip.wood123@gmail.com>
To: "Samuel Čavoj" <samuel@cavoj.net>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	"brian m. carlson" <sandals@crustytoothpaste.net>
Subject: Re: [PATCH v2 1/2] sequencer: fix gpg option passed to merge subcommand
Date: Tue, 13 Oct 2020 14:28:20 +0100	[thread overview]
Message-ID: <1bc5b455-68f7-eb7a-763d-d541086ddcdb@gmail.com> (raw)
In-Reply-To: <20201013105159.rfpczn563agvih5y@fastboi.localdomain>

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

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

  reply	other threads:[~2020-10-13 13:28 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-12 23:49 [PATCH v2 1/2] sequencer: fix gpg option passed to merge subcommand Samuel Čavoj
2020-10-12 23:49 ` [PATCH v2 2/2] sequencer: pass explicit --no-gpg-sign to merge Samuel Čavoj
2020-10-13  4:46   ` Junio C Hamano
2020-10-13 10:03   ` Phillip Wood
2020-10-12 23:52 ` [PATCH v2 1/2] sequencer: fix gpg option passed to merge subcommand Samuel Čavoj
2020-10-13  9:55 ` Phillip Wood
2020-10-13 10:43   ` Samuel Čavoj
2020-10-13 13:15     ` Phillip Wood
2020-10-13 10:02 ` Phillip Wood
2020-10-13 10:51   ` Samuel Čavoj
2020-10-13 13:28     ` Phillip Wood [this message]
2020-10-13 22:06   ` Junio C Hamano
2020-10-13 23:45     ` Samuel Čavoj
2020-10-14 15:31       ` Junio C Hamano
2020-10-16 13:40     ` Phillip Wood
2020-10-16 16:37       ` Junio C Hamano
2020-10-16 17:25         ` Phillip Wood
2020-10-16 20:15       ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1bc5b455-68f7-eb7a-763d-d541086ddcdb@gmail.com \
    --to=phillip.wood123@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=phillip.wood@dunelm.org.uk \
    --cc=samuel@cavoj.net \
    --cc=sandals@crustytoothpaste.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).