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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 15, 2020, 17:02 UTC
Message-ID: <xmqqd01jh17n.fsf@gitster.c.googlers.com>
In-Reply-To: <20201013213021.3671432-3-samuel@cavoj.net>

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

Show 27 quoted lines

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

Show 14 quoted lines

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