patch, 8 partsgpg-interface: make parse_gpg_output static and remove from interface header

> The combination of verify_signed_buffer followed by parse_gpg_output is
> available as check_signature. Use that instead of implementing it again.
>
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---

>  builtin/receive-pack.c | 17 ++---------------
>  1 file changed, 2 insertions(+), 15 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 68d36e0a5..9f0583deb 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -629,8 +629,6 @@ static void prepare_push_cert_sha1(struct child_process *proc)
>  		return;
>  
>  	if (!already_done) {
> -		struct strbuf gpg_output = STRBUF_INIT;
> -		struct strbuf gpg_status = STRBUF_INIT;
>  		int bogs /* beginning_of_gpg_sig */;
>  
>  		already_done = 1;
> @@ -639,22 +637,11 @@ static void prepare_push_cert_sha1(struct child_process *proc)
>  			oidclr(&push_cert_oid);
>  
>  		memset(&sigcheck, '\0', sizeof(sigcheck));
> -		sigcheck.result = 'N';
>  
>  		bogs = parse_signature(push_cert.buf, push_cert.len);
> -		if (verify_signed_buffer(push_cert.buf, bogs,
> -					 push_cert.buf + bogs, push_cert.len - bogs,
> -					 &gpg_output, &gpg_status) < 0) {
> -			; /* error running gpg */
> -		} else {
> -			sigcheck.payload = push_cert.buf;
> -			sigcheck.gpg_output = gpg_output.buf;
> -			sigcheck.gpg_status = gpg_status.buf;
> -			parse_gpg_output(&sigcheck);
> -		}
> +		check_signature(push_cert.buf, bogs, push_cert.buf + bogs,
> +				push_cert.len - bogs, &sigcheck);
>  
> -		strbuf_release(&gpg_output);
> -		strbuf_release(&gpg_status);
>  		nonce_status = check_nonce(push_cert.buf, bogs);
>  	}
>  	if (!is_null_oid(&push_cert_oid)) {

> Henning Schild <henning.schild@siemens.com> writes:
>
>> The combination of verify_signed_buffer followed by parse_gpg_output is
>> available as check_signature. Use that instead of implementing it again.
>>
>> Signed-off-by: Henning Schild <henning.schild@siemens.com>
>> ---
>
> Makes sense.  
>
> When d05b9618 ("receive-pack: GPG-validate push certificates",
> 2014-08-14) implemented the check, there wasn't check_signature()
> available.  The commit probably should have done what a4cc18f2
> ("verify-tag: share code with verify-commit", 2015-06-21) later did
> to introduce the check_signature() function by factoring it out of
> commit.c::check_commit_signature() as a preparatory step.
>
> Will queue.  Thanks.

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Henning Schild <henning.schild@siemens.com> writes:
> >  
> >> The combination of verify_signed_buffer followed by
> >> parse_gpg_output is available as check_signature. Use that instead
> >> of implementing it again.
> >>
> >> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> >> ---  
> >
> > Makes sense.  
> >
> > When d05b9618 ("receive-pack: GPG-validate push certificates",
> > 2014-08-14) implemented the check, there wasn't check_signature()
> > available.  The commit probably should have done what a4cc18f2
> > ("verify-tag: share code with verify-commit", 2015-06-21) later did
> > to introduce the check_signature() function by factoring it out of
> > commit.c::check_commit_signature() as a preparatory step.
> >
> > Will queue.  Thanks.  
> 
> Well, I guess I won't queue this version that would waste others'
> time, as you'd be rerolling to update variable names and such, so
> I'd wait for that (and you in turn would wait for the names and
> other discussions to settle).
> 
> Thanks anyway.

> Valid values are already covered by all tests that use GPG, now also
> test what happens if we go for an invalid one.
>
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  t/t7510-signed-commit.sh | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
> index 6e2015ed9..cb523513f 100755
> --- a/t/t7510-signed-commit.sh
> +++ b/t/t7510-signed-commit.sh
> @@ -227,4 +227,14 @@ test_expect_success GPG 'log.showsignature behaves like --show-signature' '
>  	grep "gpg: Good signature" actual
>  '
>  
> +test_expect_success GPG 'check gpg config for malformed values' '
> +	mv .git/config .git/config.old &&
> +	test_when_finished "mv .git/config.old .git/config" &&

> +	test_i18ngrep "malformed value for gpg.format: malformed" result &&
> +	test_i18ngrep "fatal: .*\.git/config" result &&
> +	test_i18ngrep "fatal: .*line 2" result
> +'
> +
>  test_done

> Henning Schild <henning.schild@siemens.com> writes:
> 
> > Valid values are already covered by all tests that use GPG, now also
> > test what happens if we go for an invalid one.
> >
> > Signed-off-by: Henning Schild <henning.schild@siemens.com>
> > ---
> >  t/t7510-signed-commit.sh | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
> > index 6e2015ed9..cb523513f 100755
> > --- a/t/t7510-signed-commit.sh
> > +++ b/t/t7510-signed-commit.sh
> > @@ -227,4 +227,14 @@ test_expect_success GPG 'log.showsignature
> > behaves like --show-signature' ' grep "gpg: Good signature" actual
> >  '
> >  
> > +test_expect_success GPG 'check gpg config for malformed values' '
> > +	mv .git/config .git/config.old &&
> > +	test_when_finished "mv .git/config.old .git/config" &&  
> 
> Hmmmmm.  
> 
> Is the damage caused by throwing a bad value at gpg.format designed
> to be so severe that "test_when_finished test_unconfig ..." cannot
> recover from?  This test script is not about how "git config" is
> implemented and works, so it would be a good idea for it to be even
> oblivious to the fact that .git/config is the file being mucked with
> when we do "git config".
> 
> I have a suspicion that you can just use test_config (which would
> arrange "test_when_finished test_unconfig ..." for free).
> 
> > +	git config gpg.format malformed &&
> > +	test_expect_code 128 git commit -S --amend -m "fail"
> > 2>result &&  
> 
> Is this 128 something we document and have users rely on?  Or should
> we rather say
> 
> 	test_must_fail git commit ...
> 
> here instead?

> > +	test_i18ngrep "malformed value for gpg.format: malformed"
> > result &&
> > +	test_i18ngrep "fatal: .*\.git/config" result &&
> > +	test_i18ngrep "fatal: .*line 2" result
> > +'
> > +
> >  test_done  

> gnupg does print the keyid followed by a space and the signer comes
> next. The same pattern is also used in gpgsm, but there the key length
> would be 40 instead of 16. Instead of hardcoding the expected length,
> find the first space and calculate it.
>
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  gpg-interface.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>

> diff --git a/gpg-interface.c b/gpg-interface.c
> index cd3b1b568..aa747278e 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -88,10 +88,11 @@ static void parse_gpg_output(struct signature_check *sigc)
>  		sigc->result = sigcheck_gpg_status[i].result;
>  		/* The trust messages are not followed by key/signer information */
>  		if (sigc->result != 'U') {
> -			sigc->key = xmemdupz(found, 16);
> +			next = strchrnul(found, ' ');
> +			sigc->key = xmemdupz(found, next - found);
>  			/* The ERRSIG message is not followed by signer information */
>  			if (sigc-> result != 'E') {
> -				found += 17;
> +				found = next + 1;
>  				next = strchrnul(found, '\n');
>  				sigc->signer = xmemdupz(found, next - found);
>  			}

> On Tue, Jul 03, 2018 at 02:38:15PM +0200, Henning Schild wrote:
> > Add "gpg.format" where the user can specify which type of signature
> > to use for commits. At the moment only "PGP" is supported and the
> > value is not even used. This commit prepares for a new types of
> > signatures.  
> 
> We typically prefer to have option values specified in lower case.  I
> also think "openpgp" might be better than "PGP", since that's the name
> of the specification and it would avoid any potential unhappiness
> about compatibility with PGP or trademarks.

> On Tue, Jul 03, 2018 at 02:38:15PM +0200, Henning Schild wrote:
>> Add "gpg.format" where the user can specify which type of signature to
>> use for commits. At the moment only "PGP" is supported and the value is
>> not even used. This commit prepares for a new types of signatures.
>
> We typically prefer to have option values specified in lower case.  I
> also think "openpgp" might be better than "PGP", since that's the name
> of the specification and it would avoid any potential unhappiness about
> compatibility with PGP or trademarks.

> This commit allows git to create and check X509 type signatures using
> gpgsm.
> 
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  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::

> 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",

> 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 <henning.schild@siemens.com>
> > ---
> >  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".

> -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, }

> @@ -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(&gpg_formats[PGP_FMT].program, var,
>  					 value);
> +	if (!strcmp(var, "gpg.programX509"))
> +		return git_config_string(&gpg_formats[X509_FMT].program, var,
> +					 value);

> Create a struct that holds the format details for the supported formats.
> At the moment that is still just "PGP". This commit prepares for the
> introduction of more formats, that might use other programs and match
> other signatures.
>
> Signed-off-by: Henning Schild <henning.schild@siemens.com>

>  static char *configured_signing_key;
> -static const char *gpg_format = "PGP";
> -static const char *gpg_program = "gpg";
> +struct gpg_format_data {
> +       const char *format;
> +       const char *program;
> +       const char *extra_args_verify[1];
> +       const char *sigs[2];
> +};
>
>  #define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
>  #define PGP_MESSAGE "-----BEGIN PGP MESSAGE-----"
>
> +enum gpgformats { PGP_FMT };
> +struct gpg_format_data gpg_formats[] = {
> +       { .format = "PGP", .program = "gpg",
> +         .extra_args_verify = { "--keyid-format=long", },
> +         .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },
> +       },
> +};

> +static const char *gpg_format = "PGP";
> +
> +static struct gpg_format_data *get_format_data(void)
> +{
> +       int i;
> +       for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> +               if (!strcmp(gpg_formats[i].format, gpg_format))
> +                       return gpg_formats + i;
> +       assert(0);

> +               j = 0;
> +               for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> +                       if (!strcmp(value, gpg_formats[i].format)) {
> +                               j++;
> +                               break;
> +                       }
> +               if (!j)
>                         return error("malformed value for %s: %s", var, value);

> Hi Henning,
> 
> On 3 July 2018 at 14:38, Henning Schild <henning.schild@siemens.com>
> wrote:
> > Create a struct that holds the format details for the supported
> > formats. At the moment that is still just "PGP". This commit
> > prepares for the introduction of more formats, that might use other
> > programs and match other signatures.
> >
> > Signed-off-by: Henning Schild <henning.schild@siemens.com>  
> 
> Welcome to the mailing list! :-)

> I'll just comment on a few thoughts I had while skimming this.
> 
> >  static char *configured_signing_key;
> > -static const char *gpg_format = "PGP";
> > -static const char *gpg_program = "gpg";
> > +struct gpg_format_data {
> > +       const char *format;
> > +       const char *program;
> > +       const char *extra_args_verify[1];
> > +       const char *sigs[2];
> > +};
> >
> >  #define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
> >  #define PGP_MESSAGE "-----BEGIN PGP MESSAGE-----"
> >
> > +enum gpgformats { PGP_FMT };
> > +struct gpg_format_data gpg_formats[] = {
> > +       { .format = "PGP", .program = "gpg",
> > +         .extra_args_verify = { "--keyid-format=long", },
> > +         .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },
> > +       },
> > +};  
> 
> I think those trailing commas are ok now, but I'm not sure...
> 
> I had the same thought about designated initializers. Those should be
> ok now, c.f. cbc0f81d96 (strbuf: use designated initializers in
> STRBUF_INIT, 2017-07-10) and a73b3680c4 (Add and use generic name->id
> mapping code for color slot parsing, 2018-05-26).

> > +static const char *gpg_format = "PGP";
> > +
> > +static struct gpg_format_data *get_format_data(void)
> > +{
> > +       int i;
> > +       for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> > +               if (!strcmp(gpg_formats[i].format, gpg_format))
> > +                       return gpg_formats + i;
> > +       assert(0);  
> 
> This might be better written as `BUG("bad gpg_format '%s'",
> gpg_format);` or something like that.
> 
> (It's not supposed to be triggered, not even by invalid data from the
> user, right?)

> 
> >         if (!strcmp(var, "gpg.format")) {
> > -               if (!strcmp(value, "PGP"))  
> 
> This line was added in patch 3. It errors out precisely when
> gpg.format is "PGP", no? That this doesn't break the whole series is
> explained by 1) it being removed in this patch 4, and 2) there being
> no tests. It makes me wonder if something like patch 5 (test
> gpg.format) could be part of patch 3, both with negative ("=
> malformed") and positive ("= PGP") tests?

> > +               j = 0;
> > +               for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> > +                       if (!strcmp(value, gpg_formats[i].format)) {
> > +                               j++;
> > +                               break;
> > +                       }
> > +               if (!j)
> >                         return error("malformed value for %s: %s",
> > var, value);  
> 
> `if (i == ARRAY_SIZE(gpg_formats))` and drop `j`?
> 
> Or check whether `get_format_data()` returns NULL? Hmm, well you
> can't, since it takes its "input" from a global variable...
> 
> If you want to keep that global nature, the duplication of
> search-logic could perhaps be avoided by having a helper function for
> returning the index of a gpg_format (or -1).

>> +enum gpgformats { PGP_FMT };
>> +struct gpg_format_data gpg_formats[] = {
>> +       { .format = "PGP", .program = "gpg",
>> +         .extra_args_verify = { "--keyid-format=long", },
>> +         .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },
>> +       },
>> +};
>
> I think those trailing commas are ok now, but I'm not sure...
>
> I had the same thought about designated initializers. Those should be ok
> now, c.f. cbc0f81d96 (strbuf: use designated initializers in STRBUF_INIT,
> 2017-07-10) and a73b3680c4 (Add and use generic name->id mapping code
> for color slot parsing, 2018-05-26).

> Martin Ågren <martin.agren@gmail.com> writes:
> 
> >> +enum gpgformats { PGP_FMT };
> >> +struct gpg_format_data gpg_formats[] = {
> >> +       { .format = "PGP", .program = "gpg",
> >> +         .extra_args_verify = { "--keyid-format=long", },
> >> +         .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },
> >> +       },
> >> +};  
> >
> > I think those trailing commas are ok now, but I'm not sure...
> >
> > I had the same thought about designated initializers. Those should
> > be ok now, c.f. cbc0f81d96 (strbuf: use designated initializers in
> > STRBUF_INIT, 2017-07-10) and a73b3680c4 (Add and use generic
> > name->id mapping code for color slot parsing, 2018-05-26).  
> 
> As you said, we dipped our toes in designated initializers in both
> struct and array, i.e. { .field = init }, { [offset] = init } last
> summer and we haven't got complaints from minor platforms so far.
> 
> The "comma" thing you are wondering is something else.  The comma we
> see above is after the last element in an array's initializer (and
> the last element in a struct's initializer), which we have been
> happily using from very early days (and they are kosher ANSI C).
> 
> What we've been avoiding was the comma after the last element in the
> enum (in other words, if PGP_FMT had ',' after it in the above
> quoted addition, that would have been violation of that rule), as
> having such a trailing comma used to be ANSI C violation as well.  I
> do not recall offhand if we loosened that deliberately.
> 
> 4b05548f ("enums: omit trailing comma for portability", 2010-05-14),
> c9b6782a ("enums: omit trailing comma for portability", 2011-03-16)

> Am Fri, 6 Jul 2018 10:24:58 -0700
> schrieb Junio C Hamano <gitster@pobox.com>:
> > Martin Ågren <martin.agren@gmail.com> writes:
> > >> +struct gpg_format_data gpg_formats[] = {
> > >> +       { .format = "PGP", .program = "gpg",
> > >> +         .extra_args_verify = { "--keyid-format=long", },
> > >> +         .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },
> > >> +       },
> > >> +};
> > >
> > > I think those trailing commas are ok now, but I'm not sure...
> >
> > What we've been avoiding was the comma after the last element in the
> > enum (in other words, if PGP_FMT had ',' after it in the above
> > quoted addition, that would have been violation of that rule), as
> > having such a trailing comma used to be ANSI C violation as well.
>
> I guess that means the style is acceptable and does not require
> changes, please correct me if i am wrong.

> On Mon, Jul 9, 2018 at 4:22 AM Henning Schild
> <henning.schild@siemens.com> wrote:
>> Am Fri, 6 Jul 2018 10:24:58 -0700
>> schrieb Junio C Hamano <gitster@pobox.com>:
>> > Martin Ågren <martin.agren@gmail.com> writes:
>> > >> +struct gpg_format_data gpg_formats[] = {
>> > >> +       { .format = "PGP", .program = "gpg",
>> > >> +         .extra_args_verify = { "--keyid-format=long", },
>> > >> +         .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },
>> > >> +       },
>> > >> +};
>> > >
>> > > I think those trailing commas are ok now, but I'm not sure...
>> >
>> > What we've been avoiding was the comma after the last element in the
>> > enum (in other words, if PGP_FMT had ',' after it in the above
>> > quoted addition, that would have been violation of that rule), as
>> > having such a trailing comma used to be ANSI C violation as well.
>>
>> I guess that means the style is acceptable and does not require
>> changes, please correct me if i am wrong.
>
> The trailing comma in the 'sigs' initializer is bothersome because
> 'sigs' is declared as a 2-element array, and this initializer already
> has two elements. Therefore, the comma is misleading to anyone reading
> the code, making it appears as if additional items can be added. For
> that reason, alone, it would be nice to see the unnecessary comma
> removed.
>
> Ditto with regard to the trailing comma in the 'extra_args_verify'
> initializer since 'extra_args_verify' is declared as a 1-element
> array.

> What we've been avoiding was the comma after the last element in the
> enum (in other words, if PGP_FMT had ',' after it in the above
> quoted addition, that would have been violation of that rule), as
> having such a trailing comma used to be ANSI C violation as well.  I
> do not recall offhand if we loosened that deliberately.
> 
> 4b05548f ("enums: omit trailing comma for portability", 2010-05-14),
> c9b6782a ("enums: omit trailing comma for portability", 2011-03-16)

> On Fri, Jul 06, 2018 at 10:24:58AM -0700, Junio C Hamano wrote:
>
>> What we've been avoiding was the comma after the last element in the
>> enum (in other words, if PGP_FMT had ',' after it in the above
>> quoted addition, that would have been violation of that rule), as
>> having such a trailing comma used to be ANSI C violation as well.  I
>> do not recall offhand if we loosened that deliberately.
>> 
>> 4b05548f ("enums: omit trailing comma for portability", 2010-05-14),
>> c9b6782a ("enums: omit trailing comma for portability", 2011-03-16)
>
> I think we accidentally did a weather-balloon in e1327023ea (grep:
> refactor the concept of "grep source" into an object, 2012-02-02).
> It's still there and nobody has complained about it yet.
>
> So I think we can consider that requirement loosened at this point.

> On Fri, Jul 06, 2018 at 10:24:58AM -0700, Junio C Hamano wrote:
>
>> What we've been avoiding was the comma after the last element in the
>> enum (in other words, if PGP_FMT had ',' after it in the above
>> quoted addition, that would have been violation of that rule), as
>> having such a trailing comma used to be ANSI C violation as well.  I
>> do not recall offhand if we loosened that deliberately.
>> 
>> 4b05548f ("enums: omit trailing comma for portability", 2010-05-14),
>> c9b6782a ("enums: omit trailing comma for portability", 2011-03-16)
>
> I think we accidentally did a weather-balloon in e1327023ea (grep:
> refactor the concept of "grep source" into an object, 2012-02-02).
> It's still there and nobody has complained about it yet.
>
> So I think we can consider that requirement loosened at this point.
>
> -Peff

> Jeff King <peff@peff.net> writes:
> 
> > On Fri, Jul 06, 2018 at 10:24:58AM -0700, Junio C Hamano wrote:
> >
> >> What we've been avoiding was the comma after the last element in the
> >> enum (in other words, if PGP_FMT had ',' after it in the above
> >> quoted addition, that would have been violation of that rule), as
> >> having such a trailing comma used to be ANSI C violation as well.  I
> >> do not recall offhand if we loosened that deliberately.
> >> 
> >> 4b05548f ("enums: omit trailing comma for portability", 2010-05-14),
> >> c9b6782a ("enums: omit trailing comma for portability", 2011-03-16)
> >
> > I think we accidentally did a weather-balloon in e1327023ea (grep:
> > refactor the concept of "grep source" into an object, 2012-02-02).
> > It's still there and nobody has complained about it yet.
> >
> > So I think we can consider that requirement loosened at this point.
> >
> > -Peff
> 
> Yup, thanks for digging it out.  It seems that I did the same
> digging some time ago but forgot about it.

> Add test cases to cover the new X509/gpgsm support. Most of them
> resemble existing ones. They just switch the format to X509 and set the
> signingkey when creating signatures. Validation of signatures does not
> need any configuration of git, it does need gpgsm to be configured to
> trust the key(-chain).
> We generate a self-signed key for committer@example.com and configure
> gpgsm to trust it.
> 
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  t/lib-gpg.sh               |  9 ++++++-
>  t/lib-gpg/gpgsm-gen-key.in |  6 +++++
>  t/t4202-log.sh             | 66 ++++++++++++++++++++++++++++++++++++++++++++++
>  t/t5534-push-signed.sh     | 52 ++++++++++++++++++++++++++++++++++++
>  t/t7003-filter-branch.sh   | 15 +++++++++++
>  t/t7030-verify-tag.sh      | 47 +++++++++++++++++++++++++++++++--
>  t/t7600-merge.sh           | 31 ++++++++++++++++++++++
>  7 files changed, 223 insertions(+), 3 deletions(-)
>  create mode 100644 t/lib-gpg/gpgsm-gen-key.in
> 
> diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
> index a5d3b2cba..9dcb4e990 100755
> --- a/t/lib-gpg.sh
> +++ b/t/lib-gpg.sh
> @@ -38,7 +38,14 @@ then
>  			"$TEST_DIRECTORY"/lib-gpg/ownertrust &&
>  		gpg --homedir "${GNUPGHOME}" </dev/null >/dev/null 2>&1 \
>  			--sign -u committer@example.com &&
> -		test_set_prereq GPG
> +		test_set_prereq GPG &&
> +		echo | gpgsm --homedir "${GNUPGHOME}" -o "$TEST_DIRECTORY"/lib-gpg/gpgsm.crt.user --passphrase-fd 0 --pinentry-mode loopback --generate-key --batch "$TEST_DIRECTORY"/lib-gpg/gpgsm-gen-key.in &&
> +		gpgsm --homedir "${GNUPGHOME}" --import "$TEST_DIRECTORY"/lib-gpg/gpgsm.crt.user &&
> +		gpgsm --homedir "${GNUPGHOME}" -K | grep fingerprint: | cut -d" " -f4 | tr -d '\n' > ${GNUPGHOME}/trustlist.txt &&
> +		echo " S relax" >> ${GNUPGHOME}/trustlist.txt &&
> +		(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
> +		echo hello | gpgsm --homedir "${GNUPGHOME}" -u committer@example.com -o /dev/null --sign - 2>&1 &&
> +		test_set_prereq GPGSM

> On Tue, Jul 03, 2018 at 02:38:20PM +0200, Henning Schild wrote:
> > Add test cases to cover the new X509/gpgsm support. Most of them
> > resemble existing ones. They just switch the format to X509 and set
> > the signingkey when creating signatures. Validation of signatures
> > does not need any configuration of git, it does need gpgsm to be
> > configured to trust the key(-chain).
> > We generate a self-signed key for committer@example.com and
> > configure gpgsm to trust it.
> > 
> > Signed-off-by: Henning Schild <henning.schild@siemens.com>
> > ---
> >  t/lib-gpg.sh               |  9 ++++++-
> >  t/lib-gpg/gpgsm-gen-key.in |  6 +++++
> >  t/t4202-log.sh             | 66
> > ++++++++++++++++++++++++++++++++++++++++++++++
> > t/t5534-push-signed.sh     | 52
> > ++++++++++++++++++++++++++++++++++++ t/t7003-filter-branch.sh   |
> > 15 +++++++++++ t/t7030-verify-tag.sh      | 47
> > +++++++++++++++++++++++++++++++-- t/t7600-merge.sh           | 31
> > ++++++++++++++++++++++ 7 files changed, 223 insertions(+), 3
> > deletions(-) create mode 100644 t/lib-gpg/gpgsm-gen-key.in
> > 
> > diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
> > index a5d3b2cba..9dcb4e990 100755
> > --- a/t/lib-gpg.sh
> > +++ b/t/lib-gpg.sh
> > @@ -38,7 +38,14 @@ then
> >  			"$TEST_DIRECTORY"/lib-gpg/ownertrust &&
> >  		gpg --homedir "${GNUPGHOME}" </dev/null >/dev/null
> > 2>&1 \ --sign -u committer@example.com &&
> > -		test_set_prereq GPG
> > +		test_set_prereq GPG &&
> > +		echo | gpgsm --homedir "${GNUPGHOME}" -o
> > "$TEST_DIRECTORY"/lib-gpg/gpgsm.crt.user --passphrase-fd 0
> > --pinentry-mode loopback --generate-key --batch
> > "$TEST_DIRECTORY"/lib-gpg/gpgsm-gen-key.in &&
> > +		gpgsm --homedir "${GNUPGHOME}" --import
> > "$TEST_DIRECTORY"/lib-gpg/gpgsm.crt.user &&
> > +		gpgsm --homedir "${GNUPGHOME}" -K | grep
> > fingerprint: | cut -d" " -f4 | tr -d '\n' >
> > ${GNUPGHOME}/trustlist.txt &&
> > +		echo " S relax" >> ${GNUPGHOME}/trustlist.txt &&
> > +		(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
> > +		echo hello | gpgsm --homedir "${GNUPGHOME}" -u
> > committer@example.com -o /dev/null --sign - 2>&1 &&
> > +		test_set_prereq GPGSM  
> 
> It looks like the GPGSM prerequisite will only be set if the GPG
> prerequisite is set as well.  Do we want to consider the case when the
> user might have gpgsm but not gpg?

> This series adds support for signing commits with gpgsm.
> 
> The first two patches are cleanups of gpg-interface, while the next
> four prepare for the introduction of the actual feature in patch 7.
> Finally patch 8 extends the testsuite to cover the new feature.
> 
> This series can be seen as a follow up of a series that appeared under
> the name "gpg-interface: Multiple signing tools" in april 2018 [1]. After
> that series was not merged i decided to get my patches ready. The
> original series aimed at being generic for any sort of signing tool, while
> this series just introduced the X509 variant of gpg. (gpgsm)
> I collected authors and reviewers of that first series and already put them
> on cc.

> On Tue, Jul 03, 2018 at 02:38:12PM +0200, Henning Schild wrote:
> > This series adds support for signing commits with gpgsm.
> > 
> > The first two patches are cleanups of gpg-interface, while the next
> > four prepare for the introduction of the actual feature in patch 7.
> > Finally patch 8 extends the testsuite to cover the new feature.
> > 
> > This series can be seen as a follow up of a series that appeared
> > under the name "gpg-interface: Multiple signing tools" in april
> > 2018 [1]. After that series was not merged i decided to get my
> > patches ready. The original series aimed at being generic for any
> > sort of signing tool, while this series just introduced the X509
> > variant of gpg. (gpgsm) I collected authors and reviewers of that
> > first series and already put them on cc.  
> 
> Overall, I think this is heading in a good direction.  I left a few
> comments, but it seemed pretty sane.