Re: [PATCH v3 1/7] gpg-interface: add new config to select how to sign a commit

From: Jeff King <peff@peff.net>
Date: Jul 16, 2018, 21:38 UTC
Message-ID: <20180716213843.GK25189@sigill.intra.peff.net>
In-Reply-To: <xmqqh8kzszth.fsf@gitster-ct.c.googlers.com>

On Mon, Jul 16, 2018 at 01:14:34PM -0700, Junio C Hamano wrote:

Show 25 quoted lines

> >  #define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
> > @@ -138,6 +139,12 @@ int git_gpg_config(const char *var, const char *value, void *cb)
> >  		return 0;
> >  	}
> >  
> > +	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);
> 
> I may be mistaken (sorry, I read so many topics X-<) but I think the
> consensus was to accept only "openpgp" so that we can ensure that
> 
> 	[gpg "openpgp"] program = foo
> 
> etc. can be parsed more naturally without having to manually special
> case the subsection name to be case insensitive.  In other words,
> strcasecmp() should just be strcmp().  Otherwise, people would get a
> wrong expectation that
> 
> 	[gpg] format = OpenPGP
> 	[gpg "openpgp"] program = foo
> 
> should refer to the same and single OpenPGP, but that would violate
> the usual configuration syntax rules.

I was the one who argued the other way. But unless we are going to move to a two-level config for all of it (i.e., gpg.openPGPProgram), I think what you suggest here is the sanest way forward.

Show 13 quoted lines

> The structure of checking the error looks quite suboptimal even when
> we initially support the single "openpgp" at this step.  I would
> have expected you to be writing the above like so:
> 
> 	if (!value)
> 		return config_error_nonbool(var);
> 	if (strcmp(value, "openpgp"))
> 		return error("unsupported value for %s: %s", var, value);
> 	return git_config_string(...);
> 
> That would make it more clear that the variable is "nonbool", which
> does not change across additional support for new formats in later
> steps.

Agreed.

-Peff