diff mbox series

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

Message ID 20190427202123.15380-2-santiago@nyu.edu (mailing list archive)
State New, archived
Headers show
Series tag verification: do not mute gpg output | expand

Commit Message

Santiago Torres Arias April 27, 2019, 8:21 p.m. UTC 
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(-)

Comments

Jeff King May 9, 2019, 7:36 a.m. UTC | #1 
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
Santiago Torres Arias May 9, 2019, 5:36 p.m. UTC | #2 
> 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.
diff mbox series

Patch

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;