On Tue, Jan 17, 2017 at 11:57:25AM -0500, Santiago Torres wrote:
> > 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