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

> Henning Schild <henning.schild@siemens.com> writes:
> 
> > 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.

> > +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?

> > +
> > +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?
> 
> > +			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.
> 
> > @@ -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.
> 
> > +	if (fmtname) {
> > +		fmt = get_format_by_name(fmtname);
> > +		return git_config_string(&fmt->program, var,
> > value);
> > +	}
> > +
> >  	return 0;
> >  }