{"thread":{"id":"51003","subject":"[PATCH 0/2] tag verification: do not mute gpg output","startedAt":"2019-04-27T20:21:42Z","lastAt":"2019-05-09T17:40:22Z","messageCount":7,"participants":["santiago@nyu.edu","Jeff King","Santiago Torres Arias"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"374606","messageId":"20190427202123.15380-1-santiago@nyu.edu","threadId":"51003","inReplyTo":null,"subject":"[PATCH 0/2] tag verification: do not mute gpg output","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2019-04-27T20:21:21Z","receivedAt":"2019-04-27T20:21:42Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"From: Santiago Torres <santiago@nyu.edu>\n\nThe default behavior of the tag verification functions used to quiet\ndown the gpg output if --format was passed. The rationale for this was\nto avoid --format to be litterred by the gpg output. However, this may\nbe unnecessary because the gpg output is already streamed to stderr and\nthus can be easily multiplexed. \n\nSantiago Torres (2):\n  builtin/tag: do not omit -v gpg out for --format\n  builtin/verify-tag: do not omit gpg on --format\n\n builtin/tag.c        | 6 +++---\n builtin/verify-tag.c | 6 ++----\n 2 files changed, 5 insertions(+), 7 deletions(-)\n\n-- \n2.21.0\n\n"},{"id":"374607","messageId":"20190427202123.15380-2-santiago@nyu.edu","threadId":"51003","inReplyTo":"20190427202123.15380-1-santiago@nyu.edu","subject":"[PATCH 1/2] builtin/tag: do not omit -v gpg out for --format","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2019-04-27T20:21:22Z","receivedAt":"2019-04-27T20:21:44Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"From: Santiago Torres <santiago@nyu.edu>\n\nThe current implementation of git tag -v omits the gpg output when the\n--format flag is passed. This may not be useful to users that want to\nsee the gpg output *and* --format the output of the git tag -v. Instead,\npass the default gpg interface output if --format is specified.\n\nSigned-off-by: Santiago Torres <santiago@nyu.edu>\n---\n builtin/tag.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 02f6bd1279..449d91c13c 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -110,10 +110,10 @@ static int verify_tag(const char *name, const char *ref,\n {\n \tint flags;\n \tconst struct ref_format *format = cb_data;\n-\tflags = GPG_VERIFY_VERBOSE;\n+\tflags = 0;\n \n-\tif (format->format)\n-\t\tflags = GPG_VERIFY_OMIT_STATUS;\n+\tif (!format->format)\n+\t\tflags = GPG_VERIFY_VERBOSE;\n \n \tif (gpg_verify_tag(oid, name, flags))\n \t\treturn -1;\n-- \n2.21.0\n\n"},{"id":"374608","messageId":"20190427202123.15380-3-santiago@nyu.edu","threadId":"51003","inReplyTo":"20190427202123.15380-1-santiago@nyu.edu","subject":"[PATCH 2/2] builtin/verify-tag: do not omit gpg on --format","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2019-04-27T20:21:23Z","receivedAt":"2019-04-27T20:21:47Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"From: Santiago Torres <santiago@nyu.edu>\n\nThe current implementation of git-verify-tag omits the gpg output when\nthe --format flag is passed. This may not be useful to users that want\nto see the gpg output *and* --format the output of git verify-tag.\nInstead, respect the --raw flag or the default gpg output.\n\nSigned-off-by: Santiago Torres <santiago@nyu.edu>\n---\n builtin/verify-tag.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\nindex 6fa04b751a..262e73cb45 100644\n--- a/builtin/verify-tag.c\n+++ b/builtin/verify-tag.c\n@@ -47,15 +47,13 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)\n \tif (argc <= i)\n \t\tusage_with_options(verify_tag_usage, verify_tag_options);\n \n-\tif (verbose)\n+\tif (verbose && !format.format)\n \t\tflags |= GPG_VERIFY_VERBOSE;\n \n-\tif (format.format) {\n+\tif (format.format)\n \t\tif (verify_ref_format(&format))\n \t\t\tusage_with_options(verify_tag_usage,\n \t\t\t\t\t   verify_tag_options);\n-\t\tflags |= GPG_VERIFY_OMIT_STATUS;\n-\t}\n \n \twhile (i < argc) {\n \t\tstruct object_id oid;\n-- \n2.21.0\n\n"},{"id":"375205","messageId":"20190509073644.GA24493@sigill.intra.peff.net","threadId":"51003","inReplyTo":"20190427202123.15380-2-santiago@nyu.edu","subject":"Re: [PATCH 1/2] builtin/tag: do not omit -v gpg out for --format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-09T07:36:44Z","receivedAt":"2019-05-09T07:36:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 27, 2019 at 04:21:22PM -0400, santiago@nyu.edu wrote:\n\n> From: Santiago Torres <santiago@nyu.edu>\n> \n> The current implementation of git tag -v omits the gpg output when the\n> --format flag is passed. This may not be useful to users that want to\n> see the gpg output *and* --format the output of the git tag -v. Instead,\n> pass the default gpg interface output if --format is specified.\n\nYeah, I think this is the right thing to do.\n\n> @@ -110,10 +110,10 @@ static int verify_tag(const char *name, const char *ref,\n>  {\n>  \tint flags;\n>  \tconst struct ref_format *format = cb_data;\n> -\tflags = GPG_VERIFY_VERBOSE;\n> +\tflags = 0;\n>  \n> -\tif (format->format)\n> -\t\tflags = GPG_VERIFY_OMIT_STATUS;\n> +\tif (!format->format)\n> +\t\tflags = GPG_VERIFY_VERBOSE;\n\nSo we're going to stop setting OMIT_STATUS ever, which makes sense.\n\nIt took me a minute to figure out here that the behavior for VERBOSE is\nnot changed, because we _overwrite_ flags, rather than just setting a\nsingle bit. But that's definitely the right thing to do when there's a\nformat (both before and after your patch).\n\nSo this looks good to me. I think we should probably cover it with a\ntest in t7004.\n\n-Peff\n"},{"id":"375206","messageId":"20190509074455.GB24493@sigill.intra.peff.net","threadId":"51003","inReplyTo":"20190427202123.15380-3-santiago@nyu.edu","subject":"Re: [PATCH 2/2] builtin/verify-tag: do not omit gpg on --format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-09T07:44:55Z","receivedAt":"2019-05-09T07:44:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 27, 2019 at 04:21:23PM -0400, santiago@nyu.edu wrote:\n\n> From: Santiago Torres <santiago@nyu.edu>\n> \n> The current implementation of git-verify-tag omits the gpg output when\n> the --format flag is passed. This may not be useful to users that want\n> to see the gpg output *and* --format the output of git verify-tag.\n> Instead, respect the --raw flag or the default gpg output.\n\nYep, this is just the matching change to patch 1. Makes sense.\n\n> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\n> index 6fa04b751a..262e73cb45 100644\n> --- a/builtin/verify-tag.c\n> +++ b/builtin/verify-tag.c\n> @@ -47,15 +47,13 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)\n>  \tif (argc <= i)\n>  \t\tusage_with_options(verify_tag_usage, verify_tag_options);\n>  \n> -\tif (verbose)\n> +\tif (verbose && !format.format)\n>  \t\tflags |= GPG_VERIFY_VERBOSE;\n\nNow this one's VERBOSE handling is a bit interesting. Previously we'd\nset VERBOSE even if we were going to show a format.  And then later we\njust set the OMIT_STATUS bit, leaving VERBOSE in place:\n\n> -\t\tflags |= GPG_VERIFY_OMIT_STATUS;\n\nThat _usually_ didn't matter because with OMIT_STATUS, we'd never enter\nprint_signature_buffer(), which is where VERBOSE would usually kick in.\nBut there's another spot we look at it:\n\n  $ grep -nC2 VERBOSE tag.c \n  22-\n  23-\tif (size == payload_size) {\n  24:\t\tif (flags & GPG_VERIFY_VERBOSE)\n  25-\t\t\twrite_in_full(1, buf, payload_size);\n  26-\t\treturn error(\"no signature found\");\n\nSo the code prior to your patch actually had another weird behavior. Try\nthis:\n\n  $ git verify-tag -v --format='my tag is %(tag)' v2.21.0\n  my tag is v2.21.0\n\n  $ git tag -m bar foo\n  $ git verify-tag -v --format='my tag is %(tag)' foo\n  object 66395b630f8ca08705b36c359415af8b25da9a11\n  type commit\n  tag foo\n  tagger Jeff King <peff@peff.net> 1557387618 -0400\n  \n  bar\n  error: no signature found\n\nThe \"-v\" only kicks in when there's an error. I think what your patch is\ndoing (consistently ignoring \"-v\" when there's a format) makes more\nsense. It may be worth alerting the user when \"-v\" and \"--format\" are\nused together (or arguably we should _always_ show \"-v\" if the user\nreally asked for it, but it does not make any sense to me for somebody\nto do so).\n\n> -\tif (format.format) {\n> +\tif (format.format)\n>  \t\tif (verify_ref_format(&format))\n>  \t\t\tusage_with_options(verify_tag_usage,\n>  \t\t\t\t\t   verify_tag_options);\n> -\t}\n\nThis leaves us with a weird doubled conditional (with no braces\neither!). Maybe:\n\n  if (format.format && verify_ref_format(&format))\n\tusage_with_options(...);\n\n?\n\nOther than that, the patch looks good. I think it could use a test in\nt7030, though.\n\n-Peff\n"},{"id":"375244","messageId":"20190509173639.4dghpurz72vhxzzt@LykOS.localdomain","threadId":"51003","inReplyTo":"20190509073644.GA24493@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] builtin/tag: do not omit -v gpg out for --format","fromName":"Santiago Torres Arias","fromEmail":"santiago@nyu.edu","sentAt":"2019-05-09T17:36:40Z","receivedAt":"2019-05-09T17:36:45Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"> So we're going to stop setting OMIT_STATUS ever, which makes sense.\n> \n> It took me a minute to figure out here that the behavior for VERBOSE is\n> not changed, because we _overwrite_ flags, rather than just setting a\n> single bit. But that's definitely the right thing to do when there's a\n> format (both before and after your patch).\n> \n> So this looks good to me. I think we should probably cover it with a\n> test in t7004.\n\nYes, that's something that surprised me originally, as these changes\ndon't make the test suite break in any way...\n\nI'll add a patch to the series with a test for this.\n\nThanks for the review!\n-Santiago.\n"},{"id":"375245","messageId":"20190509174016.us3pvef3bevfe2lb@LykOS.localdomain","threadId":"51003","inReplyTo":"20190509074455.GB24493@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] builtin/verify-tag: do not omit gpg on --format","fromName":"Santiago Torres Arias","fromEmail":"santiago@nyu.edu","sentAt":"2019-05-09T17:40:17Z","receivedAt":"2019-05-09T17:40:22Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"> Now this one's VERBOSE handling is a bit interesting. Previously we'd\n> set VERBOSE even if we were going to show a format.  And then later we\n> just set the OMIT_STATUS bit, leaving VERBOSE in place:\n> \n> > -\t\tflags |= GPG_VERIFY_OMIT_STATUS;\n> \n> That _usually_ didn't matter because with OMIT_STATUS, we'd never enter\n> print_signature_buffer(), which is where VERBOSE would usually kick in.\n> But there's another spot we look at it:\n> \n>   $ grep -nC2 VERBOSE tag.c \n>   22-\n>   23-\tif (size == payload_size) {\n>   24:\t\tif (flags & GPG_VERIFY_VERBOSE)\n>   25-\t\t\twrite_in_full(1, buf, payload_size);\n>   26-\t\treturn error(\"no signature found\");\n> \n> So the code prior to your patch actually had another weird behavior. Try\n> this:\n> \n>   $ git verify-tag -v --format='my tag is %(tag)' v2.21.0\n>   my tag is v2.21.0\n> \n>   $ git tag -m bar foo\n>   $ git verify-tag -v --format='my tag is %(tag)' foo\n>   object 66395b630f8ca08705b36c359415af8b25da9a11\n>   type commit\n>   tag foo\n>   tagger Jeff King <peff@peff.net> 1557387618 -0400\n>   \n>   bar\n>   error: no signature found\n> \n> The \"-v\" only kicks in when there's an error. I think what your patch is\n> doing (consistently ignoring \"-v\" when there's a format) makes more\n> sense. It may be worth alerting the user when \"-v\" and \"--format\" are\n> used together (or arguably we should _always_ show \"-v\" if the user\n> really asked for it, but it does not make any sense to me for somebody\n> to do so).\n\nAha! I completely missed this but it is indeed weird. Something\nsimilar happened to me when I was sketching some patches for tag\nverification in a downstream project...\n\n> > -\tif (format.format) {\n> > +\tif (format.format)\n> >  \t\tif (verify_ref_format(&format))\n> >  \t\t\tusage_with_options(verify_tag_usage,\n> >  \t\t\t\t\t   verify_tag_options);\n> > -\t}\n> \n> This leaves us with a weird doubled conditional (with no braces\n> either!). Maybe:\n> \n>   if (format.format && verify_ref_format(&format))\n> \tusage_with_options(...);\n> \n> ?\n\nYes, I think chaining this if here is cleaner/less error prone.\n\n> \n> Other than that, the patch looks good. I think it could use a test in\n> t7030, though.\n\nLet me make a re-roll with these changes included and a test suite for\nboth t7030 or t7004.\n\nThanks!\n-Santiago.\n"}]}