Re: [PATCH 4/5] merge: verify signatures if gpg.verifySignatures is true

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 6, 2020, 21:01 UTC
Message-ID: <xmqqa7708flc.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20200105135616.19102-5-hji@dyntopia.com>

Hans Jerry Illikainen <hji@dyntopia.com> writes:

Show 6 quoted lines

> @@ -610,6 +610,8 @@ static int git_merge_config(const char *k, const char *v, void *cb)
>  		show_diffstat = git_config_bool(k, v);
>  	else if (!strcmp(k, "merge.verifysignatures"))
>  		verify_signatures = git_config_bool(k, v);
> +	else if (!strcmp(k, "gpg.verifysignatures") && verify_signatures < 0)
> +		verify_signatures = git_config_bool(k, v);

Hmm, the next person who attempts to generalize the mechanism further would have a hard time introducing a fallback configuration that is even common than "gpg" when s/he has to start with this code, no? That is, this patch introduced "gpg.verifysignatures is used when merge.verifysignatures is not defined" and with the two level override the code works OK, but to implement "if neither gpg.* or merge.* is defined, common.verifysignatures is used instead", the above part needs to be dismantled and redone.

Keeping the "initialize verify_signatures to -1 (unspecified)" from this patch, setting a separate gpg_verify_signatures variable upon seeing gpg.verifysignatures, and consolidating the final finding after git_config(git_merge_config, NULL) returns into verify_signatures like so:

	init_diff_ui_defaults();
	git_config(git_merge_config, NULL);

+ if (verify_signatures < 0) + verify_signatures = (0 <= gpg_verify_signatures) + ? gpg_verify_signatures + : 0;

would be more in line with the way we arrange multiple configuration variables to serve as fallback defaults. And that is more easily extensible.

Also with such an arrangement, "if (verify_signatures == 1)" we see below will become unnecessary, which is another plus.

Thanks.

Show 21 quoted lines

>  	else if (!strcmp(k, "pull.twohead"))
>  		return git_config_string(&pull_twohead, k, v);
>  	else if (!strcmp(k, "pull.octopus"))
> @@ -1399,7 +1401,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  		if (remoteheads->next)
>  			die(_("Can merge only exactly one commit into empty head"));
>  
> -		if (verify_signatures &&
> +		if (verify_signatures == 1 &&
>  		    gpg_verify_commit(&remoteheads->item->object.oid, NULL,
>  				      NULL, gpg_flags))
>  			die(_("Signature verification failed"));
> @@ -1423,7 +1425,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  		usage_with_options(builtin_merge_usage,
>  			builtin_merge_options);
>  
> -	if (verify_signatures) {
> +	if (verify_signatures == 1) {
>  		for (p = remoteheads; p; p = p->next) {
>  			if (gpg_verify_commit(&p->item->object.oid, NULL, NULL,
>  					      gpg_flags))