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

["Samuel Čavoj" <samuel@cavoj.net>]

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 <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.
> 
> 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 Johannes.Schindelin@gmx.de,
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 <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
> > +'
> > +
> >   test_done
> > 
>