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

> Henning Schild <henning.schild@siemens.com> writes:
> 
> > Add "gpg.format" where the user can specify which type of signature
> > to use for commits. At the moment only "openpgp" is supported and
> > the value is not even used. This commit prepares for a new types of
> > signatures.
> >
> > Signed-off-by: Henning Schild <henning.schild@siemens.com>
> > ---
> >  Documentation/config.txt | 4 ++++
> >  gpg-interface.c          | 7 +++++++
> >  2 files changed, 11 insertions(+)
> >
> > diff --git a/Documentation/config.txt b/Documentation/config.txt
> > index 1cc18a828..ac373e3f4 100644
> > --- a/Documentation/config.txt
> > +++ b/Documentation/config.txt
> > @@ -1828,6 +1828,10 @@ gpg.program::
> >  	signed, and the program is expected to send the result to
> > its standard output.
> >  
> > +gpg.format::
> > +	Specifies which key format to use when signing with
> > `--gpg-sign`.
> > +	Default is "openpgp", that is also the only supported
> > value. +
> >  gui.commitMsgWidth::
> >  	Defines how wide the commit message window is in the
> >  	linkgit:git-gui[1]. "75" is the default.
> > diff --git a/gpg-interface.c b/gpg-interface.c
> > index 09ddfbc26..960c40086 100644
> > --- a/gpg-interface.c
> > +++ b/gpg-interface.c
> > @@ -7,6 +7,7 @@
> >  #include "tempfile.h"
> >  
> >  static char *configured_signing_key;
> > +static const char *gpg_format = "openpgp";
> >  static const char *gpg_program = "gpg";
> >  
> >  #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.

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

> > +	}
> > +
> >  	if (!strcmp(var, "gpg.program")) {
> >  		if (!value)
> >  			return config_error_nonbool(var);