Re: [PATCH v3 1/7] gpg-interface: add new config to select how to sign a commit

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 16, 2018, 20:14 UTC
Message-ID: <xmqqh8kzszth.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <5e88da5eac1e1aea8a946960d455ba321e3e42b1.1531470729.git.henning.schild@siemens.com>

Henning Schild <henning.schild@siemens.com> writes:

Show 45 quoted lines

> Add "gpg.format" where the user can specify which type of signature to
> use for commits. At the moment only "openpgp" is supported and the value is
> not even used. This commit prepares for a new types of signatures.
>
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  Documentation/config.txt | 4 ++++
>  gpg-interface.c          | 7 +++++++
>  2 files changed, 11 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 1cc18a828..ac373e3f4 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1828,6 +1828,10 @@ gpg.program::
>  	signed, and the program is expected to send the result to its
>  	standard output.
>  
> +gpg.format::
> +	Specifies which key format to use when signing with `--gpg-sign`.
> +	Default is "openpgp", that is also the only supported value.
> +
>  gui.commitMsgWidth::
>  	Defines how wide the commit message window is in the
>  	linkgit:git-gui[1]. "75" is the default.
> diff --git a/gpg-interface.c b/gpg-interface.c
> index 09ddfbc26..960c40086 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -7,6 +7,7 @@
>  #include "tempfile.h"
>  
>  static char *configured_signing_key;
> +static const char *gpg_format = "openpgp";
>  static const char *gpg_program = "gpg";
>  
>  #define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
> @@ -138,6 +139,12 @@ int git_gpg_config(const char *var, const char *value, void *cb)
>  		return 0;
>  	}
>  
> +	if (!strcmp(var, "gpg.format")) {
> +		if (value && strcasecmp(value, "openpgp"))
> +			return error("malformed value for %s: %s", var, value);
> +		return git_config_string(&gpg_format, var, value);

I may be mistaken (sorry, I read so many topics X-<) but I think the consensus was to accept only "openpgp" so that we can ensure that

	[gpg "openpgp"] program = foo

etc. can be parsed more naturally without having to manually special case the subsection name to be case insensitive. In other words, strcasecmp() should just be strcmp(). Otherwise, people would get a wrong expectation that

	[gpg] format = OpenPGP
	[gpg "openpgp"] program = foo

should refer to the same and single OpenPGP, but that would violate the usual configuration syntax rules.

The structure of checking the error looks quite suboptimal even when we initially support the single "openpgp" at this step. I would have expected you to be writing the above like so:

	if (!value)
		return config_error_nonbool(var);
	if (strcmp(value, "openpgp"))
		return error("unsupported value for %s: %s", var, value);
	return git_config_string(...);

That would make it more clear that the variable is "nonbool", which does not change across additional support for new formats in later steps.

Show 5 quoted lines

> +	}
> +
>  	if (!strcmp(var, "gpg.program")) {
>  		if (!value)
>  			return config_error_nonbool(var);