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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Henning Schild <henning.schild@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> ---
>  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);



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux