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

From: Samuel Čavoj <samuel@cavoj.net>
Date: Oct 13, 2020, 10:51 UTC
Message-ID: <20201013105159.rfpczn563agvih5y@fastboi.localdomain>
In-Reply-To: <d2491759-e761-647b-7e76-3515b8eeaf3f@gmail.com>

Hi Phillip,

thank you very much for the feedback.

On 13.10.2020 11:02, Phillip Wood wrote:

Show 56 quoted lines

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

Show 8 quoted lines

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