[RFC PATCH] builtin:tag:verify_tag: allow gpg output + pretty

[RFC PATCH] builtin:tag:verify_tag: allow gpg output + pretty

[santiago]

From: Santiago Torres <santiago@nyu.edu>

On the git tag -v code, there is a guard to suppress gpg output if a
pretty format is provided. The rationale for this is that the gpg output
*and* the pretty formats together may conflict with each other. However,
both outputs are directed to different output streams and, as such,
they can safely coexist. Drop the guard clause and use
GPG_VERIFY_VERBOSE regardless of the pretty format

Signed-off-by: Santiago Torres <santiago@nyu.edu>
---
 builtin/tag.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 8c493a569..4b91c769c 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -110,9 +110,6 @@ static int verify_tag(const char *name, const char *ref,
 	const struct ref_format *format = cb_data;
 	flags = GPG_VERIFY_VERBOSE;
 
-	if (format->format)
-		flags = GPG_VERIFY_OMIT_STATUS;
-
 	if (gpg_verify_tag(oid, name, flags))
 		return -1;
 
-- 
2.21.0

Re: [RFC PATCH] builtin:tag:verify_tag: allow gpg output + pretty

[Santiago Torres Arias]

[-- Attachment #1: Type: text/plain, Size: 808 bytes --]

On Fri, Apr 12, 2019 at 04:14:32PM -0400, santiago@nyu.edu wrote:
> From: Santiago Torres <santiago@nyu.edu>
> 
> On the git tag -v code, there is a guard to suppress gpg output if a
> pretty format is provided. The rationale for this is that the gpg output
> *and* the pretty formats together may conflict with each other. However,
> both outputs are directed to different output streams and, as such,
> they can safely coexist. Drop the guard clause and use
> GPG_VERIFY_VERBOSE regardless of the pretty format

I tried digging for the rationale for this, but I couldn't figure it
out. I noticed that the output of gpg verification is sent to stderr,
while the pretty format goes towards stdout, and they can thus be
multiplexed accordingly.

What do you guys think?

Thanks!
-Santiago.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH] builtin:tag:verify_tag: allow gpg output + pretty

[Jeff King]

On Fri, Apr 12, 2019 at 04:14:32PM -0400, santiago@nyu.edu wrote:

> From: Santiago Torres <santiago@nyu.edu>
> 
> On the git tag -v code, there is a guard to suppress gpg output if a
> pretty format is provided. The rationale for this is that the gpg output
> *and* the pretty formats together may conflict with each other. However,
> both outputs are directed to different output streams and, as such,
> they can safely coexist. Drop the guard clause and use
> GPG_VERIFY_VERBOSE regardless of the pretty format

I think this makes sense. My first worry was whether this would be
surprising to any callers, but as you note, they go to different
streams.

However, I don't think this patch is quite right, as it causes us to
dump the whole tag contents to stdout, as well. E.g.:

  [before]
  $ git tag -v --format='foo %(tag)' v2.21.0
  foo v2.21.0

  [after]
  $ git tag -v --format='foo %(tag)' v2.21.0
  object 8104ec994ea3849a968b4667d072fedd1e688642
  type commit
  tag v2.21.0
  tagger Junio C Hamano <gitster@pobox.com> 1551023739 -0800
  
  Git 2.21
  gpg: Signature made Sun Feb 24 10:55:39 2019 EST
  gpg:                using RSA key E1F036B1FEE7221FC778ECEFB0B5E88696AFE6CB
  gpg: Good signature from "Junio C Hamano <gitster@pobox.com>" [full]
  gpg:                 aka "Junio C Hamano <jch@google.com>" [full]
  gpg:                 aka "Junio C Hamano <junio@pobox.com>" [full]
  foo v2.21.0

I think "git verify-tag" would need similar treatment, too:

  $ git verify-tag v2.21.0
  gpg: Signature made Sun Feb 24 10:55:39 2019 EST
  gpg:                using RSA key E1F036B1FEE7221FC778ECEFB0B5E88696AFE6CB
  gpg: Good signature from "Junio C Hamano <gitster@pobox.com>" [full]
  gpg:                 aka "Junio C Hamano <jch@google.com>" [full]
  gpg:                 aka "Junio C Hamano <junio@pobox.com>" [full]

  $ git verify-tag --format='foo %(tag)' v2.21.0
  foo v2.21.0

In some ways I'm less concerned about verify-tag, though, because the
point is that it should be scriptable. And scraping gpg's stderr is not
ideal there. We should be parsing --status-fd ourselves and making the
result available via format specifier, similar to the way "log
--format=%G?" works.

So I think ultimately that's the direction we want to go, but I think
in the meantime restoring the gpg output to stderr especially for the
porcelain "git tag -v" makes sense for human eyes.

-Peff

Re: [RFC PATCH] builtin:tag:verify_tag: allow gpg output + pretty

[Santiago Torres Arias]

[-- Attachment #1: Type: text/plain, Size: 2164 bytes --]

> However, I don't think this patch is quite right, as it causes us to
> dump the whole tag contents to stdout, as well. E.g.:
> 
>   [before]
>   $ git tag -v --format='foo %(tag)' v2.21.0
>   foo v2.21.0
> 
>   [after]
>   $ git tag -v --format='foo %(tag)' v2.21.0
>   object 8104ec994ea3849a968b4667d072fedd1e688642
>   type commit
>   tag v2.21.0
>   tagger Junio C Hamano <gitster@pobox.com> 1551023739 -0800
>   
>   Git 2.21
>   gpg: Signature made Sun Feb 24 10:55:39 2019 EST
>   gpg:                using RSA key E1F036B1FEE7221FC778ECEFB0B5E88696AFE6CB
>   gpg: Good signature from "Junio C Hamano <gitster@pobox.com>" [full]
>   gpg:                 aka "Junio C Hamano <jch@google.com>" [full]
>   gpg:                 aka "Junio C Hamano <junio@pobox.com>" [full]
>   foo v2.21.0
> 
> I think "git verify-tag" would need similar treatment, too:
> 
>   $ git verify-tag v2.21.0
>   gpg: Signature made Sun Feb 24 10:55:39 2019 EST
>   gpg:                using RSA key E1F036B1FEE7221FC778ECEFB0B5E88696AFE6CB
>   gpg: Good signature from "Junio C Hamano <gitster@pobox.com>" [full]
>   gpg:                 aka "Junio C Hamano <jch@google.com>" [full]
>   gpg:                 aka "Junio C Hamano <junio@pobox.com>" [full]
> 
>   $ git verify-tag --format='foo %(tag)' v2.21.0
>   foo v2.21.0
> 

Ah, let me look into these issues. I'm almost sure I also need to review
the test suite and adapt it to this behavior.

> In some ways I'm less concerned about verify-tag, though, because the
> point is that it should be scriptable. And scraping gpg's stderr is not
> ideal there. We should be parsing --status-fd ourselves and making the
> result available via format specifier, similar to the way "log
> --format=%G?" works.

I think that would be great, as we could make it simpler for verifiers
to parse gpg output.

> So I think ultimately that's the direction we want to go, but I think
> in the meantime restoring the gpg output to stderr especially for the
> porcelain "git tag -v" makes sense for human eyes.

Great! let me re-roll and make a more formal take on this.

Thanks!
-Santiago

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH] builtin:tag:verify_tag: allow gpg output + pretty

[Jeff King]

On Mon, Apr 22, 2019 at 11:46:56AM -0400, Santiago Torres Arias wrote:

> > In some ways I'm less concerned about verify-tag, though, because the
> > point is that it should be scriptable. And scraping gpg's stderr is not
> > ideal there. We should be parsing --status-fd ourselves and making the
> > result available via format specifier, similar to the way "log
> > --format=%G?" works.
> 
> I think that would be great, as we could make it simpler for verifiers
> to parse gpg output.

Alternatively, we could make it an option to dump the --status-fd output
to stderr (or to a custom fd). That still leaves the caller with the
responsibility to parse gpg's output, but at least they're parsing the
machine-readable bits and not the regular human-readable stderr.

-Peff

Re: [RFC PATCH] builtin:tag:verify_tag: allow gpg output + pretty

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 3475 bytes --]

On Mon, Apr 22, 2019 at 12:02:11PM -0400, Jeff King wrote:
> On Mon, Apr 22, 2019 at 11:46:56AM -0400, Santiago Torres Arias wrote:
> 
> > > In some ways I'm less concerned about verify-tag, though, because the
> > > point is that it should be scriptable. And scraping gpg's stderr is not
> > > ideal there. We should be parsing --status-fd ourselves and making the
> > > result available via format specifier, similar to the way "log
> > > --format=%G?" works.
> > 
> > I think that would be great, as we could make it simpler for verifiers
> > to parse gpg output.
> 
> Alternatively, we could make it an option to dump the --status-fd output
> to stderr (or to a custom fd). That still leaves the caller with the
> responsibility to parse gpg's output, but at least they're parsing the
> machine-readable bits and not the regular human-readable stderr.

Don't we already have that for verify-tag and verify-commit? I recall
adding "--raw" for that very reason:

genre ok % git verify-tag --raw v2.21.0
[GNUPG:] NEWSIG
[GNUPG:] KEYEXPIRED 1442879137
[GNUPG:] KEYEXPIRED 1505842336
[GNUPG:] KEY_CONSIDERED 96E07AF25771955980DAD10020D04E5A713660A7 0
[GNUPG:] KEYEXPIRED 1442879137
[GNUPG:] SIG_ID NZHib/GfN4TzXBhuI9ABwYXqluE 2019-02-24 1551023739
[GNUPG:] KEYEXPIRED 1442879137
[GNUPG:] KEYEXPIRED 1505842336
[GNUPG:] KEY_CONSIDERED 96E07AF25771955980DAD10020D04E5A713660A7 0
[GNUPG:] KEYEXPIRED 1442879137
[GNUPG:] KEYEXPIRED 1505842336
[GNUPG:] KEY_CONSIDERED 96E07AF25771955980DAD10020D04E5A713660A7 0
[GNUPG:] EXPKEYSIG B0B5E88696AFE6CB Junio C Hamano <gitster@pobox.com>
[GNUPG:] KEYEXPIRED 1442879137
[GNUPG:] KEYEXPIRED 1505842336
[GNUPG:] KEY_CONSIDERED 96E07AF25771955980DAD10020D04E5A713660A7 0
[GNUPG:] KEYEXPIRED 1442879137
[GNUPG:] KEYEXPIRED 1505842336
[GNUPG:] KEY_CONSIDERED 96E07AF25771955980DAD10020D04E5A713660A7 0
[GNUPG:] VALIDSIG E1F036B1FEE7221FC778ECEFB0B5E88696AFE6CB 2019-02-24 1551023739 0 4 0 1 8 00 96E07AF25771955980DAD10020D04E5A713660A7
[GNUPG:] KEYEXPIRED 1442879137
[GNUPG:] KEYEXPIRED 1505842336
[GNUPG:] KEY_CONSIDERED 96E07AF25771955980DAD10020D04E5A713660A7 0
[GNUPG:] KEYEXPIRED 1442879137
[GNUPG:] KEYEXPIRED 1505842336
[GNUPG:] KEY_CONSIDERED 96E07AF25771955980DAD10020D04E5A713660A7 0
[GNUPG:] KEYEXPIRED 1442879137
[GNUPG:] KEYEXPIRED 1505842336
[GNUPG:] KEY_CONSIDERED 96E07AF25771955980DAD10020D04E5A713660A7 0
[GNUPG:] TOFU_USER 96E07AF25771955980DAD10020D04E5A713660A7 gitster@pobox.com
[GNUPG:] TOFU_STATS 2 1 0 auto 1555974073 1555974073 0 0 2 1 0
[GNUPG:] TOFU_STATS_LONG gitster@pobox.com: Verified 1~signature in the past 0~seconds.  Encrypted%0A0 messages.
[GNUPG:] TOFU_USER 96E07AF25771955980DAD10020D04E5A713660A7 jch@google.com
[GNUPG:] TOFU_STATS 2 1 0 auto 1555974073 1555974073 0 0 2 1 0
[GNUPG:] TOFU_STATS_LONG jch@google.com: Verified 1~signature in the past 0~seconds.  Encrypted 0%0Amessages.
[GNUPG:] TOFU_USER 96E07AF25771955980DAD10020D04E5A713660A7 junio@pobox.com
[GNUPG:] TOFU_STATS 2 1 0 auto 1555974073 1555974073 0 0 2 1 0
[GNUPG:] TOFU_STATS_LONG junio@pobox.com: Verified 1~signature in the past 0~seconds.  Encrypted%0A0 messages.
[GNUPG:] VERIFICATION_COMPLIANCE_MODE 23

The idea was that users might want to restrict signatures to using
subkeys or certain algorithms or what-have-you, and this was the easiest
way to let people have all of that power.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [RFC PATCH] builtin:tag:verify_tag: allow gpg output + pretty

[Santiago Torres Arias]

[-- Attachment #1: Type: text/plain, Size: 3242 bytes --]

On Mon, Apr 22, 2019 at 11:07:01PM +0000, brian m. carlson wrote:
> On Mon, Apr 22, 2019 at 12:02:11PM -0400, Jeff King wrote:
> > On Mon, Apr 22, 2019 at 11:46:56AM -0400, Santiago Torres Arias wrote:
> > 
> > > I think that would be great, as we could make it simpler for verifiers
> > > to parse gpg output.
> > 
> > Alternatively, we could make it an option to dump the --status-fd output
> > to stderr (or to a custom fd). That still leaves the caller with the
> > responsibility to parse gpg's output, but at least they're parsing the
> > machine-readable bits and not the regular human-readable stderr.
> 
> Don't we already have that for verify-tag and verify-commit? I recall
> adding "--raw" for that very reason:
> 
> genre ok % git verify-tag --raw v2.21.0
> [GNUPG:] NEWSIG
> [GNUPG:] KEYEXPIRED 1442879137
> [GNUPG:] KEYEXPIRED 1505842336
> [GNUPG:] KEY_CONSIDERED 96E07AF25771955980DAD10020D04E5A713660A7 0
> [GNUPG:] KEYEXPIRED 1442879137
> [GNUPG:] SIG_ID NZHib/GfN4TzXBhuI9ABwYXqluE 2019-02-24 1551023739
> [GNUPG:] KEYEXPIRED 1442879137
> [GNUPG:] KEYEXPIRED 1505842336
> [GNUPG:] KEY_CONSIDERED 96E07AF25771955980DAD10020D04E5A713660A7 0
> [GNUPG:] KEYEXPIRED 1442879137
> [GNUPG:] KEYEXPIRED 1505842336
> [GNUPG:] KEY_CONSIDERED 96E07AF25771955980DAD10020D04E5A713660A7 0
> [GNUPG:] EXPKEYSIG B0B5E88696AFE6CB Junio C Hamano <gitster@pobox.com>
> [GNUPG:] KEYEXPIRED 1442879137
> [GNUPG:] KEYEXPIRED 1505842336
> [GNUPG:] KEY_CONSIDERED 96E07AF25771955980DAD10020D04E5A713660A7 0
> [GNUPG:] KEYEXPIRED 1442879137
> [GNUPG:] KEYEXPIRED 1505842336
> [GNUPG:] KEY_CONSIDERED 96E07AF25771955980DAD10020D04E5A713660A7 0
> [GNUPG:] VALIDSIG E1F036B1FEE7221FC778ECEFB0B5E88696AFE6CB 2019-02-24 1551023739 0 4 0 1 8 00 96E07AF25771955980DAD10020D04E5A713660A7
> [GNUPG:] KEYEXPIRED 1442879137
> [GNUPG:] KEYEXPIRED 1505842336
> [GNUPG:] KEY_CONSIDERED 96E07AF25771955980DAD10020D04E5A713660A7 0
> [GNUPG:] KEYEXPIRED 1442879137
> [GNUPG:] KEYEXPIRED 1505842336
> [GNUPG:] KEY_CONSIDERED 96E07AF25771955980DAD10020D04E5A713660A7 0
> [GNUPG:] KEYEXPIRED 1442879137
> [GNUPG:] KEYEXPIRED 1505842336
> [GNUPG:] KEY_CONSIDERED 96E07AF25771955980DAD10020D04E5A713660A7 0
> [GNUPG:] TOFU_USER 96E07AF25771955980DAD10020D04E5A713660A7 gitster@pobox.com
> [GNUPG:] TOFU_STATS 2 1 0 auto 1555974073 1555974073 0 0 2 1 0
> [GNUPG:] TOFU_STATS_LONG gitster@pobox.com: Verified 1~signature in the past 0~seconds.  Encrypted%0A0 messages.
> [GNUPG:] TOFU_USER 96E07AF25771955980DAD10020D04E5A713660A7 jch@google.com
> [GNUPG:] TOFU_STATS 2 1 0 auto 1555974073 1555974073 0 0 2 1 0
> [GNUPG:] TOFU_STATS_LONG jch@google.com: Verified 1~signature in the past 0~seconds.  Encrypted 0%0Amessages.
> [GNUPG:] TOFU_USER 96E07AF25771955980DAD10020D04E5A713660A7 junio@pobox.com
> [GNUPG:] TOFU_STATS 2 1 0 auto 1555974073 1555974073 0 0 2 1 0
> [GNUPG:] TOFU_STATS_LONG junio@pobox.com: Verified 1~signature in the past 0~seconds.  Encrypted%0A0 messages.
> [GNUPG:] VERIFICATION_COMPLIANCE_MODE 23

I think this interface only shows you raw gpg output, but not any
--format= specifiers that you may want. The idea would be to support
both. Or am I missing something?

Thanks,
-Santiago.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH] builtin:tag:verify_tag: allow gpg output + pretty

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1866 bytes --]

On Mon, Apr 22, 2019 at 07:26:29PM -0400, Santiago Torres Arias wrote:
> On Mon, Apr 22, 2019 at 11:07:01PM +0000, brian m. carlson wrote:
> > On Mon, Apr 22, 2019 at 12:02:11PM -0400, Jeff King wrote:
> > > On Mon, Apr 22, 2019 at 11:46:56AM -0400, Santiago Torres Arias wrote:
> > > 
> > > > I think that would be great, as we could make it simpler for verifiers
> > > > to parse gpg output.
> > > 
> > > Alternatively, we could make it an option to dump the --status-fd output
> > > to stderr (or to a custom fd). That still leaves the caller with the
> > > responsibility to parse gpg's output, but at least they're parsing the
> > > machine-readable bits and not the regular human-readable stderr.
> > 
> > Don't we already have that for verify-tag and verify-commit? I recall
> > adding "--raw" for that very reason:
> 
> I think this interface only shows you raw gpg output, but not any
> --format= specifiers that you may want. The idea would be to support
> both. Or am I missing something?

My response was mostly in reply to Peff's suggestion that we have an
option to dump the --status-fd output, which we have. I think that
behavior properly belongs to verify-tag and verify-commit, which are
plumbing.

I'm not so sure that it's necessary to have the --status-fd output in git
tag -v, which is more for interactive use, although I don't feel
strongly about it. I think of --format as a tool I typically want to use
on multiple of something, and while it's theoretically possible to
distinguish multiple signatures by GnuPG's "NEWSIG", parsing multiple
tags' worth of output between standard output and standard error is
going to be pretty unpleasant.

As I said, I don't feel strongly about it, so if you want to implement
it, feel free.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [RFC PATCH] builtin:tag:verify_tag: allow gpg output + pretty

[Jeff King]

On Mon, Apr 22, 2019 at 11:07:01PM +0000, brian m. carlson wrote:

> On Mon, Apr 22, 2019 at 12:02:11PM -0400, Jeff King wrote:
> > On Mon, Apr 22, 2019 at 11:46:56AM -0400, Santiago Torres Arias wrote:
> > 
> > > > In some ways I'm less concerned about verify-tag, though, because the
> > > > point is that it should be scriptable. And scraping gpg's stderr is not
> > > > ideal there. We should be parsing --status-fd ourselves and making the
> > > > result available via format specifier, similar to the way "log
> > > > --format=%G?" works.
> > > 
> > > I think that would be great, as we could make it simpler for verifiers
> > > to parse gpg output.
> > 
> > Alternatively, we could make it an option to dump the --status-fd output
> > to stderr (or to a custom fd). That still leaves the caller with the
> > responsibility to parse gpg's output, but at least they're parsing the
> > machine-readable bits and not the regular human-readable stderr.
> 
> Don't we already have that for verify-tag and verify-commit? I recall
> adding "--raw" for that very reason:

Heh. Today I learned about "--raw". :)

Thanks for pointing it out. I do still think it would be nice for some
cases to have --format specifiers to get the basic info, but I am glad
that we already have a reasonable method that scripts can use.

It might make sense to make it available from the git-tag porcelain,
too, but since the point is scripting, I'm not sure it's all that
important.

It looks like using "--format" suppresses it, too, which we'd probably
want to fix (presumably it's the same as the fix for the non-raw
output).

> The idea was that users might want to restrict signatures to using
> subkeys or certain algorithms or what-have-you, and this was the easiest
> way to let people have all of that power.

Yeah, that makes perfect sense.

-Peff