Re: [PATCH v3 3/7] gpg-interface: introduce an abstraction for multiple gpg formats

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

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

Show 28 quoted lines

> Create a struct that holds the format details for the supported formats.
> At the moment that is still just "openpgp". This commit prepares for the
> introduction of more formats, that might use other programs and match
> other signatures.
>
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  gpg-interface.c | 84 ++++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 63 insertions(+), 21 deletions(-)
>
> diff --git a/gpg-interface.c b/gpg-interface.c
> index 960c40086..699651fd9 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -7,11 +7,46 @@
>  #include "tempfile.h"
>  
>  static char *configured_signing_key;
> -static const char *gpg_format = "openpgp";
> -static const char *gpg_program = "gpg";
> +struct gpg_format {
> +	const char *name;
> +	const char *program;
> +	const char **extra_args_verify;
> +	const char **sigs;
> +};
> +
> +static const char *openpgp_verify_args[] = { "--keyid-format=long", NULL };

Let's write it like this, even if the current line is short enough:

static const char *openpgp_verify_args[] = {
	"--keyid-format=long",
	NULL
};

Ditto for the next one. Even if you do not expect these two arrays to get longer, people tend to copy & paste to imitate existing code when making new things, and we definitely would not want them to be doing so on a code like the above (or below). When they need to add new elements to their arrays, having the terminating NULL on its own line means they will have to see less patch noise.

Show 7 quoted lines

> +static const char *openpgp_sigs[] = {
> +	"-----BEGIN PGP SIGNATURE-----",
> +	"-----BEGIN PGP MESSAGE-----", NULL };
> +
> +static struct gpg_format gpg_formats[] = {
> +	{ .name = "openpgp", .program = "gpg",
> +	  .extra_args_verify = openpgp_verify_args,

If the underlying aray is "verify_args" (not "extra"), perhaps the field name should also be .verify_args to match.

Giving an array of things a name "things[]" is a disease, unless the array is very often passed around as a whole to represent the collection of everything. When the array is mostly accessed one element at a time, being able to say thing[3] to mean the third thing is much more pleasant.

So, from that point of view, I pretty much agree with openpgp_verify_args[] and openpgp_sigs[], but am against gpg_formats[]. The last one should be gpg_format[], for which we happen to have only one variant right now.

> +	  .sigs = openpgp_sigs
> +	},
> +};
> +static struct gpg_format *current_format = &gpg_formats[0];

Have a blank line before the above one.

What does "current_format" mean? Is it different from "format to be used"? As we will overwrite the variable upon reading the config, we cannot afford to call it default_format, but somehow "current" feels misleading, at least to me. I expected to find "old format" elsewhere and there may be some format conversion or something from the variable name.

    static struct gpg_format *use_format = &gpg_format[0];

perhaps?

Show 7 quoted lines

> +
> +static struct gpg_format *get_format_by_name(const char *str)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> +		if (!strcasecmp(gpg_formats[i].name, str))

As [1/7], this would become strcmp(), I presume?

Show 16 quoted lines

> +			return gpg_formats + i;
> +	return NULL;
> +}
>  
> -#define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
> -#define PGP_MESSAGE "-----BEGIN PGP MESSAGE-----"
> +static struct gpg_format *get_format_by_sig(const char *sig)
> +{
> +	int i, j;
> +
> +	for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> +		for (j = 0; gpg_formats[i].sigs[j]; j++)
> +			if (starts_with(sig, gpg_formats[i].sigs[j]))
> +				return gpg_formats + i;
> +	return NULL;
> +}

OK.

Show 16 quoted lines

> @@ -140,18 +172,23 @@ int git_gpg_config(const char *var, const char *value, void *cb)
>  	}
>  
>  	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);
> -	}
> -
> -	if (!strcmp(var, "gpg.program")) {
>  		if (!value)
>  			return config_error_nonbool(var);
> -		gpg_program = xstrdup(value);
> +		fmt = get_format_by_name(value);
> +		if (!fmt)
> +			return error("malformed value for %s: %s", var, value);

If I say "opongpg", that probably is not "malformed" (it is all lowercase ascii alphabet that is very plausible-looking string), but simply "bad value".

But other than the minor nit, yes, this structure is what I expected to see from the very first step 1/7.

> +		current_format = fmt;
>  		return 0;
>  	}

>  
> +	if (!strcmp(var, "gpg.program"))
> +		fmtname = "openpgp";

OK, this is a backward compatibility measure to treat gpg.program as if it were gpg.openpgp.program, which makes sense.

Show 7 quoted lines

> +	if (fmtname) {
> +		fmt = get_format_by_name(fmtname);
> +		return git_config_string(&fmt->program, var, value);
> +	}
> +
>  	return 0;
>  }