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

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

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

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

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