* [PATCH 0/2] tag verification: do not mute gpg output @ 2019-04-27 20:21 santiago 2019-04-27 20:21 ` [PATCH 1/2] builtin/tag: do not omit -v gpg out for --format santiago 2019-04-27 20:21 ` [PATCH 2/2] builtin/verify-tag: do not omit gpg on --format santiago 0 siblings, 2 replies; 7+ messages in thread From: santiago @ 2019-04-27 20:21 UTC (permalink / raw) To: git; +Cc: gitster, peff, sunshine, walters, Santiago Torres 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] builtin/tag: do not omit -v gpg out for --format 2019-04-27 20:21 [PATCH 0/2] tag verification: do not mute gpg output santiago @ 2019-04-27 20:21 ` santiago 2019-05-09 7:36 ` Jeff King 2019-04-27 20:21 ` [PATCH 2/2] builtin/verify-tag: do not omit gpg on --format santiago 1 sibling, 1 reply; 7+ messages in thread From: santiago @ 2019-04-27 20:21 UTC (permalink / raw) To: git; +Cc: gitster, peff, sunshine, walters, Santiago Torres 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] builtin/tag: do not omit -v gpg out for --format 2019-04-27 20:21 ` [PATCH 1/2] builtin/tag: do not omit -v gpg out for --format santiago @ 2019-05-09 7:36 ` Jeff King 2019-05-09 17:36 ` Santiago Torres Arias 0 siblings, 1 reply; 7+ messages in thread From: Jeff King @ 2019-05-09 7:36 UTC (permalink / raw) To: santiago; +Cc: git, gitster, sunshine, walters 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] builtin/tag: do not omit -v gpg out for --format 2019-05-09 7:36 ` Jeff King @ 2019-05-09 17:36 ` Santiago Torres Arias 0 siblings, 0 replies; 7+ messages in thread From: Santiago Torres Arias @ 2019-05-09 17:36 UTC (permalink / raw) To: Jeff King; +Cc: git, gitster, sunshine, walters [-- 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] builtin/verify-tag: do not omit gpg on --format 2019-04-27 20:21 [PATCH 0/2] tag verification: do not mute gpg output santiago 2019-04-27 20:21 ` [PATCH 1/2] builtin/tag: do not omit -v gpg out for --format santiago @ 2019-04-27 20:21 ` santiago 2019-05-09 7:44 ` Jeff King 1 sibling, 1 reply; 7+ messages in thread From: santiago @ 2019-04-27 20:21 UTC (permalink / raw) To: git; +Cc: gitster, peff, sunshine, walters, Santiago Torres 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] builtin/verify-tag: do not omit gpg on --format 2019-04-27 20:21 ` [PATCH 2/2] builtin/verify-tag: do not omit gpg on --format santiago @ 2019-05-09 7:44 ` Jeff King 2019-05-09 17:40 ` Santiago Torres Arias 0 siblings, 1 reply; 7+ messages in thread From: Jeff King @ 2019-05-09 7:44 UTC (permalink / raw) To: santiago; +Cc: git, gitster, sunshine, walters 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] builtin/verify-tag: do not omit gpg on --format 2019-05-09 7:44 ` Jeff King @ 2019-05-09 17:40 ` Santiago Torres Arias 0 siblings, 0 replies; 7+ messages in thread From: Santiago Torres Arias @ 2019-05-09 17:40 UTC (permalink / raw) To: Jeff King; +Cc: git, gitster, sunshine, walters [-- 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-05-09 17:40 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-27 20:21 [PATCH 0/2] tag verification: do not mute gpg output santiago 2019-04-27 20:21 ` [PATCH 1/2] builtin/tag: do not omit -v gpg out for --format santiago 2019-05-09 7:36 ` Jeff King 2019-05-09 17:36 ` Santiago Torres Arias 2019-04-27 20:21 ` [PATCH 2/2] builtin/verify-tag: do not omit gpg on --format santiago 2019-05-09 7:44 ` Jeff King 2019-05-09 17:40 ` Santiago Torres Arias
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).