Re: [PATCH v5 3/7] tag: add format specifier to gpg_verify_tag

[Santiago Torres]

Yeah, this actually looks more cleaner.

Let me give it a go.

Thanks!
-Santiago.

On Tue, Jan 17, 2017 at 12:30:04PM -0500, Jeff King wrote:
> On Tue, Jan 17, 2017 at 12:25:31PM -0500, Jeff King wrote:
> 
> > 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.
> 
> Oh, sorry, I misread it. We do indeed early-return from the verification
> and skip the printing in that case. So it'd be more like:
> 
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 9da11e0c2..068f392b6 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -114,7 +114,11 @@ static int verify_tag(const char *name, const char *ref,
>   if (fmt_pretty)
>   flags = GPG_VERIFY_QUIET;
>  
> - return verify_and_format_tag(sha1, ref, fmt_pretty, flags);
> + if (gpg_verify_tag(sha1, ref, flags))
> + return -1;
> +
> + pretty_print_ref(name, sha1, fmt_pretty);
> + return 0;
>  }
>  
>  static int do_sign(struct strbuf *buffer)
> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> index 212449f47..b3f08f705 100644
> --- a/builtin/verify-tag.c
> +++ b/builtin/verify-tag.c
> @@ -58,10 +58,19 @@ 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 (gpg_verify_tag(sha1, name, flags)) {
>   had_error = 1;
> + continue;
> + }
> +
> + if (fmt_pretty)
> + pretty_print_ref(name, sha1, fmt_pretty);
>   }
>   return had_error;
>  }
> 
> which I think is still an improvement (the printing, rather than being
> an obscure parameter to the overloaded verify_and_format_tag()
> function, is clearly a first-class operation).
> 
> -Peff


signature.asc
Description: PGP signature

Re: [PATCH v5 3/7] tag: add format specifier to gpg_verify_tag

[Jeff King]

On Tue, Jan 17, 2017 at 12:33:46PM -0500, Santiago Torres wrote:

> Yeah, this actually looks more cleaner.
> 
> Let me give it a go.

Neat. Note there's a bug:

> > diff --git a/builtin/tag.c b/builtin/tag.c
> > index 9da11e0c2..068f392b6 100644
> > --- a/builtin/tag.c
> > +++ b/builtin/tag.c
> > @@ -114,7 +114,11 @@ static int verify_tag(const char *name, const char 
> > *ref,
> > if (fmt_pretty)
> > flags = GPG_VERIFY_QUIET;
> >  
> > -   return verify_and_format_tag(sha1, ref, fmt_pretty, flags);
> > +   if (gpg_verify_tag(sha1, ref, flags))
> > +   return -1;
> > +
> > +   pretty_print_ref(name, sha1, fmt_pretty);
> > +   return 0;

The final print should be guarded by "if (fmt_pretty)".

-Peff

Re: [PATCH v5 3/7] tag: add format specifier to gpg_verify_tag

[Jeff King]

On Tue, Jan 17, 2017 at 12:25:31PM -0500, Jeff King wrote:

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

Oh, sorry, I misread it. We do indeed early-return from the verification
and skip the printing in that case. So it'd be more like:

diff --git a/builtin/tag.c b/builtin/tag.c
index 9da11e0c2..068f392b6 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -114,7 +114,11 @@ static int verify_tag(const char *name, const char *ref,
if (fmt_pretty)
flags = GPG_VERIFY_QUIET;
 
-   return verify_and_format_tag(sha1, ref, fmt_pretty, flags);
+   if (gpg_verify_tag(sha1, ref, flags))
+   return -1;
+
+   pretty_print_ref(name, sha1, fmt_pretty);
+   return 0;
 }
 
 static int do_sign(struct strbuf *buffer)
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 212449f47..b3f08f705 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -58,10 +58,19 @@ 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 (gpg_verify_tag(sha1, name, flags)) {
had_error = 1;
+   continue;
+   }
+
+   if (fmt_pretty)
+   pretty_print_ref(name, sha1, fmt_pretty);
}
return had_error;
 }

which I think is still an improvement (the printing, rather than being
an obscure parameter to the overloaded verify_and_format_tag()
function, is clearly a first-class operation).

-Peff

Re: [PATCH v5 3/7] tag: add format specifier to gpg_verify_tag

[Jeff King]

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

Re: [PATCH v5 3/7] tag: add format specifier to gpg_verify_tag

[Santiago Torres]

> > Hrm. Maybe I am missing something, but what does:
> > 
> >   verify_and_format_tag(sha1, name, fmt, flags);
> > 
> > get you over:
> > 
> >   gpg_verify_tag(sha1, name, flags);
> >   pretty_print_ref(name, sha1, fmt);
> > 
> > ? The latter seems much more flexible, and I do not see how the
> > verification step impacts the printing at all (or vice versa).
> > 
> > I could understand it more if there were patches later in the series
> > that somehow used the format and verification results together. But I
> > didn't see that.
> 
> 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?

Thanks!
-Santiago


signature.asc
Description: PGP signature

Re: [PATCH v5 3/7] tag: add format specifier to gpg_verify_tag

[Jeff King]

On Tue, Jan 17, 2017 at 10:24:55AM -0500, Jeff King wrote:

> On Sun, Jan 15, 2017 at 01:47:01PM -0500, santi...@nyu.edu wrote:
> 
> > From: Lukas Puehringer 
> > 
> > Calling functions for gpg_verify_tag() may desire to print relevant
> > information about the header for further verification. Add an optional
> > format argument to print any desired information after GPG verification.
> 
> Hrm. Maybe I am missing something, but what does:
> 
>   verify_and_format_tag(sha1, name, fmt, flags);
> 
> get you over:
> 
>   gpg_verify_tag(sha1, name, flags);
>   pretty_print_ref(name, sha1, fmt);
> 
> ? The latter seems much more flexible, and I do not see how the
> verification step impacts the printing at all (or vice versa).
> 
> I could understand it more if there were patches later in the series
> that somehow used the format and verification results together. But I
> didn't see that.

Having read through the rest of the series, it looks like you'd
sometimes have to do:

  int ret;

  ret = gpg_verify_tag(sha1, name, flags);
  pretty_print_ref(name, sha1, fmt);
  if (ret)
  ... do something ...

and this function lets you do it in a single line.

Still, I think I'd rather see it done as a wrapper than modifying
gpg_verify_tag().

-Peff

Re: [PATCH v5 3/7] tag: add format specifier to gpg_verify_tag

[Jeff King]

On Sun, Jan 15, 2017 at 01:47:01PM -0500, santi...@nyu.edu wrote:

> From: Lukas Puehringer 
> 
> Calling functions for gpg_verify_tag() may desire to print relevant
> information about the header for further verification. Add an optional
> format argument to print any desired information after GPG verification.

Hrm. Maybe I am missing something, but what does:

  verify_and_format_tag(sha1, name, fmt, flags);

get you over:

  gpg_verify_tag(sha1, name, flags);
  pretty_print_ref(name, sha1, fmt);

? The latter seems much more flexible, and I do not see how the
verification step impacts the printing at all (or vice versa).

I could understand it more if there were patches later in the series
that somehow used the format and verification results together. But I
didn't see that.

-Peff

[PATCH v5 3/7] tag: add format specifier to gpg_verify_tag

[santiago]

From: Lukas Puehringer 

Calling functions for gpg_verify_tag() may desire to print relevant
information about the header for further verification. Add an optional
format argument to print any desired information after GPG verification.

Signed-off-by: Lukas Puehringer 
---
 builtin/tag.c|  2 +-
 builtin/verify-tag.c |  2 +-
 tag.c| 17 +++--
 tag.h|  4 ++--
 4 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 73df72811..880677df5 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -105,7 +105,7 @@ static int delete_tag(const char *name, const char *ref,
 static int verify_tag(const char *name, const char *ref,
const unsigned char *sha1)
 {
-   return gpg_verify_tag(sha1, name, GPG_VERIFY_VERBOSE);
+   return verify_and_format_tag(sha1, name, NULL, GPG_VERIFY_VERBOSE);
 }
 
 static int do_sign(struct strbuf *buffer)
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 99f8148cf..de10198c4 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -51,7 +51,7 @@ int cmd_verify_tag(int argc, const char **argv, const char 
*prefix)
const char *name = argv[i++];
if (get_sha1(name, sha1))
had_error = !!error("tag '%s' not found.", name);
-   else if (gpg_verify_tag(sha1, name, flags))
+   else if (verify_and_format_tag(sha1, name, NULL, flags))
had_error = 1;
}
return had_error;
diff --git a/tag.c b/tag.c
index 291073f6e..d5a7cfb20 100644
--- a/tag.c
+++ b/tag.c
@@ -4,6 +4,7 @@
 #include "tree.h"
 #include "blob.h"
 #include "gpg-interface.h"
+#include "ref-filter.h"
 
 const char *tag_type = "tag";
 
@@ -33,8 +34,8 @@ static int run_gpg_verify(const char *buf, unsigned long 
size, unsigned flags)
return ret;
 }
 
-int gpg_verify_tag(const unsigned char *sha1, const char *name_to_report,
-   unsigned flags)
+int verify_and_format_tag(const unsigned char *sha1, const char *name,
+   const char *fmt_pretty, unsigned flags)
 {
enum object_type type;
char *buf;
@@ -44,21 +45,25 @@ int gpg_verify_tag(const unsigned char *sha1, const char 
*name_to_report,
type = sha1_object_info(sha1, NULL);
if (type != OBJ_TAG)
return error("%s: cannot verify a non-tag object of type %s.",
-   name_to_report ?
-   name_to_report :
+   name ?
+   name :
find_unique_abbrev(sha1, DEFAULT_ABBREV),
typename(type));
 
buf = read_sha1_file(sha1, , );
if (!buf)
return error("%s: unable to read file.",
-   name_to_report ?
-   name_to_report :
+   name ?
+   name :
find_unique_abbrev(sha1, DEFAULT_ABBREV));
 
ret = run_gpg_verify(buf, size, flags);
 
free(buf);
+
+   if (fmt_pretty)
+   pretty_print_ref(name, sha1, fmt_pretty);
+
return ret;
 }
 
diff --git a/tag.h b/tag.h
index a5721b673..896b9c2dc 100644
--- a/tag.h
+++ b/tag.h
@@ -17,7 +17,7 @@ extern int parse_tag_buffer(struct tag *item, const void 
*data, unsigned long si
 extern int parse_tag(struct tag *item);
 extern struct object *deref_tag(struct object *, const char *, int);
 extern struct object *deref_tag_noverify(struct object *);
-extern int gpg_verify_tag(const unsigned char *sha1,
-   const char *name_to_report, unsigned flags);
+extern int verify_and_format_tag(const unsigned char *sha1, const char *name,
+   const char *fmt_pretty, unsigned flags);
 
 #endif /* TAG_H */
-- 
2.11.0