patch, 2 partstag verification: do not mute gpg output

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(-)

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(-)

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

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

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

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.

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(-)

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

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

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:

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).

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

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...

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

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

Thanks! -Santiago.