Re: [PATCH v3 1/7] gpg-interface: add new config to select how to sign a commit
Am Mon, 16 Jul 2018 13:14:34 -0700
schrieb Junio C Hamano :
> Henning Schild 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
> > ---
> > 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(_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.
Ok, also having read the other mails i think we are still at
gpg.format and gpg..program, so i switched the two patches from
strcasecmp back to strcmp.
> 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.
Right, at one point (not mailed) i had that but expected it would not
pass the review, since git_config_string contains that check as well.
Changed.
Henning
> > + }
> > +
> > if (!strcmp(var, "gpg.program")) {
> > if (!value)
> > return config_error_nonbool(var);
Re: [PATCH v3 1/7] gpg-interface: add new config to select how to sign a commit
On Tue, Jul 17, 2018 at 12:03:11AM +, brian m. carlson wrote: > > +gpg.format:: > > + Specifies which key format to use when signing with `--gpg-sign`. > > + Default is "openpgp", that is also the only supported value. > > I think, as discussed in the other thread, perhaps a different prefix > for these options is in order if we'd like to plan for the future. > Maybe this could be "signature.format", "sign.format", "signing.format", > or "signingtool.format" (I tend to prefer the former, but not too > strongly). > > I anticipate that some projects will prefer other formats, and it makes > it easier if we don't have to maintain two sets of legacy names. Heh. This is slowly morphing into the original signingtool series. For the record (since I think my response is what you meant by the "other thread"), I'm OK with going down this gpg.* road for now, and dealing with other tools if and when they appear (via the extra level of indirection). -Peff
Re: [PATCH v3 1/7] gpg-interface: add new config to select how to sign a commit
On Fri, Jul 13, 2018 at 10:41:23AM +0200, Henning Schild wrote: > 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 > --- > 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. I think, as discussed in the other thread, perhaps a different prefix for these options is in order if we'd like to plan for the future. Maybe this could be "signature.format", "sign.format", "signing.format", or "signingtool.format" (I tend to prefer the former, but not too strongly). I anticipate that some projects will prefer other formats, and it makes it easier if we don't have to maintain two sets of legacy names. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: [PATCH v3 1/7] gpg-interface: add new config to select how to sign a commit
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(_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
Re: [PATCH v3 1/7] gpg-interface: add new config to select how to sign a commit
Henning Schild 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
> ---
> 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(_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);
[PATCH v3 1/7] gpg-interface: add new config to select how to sign a commit
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
---
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(_format, var, value);
+ }
+
if (!strcmp(var, "gpg.program")) {
if (!value)
return config_error_nonbool(var);
--
2.16.4
