[PATCH 0/2] tag verification: do not mute gpg output

[PATCH 0/2] tag verification: do not mute gpg output

[santiago]

From: Santiago Torres <santiago@nyu.edu>

The default behavior of the tag verification functions used to quiet
down the gpg output if --format was passed. The rationale for this was
to avoid --format to be litterred by the gpg output. However, this may
be unnecessary because the gpg output is already streamed to stderr and
thus can be easily multiplexed. 

Santiago Torres (2):
  builtin/tag: do not omit -v gpg out for --format
  builtin/verify-tag: do not omit gpg on --format

 builtin/tag.c        | 6 +++---
 builtin/verify-tag.c | 6 ++----
 2 files changed, 5 insertions(+), 7 deletions(-)

-- 
2.21.0

[PATCH 1/2] builtin/tag: do not omit -v gpg out for --format

[santiago]

From: Santiago Torres <santiago@nyu.edu>

The current implementation of git tag -v omits the gpg output when the
--format flag is passed. This may not be useful to users that want to
see the gpg output *and* --format the output of the git tag -v. Instead,
pass the default gpg interface output if --format is specified.

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

diff --git a/builtin/tag.c b/builtin/tag.c
index 02f6bd1279..449d91c13c 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -110,10 +110,10 @@ static int verify_tag(const char *name, const char *ref,
 {
 	int flags;
 	const struct ref_format *format = cb_data;
-	flags = GPG_VERIFY_VERBOSE;
+	flags = 0;
 
-	if (format->format)
-		flags = GPG_VERIFY_OMIT_STATUS;
+	if (!format->format)
+		flags = GPG_VERIFY_VERBOSE;
 
 	if (gpg_verify_tag(oid, name, flags))
 		return -1;
-- 
2.21.0

[PATCH 2/2] builtin/verify-tag: do not omit gpg on --format

[santiago]

From: Santiago Torres <santiago@nyu.edu>

The current implementation of git-verify-tag omits the gpg output when
the --format flag is passed. This may not be useful to users that want
to see the gpg output *and* --format the output of git verify-tag.
Instead, respect the --raw flag or the default gpg output.

Signed-off-by: Santiago Torres <santiago@nyu.edu>
---
 builtin/verify-tag.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 6fa04b751a..262e73cb45 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -47,15 +47,13 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 	if (argc <= i)
 		usage_with_options(verify_tag_usage, verify_tag_options);
 
-	if (verbose)
+	if (verbose && !format.format)
 		flags |= GPG_VERIFY_VERBOSE;
 
-	if (format.format) {
+	if (format.format)
 		if (verify_ref_format(&format))
 			usage_with_options(verify_tag_usage,
 					   verify_tag_options);
-		flags |= GPG_VERIFY_OMIT_STATUS;
-	}
 
 	while (i < argc) {
 		struct object_id oid;
-- 
2.21.0

Re: [PATCH 1/2] builtin/tag: do not omit -v gpg out for --format

[Jeff King]

On Sat, Apr 27, 2019 at 04:21:22PM -0400, santiago@nyu.edu wrote:

> From: Santiago Torres <santiago@nyu.edu>
> 
> The current implementation of git tag -v omits the gpg output when the
> --format flag is passed. This may not be useful to users that want to
> see the gpg output *and* --format the output of the git tag -v. Instead,
> pass the default gpg interface output if --format is specified.

Yeah, I think this is the right thing to do.

> @@ -110,10 +110,10 @@ static int verify_tag(const char *name, const char *ref,
>  {
>  	int flags;
>  	const struct ref_format *format = cb_data;
> -	flags = GPG_VERIFY_VERBOSE;
> +	flags = 0;
>  
> -	if (format->format)
> -		flags = GPG_VERIFY_OMIT_STATUS;
> +	if (!format->format)
> +		flags = GPG_VERIFY_VERBOSE;

So we're going to stop setting OMIT_STATUS ever, which makes sense.

It took me a minute to figure out here that the behavior for VERBOSE is
not changed, because we _overwrite_ flags, rather than just setting a
single bit. But that's definitely the right thing to do when there's a
format (both before and after your patch).

So this looks good to me. I think we should probably cover it with a
test in t7004.

-Peff

Re: [PATCH 2/2] builtin/verify-tag: do not omit gpg on --format

[Jeff King]

On Sat, Apr 27, 2019 at 04:21:23PM -0400, santiago@nyu.edu wrote:

> From: Santiago Torres <santiago@nyu.edu>
> 
> The current implementation of git-verify-tag omits the gpg output when
> the --format flag is passed. This may not be useful to users that want
> to see the gpg output *and* --format the output of git verify-tag.
> Instead, respect the --raw flag or the default gpg output.

Yep, this is just the matching change to patch 1. Makes sense.

> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> index 6fa04b751a..262e73cb45 100644
> --- a/builtin/verify-tag.c
> +++ b/builtin/verify-tag.c
> @@ -47,15 +47,13 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
>  	if (argc <= i)
>  		usage_with_options(verify_tag_usage, verify_tag_options);
>  
> -	if (verbose)
> +	if (verbose && !format.format)
>  		flags |= GPG_VERIFY_VERBOSE;

Now this one's VERBOSE handling is a bit interesting. Previously we'd
set VERBOSE even if we were going to show a format.  And then later we
just set the OMIT_STATUS bit, leaving VERBOSE in place:

> -		flags |= GPG_VERIFY_OMIT_STATUS;

That _usually_ didn't matter because with OMIT_STATUS, we'd never enter
print_signature_buffer(), which is where VERBOSE would usually kick in.
But there's another spot we look at it:

  $ grep -nC2 VERBOSE tag.c 
  22-
  23-	if (size == payload_size) {
  24:		if (flags & GPG_VERIFY_VERBOSE)
  25-			write_in_full(1, buf, payload_size);
  26-		return error("no signature found");

So the code prior to your patch actually had another weird behavior. Try
this:

  $ git verify-tag -v --format='my tag is %(tag)' v2.21.0
  my tag is v2.21.0

  $ git tag -m bar foo
  $ git verify-tag -v --format='my tag is %(tag)' foo
  object 66395b630f8ca08705b36c359415af8b25da9a11
  type commit
  tag foo
  tagger Jeff King <peff@peff.net> 1557387618 -0400
  
  bar
  error: no signature found

The "-v" only kicks in when there's an error. I think what your patch is
doing (consistently ignoring "-v" when there's a format) makes more
sense. It may be worth alerting the user when "-v" and "--format" are
used together (or arguably we should _always_ show "-v" if the user
really asked for it, but it does not make any sense to me for somebody
to do so).

> -	if (format.format) {
> +	if (format.format)
>  		if (verify_ref_format(&format))
>  			usage_with_options(verify_tag_usage,
>  					   verify_tag_options);
> -	}

This leaves us with a weird doubled conditional (with no braces
either!). Maybe:

  if (format.format && verify_ref_format(&format))
	usage_with_options(...);

?

Other than that, the patch looks good. I think it could use a test in
t7030, though.

-Peff

Re: [PATCH 1/2] builtin/tag: do not omit -v gpg out for --format

[Santiago Torres Arias]

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

> So we're going to stop setting OMIT_STATUS ever, which makes sense.
> 
> It took me a minute to figure out here that the behavior for VERBOSE is
> not changed, because we _overwrite_ flags, rather than just setting a
> single bit. But that's definitely the right thing to do when there's a
> format (both before and after your patch).
> 
> So this looks good to me. I think we should probably cover it with a
> test in t7004.

Yes, that's something that surprised me originally, as these changes
don't make the test suite break in any way...

I'll add a patch to the series with a test for this.

Thanks for the review!
-Santiago.

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

Re: [PATCH 2/2] builtin/verify-tag: do not omit gpg on --format

[Santiago Torres Arias]

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

> Now this one's VERBOSE handling is a bit interesting. Previously we'd
> set VERBOSE even if we were going to show a format.  And then later we
> just set the OMIT_STATUS bit, leaving VERBOSE in place:
> 
> > -		flags |= GPG_VERIFY_OMIT_STATUS;
> 
> That _usually_ didn't matter because with OMIT_STATUS, we'd never enter
> print_signature_buffer(), which is where VERBOSE would usually kick in.
> But there's another spot we look at it:
> 
>   $ grep -nC2 VERBOSE tag.c 
>   22-
>   23-	if (size == payload_size) {
>   24:		if (flags & GPG_VERIFY_VERBOSE)
>   25-			write_in_full(1, buf, payload_size);
>   26-		return error("no signature found");
> 
> So the code prior to your patch actually had another weird behavior. Try
> this:
> 
>   $ git verify-tag -v --format='my tag is %(tag)' v2.21.0
>   my tag is v2.21.0
> 
>   $ git tag -m bar foo
>   $ git verify-tag -v --format='my tag is %(tag)' foo
>   object 66395b630f8ca08705b36c359415af8b25da9a11
>   type commit
>   tag foo
>   tagger Jeff King <peff@peff.net> 1557387618 -0400
>   
>   bar
>   error: no signature found
> 
> The "-v" only kicks in when there's an error. I think what your patch is
> doing (consistently ignoring "-v" when there's a format) makes more
> sense. It may be worth alerting the user when "-v" and "--format" are
> used together (or arguably we should _always_ show "-v" if the user
> really asked for it, but it does not make any sense to me for somebody
> to do so).

Aha! I completely missed this but it is indeed weird. Something
similar happened to me when I was sketching some patches for tag
verification in a downstream project...

> > -	if (format.format) {
> > +	if (format.format)
> >  		if (verify_ref_format(&format))
> >  			usage_with_options(verify_tag_usage,
> >  					   verify_tag_options);
> > -	}
> 
> This leaves us with a weird doubled conditional (with no braces
> either!). Maybe:
> 
>   if (format.format && verify_ref_format(&format))
> 	usage_with_options(...);
> 
> ?

Yes, I think chaining this if here is cleaner/less error prone.

> 
> Other than that, the patch looks good. I think it could use a test in
> t7030, though.

Let me make a re-roll with these changes included and a test suite for
both t7030 or t7004.

Thanks!
-Santiago.

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