On Mon, Jul 16, 2018 at 01:14:34PM -0700, Junio C Hamano wrote:
> > #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.
> 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
