Re: [PATCH v3 3/3] sequencer: pass explicit --no-gpg-sign to merge

[Junio C Hamano <gitster@pobox.com>]

Samuel Čavoj <samuel@cavoj.net> writes:

> The merge subcommand launched for merges with non-default strategy would
> use its own default behaviour to decide how to sign commits, regardless
> of what opts->gpg_sign was set to. For example the --no-gpg-sign flag
> given to rebase explicitly would get ignored, if commit.gpgsign was set
> to true.
>
> Fix the issue and add a test case excercising this behaviour.
>
> Signed-off-by: Samuel Čavoj <samuel@cavoj.net>
> ---
> v2 -> v3:
>     - added test case
> ---
>  sequencer.c                | 2 ++
>  t/t3435-rebase-gpg-sign.sh | 7 +++++++
>  2 files changed, 9 insertions(+)
>
> diff --git a/sequencer.c b/sequencer.c
> index 88ccff4838..043d606829 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3678,6 +3678,8 @@ static int do_merge(struct repository *r,
>  		strvec_push(&cmd.args, git_path_merge_msg(r));
>  		if (opts->gpg_sign)
>  			strvec_pushf(&cmd.args, "-S%s", opts->gpg_sign);
> +		else
> +			strvec_push(&cmd.args, "--no-gpg-sign");

Makes sense, I guess.  As long as opts->gpg_sign reflects not just
the command line but also the configuration.  Otherwise, an
invocation of "git rebase" with no gpg-sign related command line
options would say "ah, opts->gpg_sign is false, we must have been
told from the command line not to sign, so pass --no-gpg-sign here"
and that is not correct.

> diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh
> index 9d2faffa03..773c2a1d72 100755
> --- a/t/t3435-rebase-gpg-sign.sh
> +++ b/t/t3435-rebase-gpg-sign.sh
> @@ -81,4 +81,11 @@ test_expect_success 'rebase -r, GPG config and merge strategies' '
>  	git verify-commit HEAD
>  '
>  
> +test_expect_success 'rebase -r, --no-gpg-sign and merge strategies' '
> +	git reset --hard merged &&
> +	test_config commit.gpgsign true &&
> +	git rebase -fr --no-gpg-sign -s resolve --root &&
> +	test_must_fail git verify-commit HEAD
> +'

I think that before this patch, we've tested the "no command line
option, but configuration tells us to sign" combination already to
make sure the result is signed, so this new test is sufficient.

I briefly wondered if "test_must_fail git verify-commit" sufficient
to make sure that the rebased commits are not signed (i.e. verify
may fail for reasons other than the commit lacks signature, like the
commit is signed but with a wrong key, etc.), but I think it is OK
at least for now.  Others might have clever ideas to cleanly and
cheaply reject other kinds of failures, in which case we may want to
adopt such a solution.

Now that we know that the root cause of the bug you fixed was
because rebase rebase with the default merge strategy for two-head
merges use separate codepaths from and all other rebases, I wonder
if it is prudent to also test the same cases this series adds
without giving "-s resolve".  That would exercise the other codepath
that handles the default merge strategy for two-head merges.  Yes,
we know that other codepath has been working even before this fix,
but tests are not about showing off what we fixed, but are about
making sure similar breakage won't be introduced by mistake in the
future.

Thanks.