Re: [PATCH 7/8] gpg-interface: introduce new signature format "X509" using gpgsm

2018-07-06 Thread Junio C Hamano 
Henning Schild  writes:

> -enum gpgformats { PGP_FMT };
> +enum gpgformats { PGP_FMT, X509_FMT };
>  struct gpg_format_data gpg_formats[] = {
>   { .format = "PGP", .program = "gpg",
> .extra_args_verify = { "--keyid-format=long", },
> .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },
>   },
> + { .format = "X509", .program = "gpgsm",
> +   .extra_args_verify = { NULL },
> +   .sigs = {X509_SIGNATURE, NULL, }

Missing SP between "{X" is a bit irritating.

Also the trailing comma (the issue is shared with the PGP side) when
the initializer is smashed on a single line feels pretty much
pointless.  If it were multi-line, then such a trailing comma would 
help future developers to add a new entry, i.e.

 .sigs = { 
PGP_SIGNATURE,
PGP_MESSAGE,
+   PGP_SOMETHING_NEW,
 }

without touching the last existing entry.  But on a single line?

-.sigs = { PGP_SIGNATURE, PGP_MESSAGE }
+.sigs = { PGP_SIGNATURE, PGP_MESSAGE, PGP_SOMETHING_NEW }

is probably prettier without such a trailing comma.

> @@ -190,6 +195,9 @@ int git_gpg_config(const char *var, const char *value, 
> void *cb)
>   if (!strcmp(var, "gpg.program"))
>   return git_config_string(_formats[PGP_FMT].program, var,
>value);
> + if (!strcmp(var, "gpg.programX509"))
> + return git_config_string(_formats[X509_FMT].program, var,
> +  value);

This is a git_config() callback, isn't it?  A two-level variable
name is given to a callback after downcasing, so nothing will match
"gpg.programX509", I suspect.  I see Brian already commented on the
name and the better organization being

 - gpg.format defines 'openpgp' or whatever other values;
 - gpg..program defines the actual program

where  is the value gpg.format would take
(e.g. "gpg.openpgp.program = gnupg").  And I agree with these
suggestions.

Re: [PATCH 7/8] gpg-interface: introduce new signature format "X509" using gpgsm

2018-07-06 Thread Henning Schild 
Am Fri, 6 Jul 2018 01:10:13 +
schrieb "brian m. carlson" :

> On Tue, Jul 03, 2018 at 02:38:19PM +0200, Henning Schild wrote:
> > This commit allows git to create and check X509 type signatures
> > using gpgsm.
> > 
> > Signed-off-by: Henning Schild 
> > ---
> >  Documentation/config.txt |  5 -
> >  gpg-interface.c  | 10 +-
> >  2 files changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/config.txt b/Documentation/config.txt
> > index c88903399..337df6e48 100644
> > --- a/Documentation/config.txt
> > +++ b/Documentation/config.txt
> > @@ -1828,9 +1828,12 @@ gpg.program::
> > signed, and the program is expected to send the result to
> > its standard output.
> >  
> > +gpg.programX509::  
> 
> I'm not super excited about this name.  It seems to indicate we want a
> level of hierarchy involved.
> 
> A hierarchy like sign.openpgp.program (falling back to gpg.program)
> and sign.x509.program might be more logical.
> 
> > diff --git a/gpg-interface.c b/gpg-interface.c
> > index aa747278e..85d721007 100644
> > --- a/gpg-interface.c
> > +++ b/gpg-interface.c
> > @@ -16,13 +16,18 @@ struct gpg_format_data {
> >  
> >  #define PGP_SIGNATURE "-BEGIN PGP SIGNATURE-"
> >  #define PGP_MESSAGE "-BEGIN PGP MESSAGE-"
> > +#define X509_SIGNATURE "-BEGIN SIGNED MESSAGE-"
> >  
> > -enum gpgformats { PGP_FMT };
> > +enum gpgformats { PGP_FMT, X509_FMT };
> >  struct gpg_format_data gpg_formats[] = {
> > { .format = "PGP", .program = "gpg",
> >   .extra_args_verify = { "--keyid-format=long", },
> >   .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },
> > },
> > +   { .format = "X509", .program = "gpgsm",  
> 
> Similarly to my comment about "PGP", I think this would do well as
> "x509".

Another naming discussion, lets keep discussing and i will implement it
once settled.

Henning

Re: [PATCH 7/8] gpg-interface: introduce new signature format "X509" using gpgsm

2018-07-05 Thread brian m. carlson 
On Tue, Jul 03, 2018 at 02:38:19PM +0200, Henning Schild wrote:
> This commit allows git to create and check X509 type signatures using
> gpgsm.
> 
> Signed-off-by: Henning Schild 
> ---
>  Documentation/config.txt |  5 -
>  gpg-interface.c  | 10 +-
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index c88903399..337df6e48 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1828,9 +1828,12 @@ gpg.program::
>   signed, and the program is expected to send the result to its
>   standard output.
>  
> +gpg.programX509::

I'm not super excited about this name.  It seems to indicate we want a
level of hierarchy involved.

A hierarchy like sign.openpgp.program (falling back to gpg.program) and
sign.x509.program might be more logical.

> diff --git a/gpg-interface.c b/gpg-interface.c
> index aa747278e..85d721007 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -16,13 +16,18 @@ struct gpg_format_data {
>  
>  #define PGP_SIGNATURE "-BEGIN PGP SIGNATURE-"
>  #define PGP_MESSAGE "-BEGIN PGP MESSAGE-"
> +#define X509_SIGNATURE "-BEGIN SIGNED MESSAGE-"
>  
> -enum gpgformats { PGP_FMT };
> +enum gpgformats { PGP_FMT, X509_FMT };
>  struct gpg_format_data gpg_formats[] = {
>   { .format = "PGP", .program = "gpg",
> .extra_args_verify = { "--keyid-format=long", },
> .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },
>   },
> + { .format = "X509", .program = "gpgsm",

Similarly to my comment about "PGP", I think this would do well as
"x509".
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

[PATCH 7/8] gpg-interface: introduce new signature format "X509" using gpgsm

2018-07-03 Thread Henning Schild 
This commit allows git to create and check X509 type signatures using
gpgsm.

Signed-off-by: Henning Schild 
---
 Documentation/config.txt |  5 -
 gpg-interface.c  | 10 +-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index c88903399..337df6e48 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1828,9 +1828,12 @@ gpg.program::
signed, and the program is expected to send the result to its
standard output.
 
+gpg.programX509::
+   Just like gpg.program, here the default you override is "`gpgsm`".
+
 gpg.format::
Specifies which key format to use when signing with `--gpg-sign`.
-   Default is "PGP", that is also the only supported value.
+   Default is "PGP" and another possible value is "X509".
 
 gui.commitMsgWidth::
Defines how wide the commit message window is in the
diff --git a/gpg-interface.c b/gpg-interface.c
index aa747278e..85d721007 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -16,13 +16,18 @@ struct gpg_format_data {
 
 #define PGP_SIGNATURE "-BEGIN PGP SIGNATURE-"
 #define PGP_MESSAGE "-BEGIN PGP MESSAGE-"
+#define X509_SIGNATURE "-BEGIN SIGNED MESSAGE-"
 
-enum gpgformats { PGP_FMT };
+enum gpgformats { PGP_FMT, X509_FMT };
 struct gpg_format_data gpg_formats[] = {
{ .format = "PGP", .program = "gpg",
  .extra_args_verify = { "--keyid-format=long", },
  .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },
},
+   { .format = "X509", .program = "gpgsm",
+ .extra_args_verify = { NULL },
+ .sigs = {X509_SIGNATURE, NULL, }
+   },
 };
 static const char *gpg_format = "PGP";
 
@@ -190,6 +195,9 @@ int git_gpg_config(const char *var, const char *value, void 
*cb)
if (!strcmp(var, "gpg.program"))
return git_config_string(_formats[PGP_FMT].program, var,
 value);
+   if (!strcmp(var, "gpg.programX509"))
+   return git_config_string(_formats[X509_FMT].program, var,
+value);
return 0;
 }
 
-- 
2.16.4