Re: [PATCH 7/8] gpg-interface: introduce new signature format "X509" using gpgsm

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 6, 2018, 20:34 UTC
Message-ID: <xmqq7em8je6q.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <1f99ef66bc5e115ec06a3c456658ad54ce405ce3.1530616446.git.henning.schild@siemens.com>

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

Show 10 quoted lines

> -enum gpgformats { PGP_FMT };
> +enum gpgformats { PGP_FMT, X509_FMT };
>  struct gpg_format_data gpg_formats[] = {
>  	{ .format = "PGP", .program = "gpg",
>  	  .extra_args_verify = { "--keyid-format=long", },
>  	  .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },
>  	},
> +	{ .format = "X509", .program = "gpgsm",
> +	  .extra_args_verify = { NULL },
> +	  .sigs = {X509_SIGNATURE, NULL, }

Missing SP between "{X" is a bit irritating.

Also the trailing comma (the issue is shared with the PGP side) when the initializer is smashed on a single line feels pretty much pointless. If it were multi-line, then such a trailing comma would help future developers to add a new entry, i.e.

	 .sigs = { 
	 	PGP_SIGNATURE,
	 	PGP_MESSAGE,
	+	PGP_SOMETHING_NEW,
	 }

without touching the last existing entry.  But on a single line?

	-.sigs = { PGP_SIGNATURE, PGP_MESSAGE }
	+.sigs = { PGP_SIGNATURE, PGP_MESSAGE, PGP_SOMETHING_NEW }

is probably prettier without such a trailing comma.

Show 7 quoted lines

> @@ -190,6 +195,9 @@ int git_gpg_config(const char *var, const char *value, void *cb)
>  	if (!strcmp(var, "gpg.program"))
>  		return git_config_string(&gpg_formats[PGP_FMT].program, var,
>  					 value);
> +	if (!strcmp(var, "gpg.programX509"))
> +		return git_config_string(&gpg_formats[X509_FMT].program, var,
> +					 value);

This is a git_config() callback, isn't it? A two-level variable name is given to a callback after downcasing, so nothing will match "gpg.programX509", I suspect. I see Brian already commented on the name and the better organization being

 - gpg.format defines 'openpgp' or whatever other values;
 - gpg.<format>.program defines the actual program

where <format> is the value gpg.format would take (e.g. "gpg.openpgp.program = gnupg"). And I agree with these suggestions.