| Message ID | 20190412201432.11328-1-santiago@nyu.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] builtin:tag:verify_tag: allow gpg output + pretty | expand |
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.
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
> 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
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
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.
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.
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.
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
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;