Re: [PATCH v2 8/9] gpg-interface: introduce new signature format "x509" using gpgsm

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 10, 2018, 17:40 UTC
Message-ID: <xmqqbmbf7zw9.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180710170109.GG23624@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 23 quoted lines

>> @@ -16,13 +16,18 @@ struct gpg_format_data {
>>  
>>  #define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
>>  #define PGP_MESSAGE "-----BEGIN PGP MESSAGE-----"
>> +#define X509_SIGNATURE "-----BEGIN SIGNED MESSAGE-----"
>>  
>> -enum gpgformats { PGP_FMT };
>> +enum gpgformats { PGP_FMT, X509_FMT };
>>  struct gpg_format_data gpg_formats[] = {
>>  	{ .format = "openpgp", .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 }
>> +	},
>
> Extremely minor nit, but if there are no other uses of PGP_SIGNATURE etc
> outside of this array (as I hope there wouldn't be after this series),
> would it make more sense to just include the literals inline in the
> array definition? That's one less layer of indirection when somebody is
> reading the code.

It is good design-sense to shoot for fewer levels of indirection, but I suspect that "'const char **' instead of maximally-sized fixed array of strings" would require a named array and constants like this:

	static const char *gpg_verify_args[] = {
		"--verify",
		"--status-fd=1",
		"--keyid-format=long",
		NULL
	};
	static const char *gpg_sigs[] = {
		"-----BEGIN PGP SIGNATURE-----",
		"-----BEGIN PGP MESSAGE-----",
		NULL
	};

	struct gpg_format {
		const char *name;
		const char *program;
		const char * const *verify_args;
		const char * const *sigs;
	} gpg_format[] = {
		{
			.name = "openpgp",
			.program = "gpg',
			.verify_args = gpg_verify_args,
			.sigs = gpg_sigs,
		},
		{
			...
		},
	};

so we may end up having the same number of levels of indirection anyway in the long-term final form.

As readers may be able to read from the above, I also have a suspicion that it is a mistake to pretend that "--verify" etc., which merely happen to be common across the variants the series covers, will stay forever to be common across _all_ variants and that is why the field no longer is called "extra" args but is meant to contain the full args.