Re: [PATCH v5 3/7] tag: add format specifier to gpg_verify_tag
- From
Jeff King <peff@peff.net>
- Date
- Jan 17, 2017, 17:25 UTC
- Message-ID
- <20170117172531.bahjekbj3om43gtq@sigill.intra.peff.net>
- In-Reply-To
- <20170117165724.2hbyfdzrhrmro54b@LykOS.localdomain>
On Tue, Jan 17, 2017 at 11:57:25AM -0500, Santiago Torres wrote:
Show 13 quoted lines
> > Having read through the rest of the series, it looks like you'd > > sometimes have to do: > > IIRC, we did this to make the diff "simpler". It also feeds odd that the > call chain is the following: > > verify_and_format_tag() > gpg_verify_tag() > run_gpg_verification() > > I'm afraid that adding yet another wrapper would further convolute the > call chain. If you think this is not an issue, I could easily do it. Do > you have any suggested name for the wrapper?
Actually, looking at the callsites, I think they are fine to just call pretty_print_ref() themselves, and I don't think it actually matters if it happens before or after the verification.
So I think you could just throw out patch 3 entirely and squash these hunks into patches 4 and 5:
diff --git a/builtin/tag.c b/builtin/tag.c index 9da11e0c2..fab9fa8f9 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -111,10 +111,12 @@ static int verify_tag(const char *name, const char *ref, char *fmt_pretty = cb_data; flags = GPG_VERIFY_VERBOSE; - if (fmt_pretty) + if (fmt_pretty) { flags = GPG_VERIFY_QUIET; + pretty_print_ref(name, sha1, fmt_pretty); + } - return verify_and_format_tag(sha1, ref, fmt_pretty, flags); + return gpg_verify_tag(sha1, ref, flags); } static int do_sign(struct strbuf *buffer) diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c index 212449f47..114df1c52 100644 --- a/builtin/verify-tag.c +++ b/builtin/verify-tag.c @@ -58,9 +58,15 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix) while (i < argc) { unsigned char sha1[20]; const char *name = argv[i++]; - if (get_sha1(name, sha1)) + + if (get_sha1(name, sha1)) { had_error = !!error("tag '%s' not found.", name); - else if (verify_and_format_tag(sha1, name, fmt_pretty, flags)) + continue; + } + + if (fmt_pretty) + pretty_print_ref(name, sha1, fmt_pretty); + if (gpg_verify_tag(sha1, name, flags)) had_error = 1; } return had_error; You could make the diff in the second one simpler by skipping the "continue" and just doing the whole thing in an "else" block. But IMHO the continue-on-error makes the logic more clear. -Peff