Re: [PATCH v5 5/7] builtin/tag: add --format argument for tag -v

[Santiago Torres]

> VERBOSE|QUIET _does_ have a meaning, which is "show the payload, but do
> not print the signature buffer". Perhaps just renaming QUIET to
> OMIT_STATUS or something would make it more clear.
> 
Let me give this a go too. OMIT_STATUS does sound less confusing.

Thanks,
-Santiago.

> -Peff


signature.asc
Description: PGP signature

Re: [PATCH v5 5/7] builtin/tag: add --format argument for tag -v

[Jeff King]

On Tue, Jan 17, 2017 at 12:00:19PM -0500, Santiago Torres wrote:

> > >  {
> > > - return verify_and_format_tag(sha1, name, NULL, GPG_VERIFY_VERBOSE);
> > > + int flags;
> > > + char *fmt_pretty = cb_data;
> > > + flags = GPG_VERIFY_VERBOSE;
> > > +
> > > + if (fmt_pretty)
> > > + flags = GPG_VERIFY_QUIET;
> > > +
> > > + return verify_and_format_tag(sha1, ref, fmt_pretty, flags);
> > 
> > It seems funny that VERBOSE and QUIET are bit-flags. What happens when
> > you ask for GPG_VERIFY_VERBOSE|GPG_VERIFY_QUIET?
> 
> If I'm not mistaken, the way the code works right now this is not
> possible (GPG_VERIFY_VERBOSE will be unset when GPG_VERIFY_QUIET). I
> would have to re-read the patch to make sure this is the case then.

No, you're right that the two flags are mutually exclusive in the
current callers. So I don't think there's a bug here. It's just that the
interface is potentially awkward/confusing, which may be a problem down
the line.

VERBOSE|QUIET _does_ have a meaning, which is "show the payload, but do
not print the signature buffer". Perhaps just renaming QUIET to
OMIT_STATUS or something would make it more clear.

-Peff

Re: [PATCH v5 5/7] builtin/tag: add --format argument for tag -v

[Santiago Torres]

> >  {
> > -   return verify_and_format_tag(sha1, name, NULL, GPG_VERIFY_VERBOSE);
> > +   int flags;
> > +   char *fmt_pretty = cb_data;
> > +   flags = GPG_VERIFY_VERBOSE;
> > +
> > +   if (fmt_pretty)
> > +   flags = GPG_VERIFY_QUIET;
> > +
> > +   return verify_and_format_tag(sha1, ref, fmt_pretty, flags);
> 
> It seems funny that VERBOSE and QUIET are bit-flags. What happens when
> you ask for GPG_VERIFY_VERBOSE|GPG_VERIFY_QUIET?

If I'm not mistaken, the way the code works right now this is not
possible (GPG_VERIFY_VERBOSE will be unset when GPG_VERIFY_QUIET). I
would have to re-read the patch to make sure this is the case then.

GPG_VERIFY_QUIET was added to suppress any VERBOSE|RAW flags, we could
defeault to QUIET if flags are not set. What do you think?

Thanks!
-Santiago


signature.asc
Description: PGP signature

Re: [PATCH v5 5/7] builtin/tag: add --format argument for tag -v

[Jeff King]

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

> -static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
> +static int for_each_tag_name(const char **argv, each_tag_name_fn fn,
> + void *cb_data)
>  {
>   const char **p;
>   char ref[PATH_MAX];
>   int had_error = 0;
>   unsigned char sha1[20];
>  
> +
>   for (p = argv; *p; p++) {
>   if (snprintf(ref, sizeof(ref), "refs/tags/%s", *p)
>   >= sizeof(ref)) {

Funny extra line?

>  {
> - return verify_and_format_tag(sha1, name, NULL, GPG_VERIFY_VERBOSE);
> + int flags;
> + char *fmt_pretty = cb_data;
> + flags = GPG_VERIFY_VERBOSE;
> +
> + if (fmt_pretty)
> + flags = GPG_VERIFY_QUIET;
> +
> + return verify_and_format_tag(sha1, ref, fmt_pretty, flags);

It seems funny that VERBOSE and QUIET are bit-flags. What happens when
you ask for GPG_VERIFY_VERBOSE|GPG_VERIFY_QUIET?

I suppose this is actually not a problem in _this_ patch, but in the
very first one that adds the QUIET flag. I'm not sure if the problem is
that the options should be more orthogonal, or that they are just badly
named to appear as opposites when they aren't.

-Peff

[PATCH v5 5/7] builtin/tag: add --format argument for tag -v

[santiago]

From: Lukas Puehringer 

Adding --format to git tag -v mutes the default output of the GPG
verification and instead prints the formatted tag object.
This allows callers to cross-check the tagname from refs/tags with
the tagname from the tag object header upon GPG verification.

The callback function for for_each_tag_name() didn't allow callers to
pass custom data to their callback functions. Add a new opaque pointer
to each_tag_name_fn's parameter to allow this.

Signed-off-by: Lukas Puehringer 
---
 Documentation/git-tag.txt |  2 +-
 builtin/tag.c | 32 ++--
 2 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 76cfe40d9..586aaa315 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -15,7 +15,7 @@ SYNOPSIS
 'git tag' [-n[]] -l [--contains ] [--points-at ]
[--column[=] | --no-column] [--create-reflog] [--sort=]
[--format=] [--[no-]merged []] [...]
-'git tag' -v ...
+'git tag' -v [--format=] ...
 
 DESCRIPTION
 ---
diff --git a/builtin/tag.c b/builtin/tag.c
index 880677df5..9da11e0c2 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -24,7 +24,7 @@ static const char * const git_tag_usage[] = {
N_("git tag -d ..."),
N_("git tag -l [-n[]] [--contains ] [--points-at ]"
"\n\t\t[--format=] [--[no-]merged []] 
[...]"),
-   N_("git tag -v ..."),
+   N_("git tag -v [--format=] ..."),
NULL
 };
 
@@ -66,15 +66,17 @@ static int list_tags(struct ref_filter *filter, struct 
ref_sorting *sorting, con
 }
 
 typedef int (*each_tag_name_fn)(const char *name, const char *ref,
-   const unsigned char *sha1);
+   const unsigned char *sha1, void *cb_data);
 
-static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
+static int for_each_tag_name(const char **argv, each_tag_name_fn fn,
+   void *cb_data)
 {
const char **p;
char ref[PATH_MAX];
int had_error = 0;
unsigned char sha1[20];
 
+
for (p = argv; *p; p++) {
if (snprintf(ref, sizeof(ref), "refs/tags/%s", *p)
>= sizeof(ref)) {
@@ -87,14 +89,14 @@ static int for_each_tag_name(const char **argv, 
each_tag_name_fn fn)
had_error = 1;
continue;
}
-   if (fn(*p, ref, sha1))
+   if (fn(*p, ref, sha1, cb_data))
had_error = 1;
}
return had_error;
 }
 
 static int delete_tag(const char *name, const char *ref,
-   const unsigned char *sha1)
+   const unsigned char *sha1, void *cb_data)
 {
if (delete_ref(ref, sha1, 0))
return 1;
@@ -103,9 +105,16 @@ static int delete_tag(const char *name, const char *ref,
 }
 
 static int verify_tag(const char *name, const char *ref,
-   const unsigned char *sha1)
+   const unsigned char *sha1, void *cb_data)
 {
-   return verify_and_format_tag(sha1, name, NULL, GPG_VERIFY_VERBOSE);
+   int flags;
+   char *fmt_pretty = cb_data;
+   flags = GPG_VERIFY_VERBOSE;
+
+   if (fmt_pretty)
+   flags = GPG_VERIFY_QUIET;
+
+   return verify_and_format_tag(sha1, ref, fmt_pretty, flags);
 }
 
 static int do_sign(struct strbuf *buffer)
@@ -428,9 +437,12 @@ int cmd_tag(int argc, const char **argv, const char 
*prefix)
if (filter.merge_commit)
die(_("--merged and --no-merged option are only allowed with 
-l"));
if (cmdmode == 'd')
-   return for_each_tag_name(argv, delete_tag);
-   if (cmdmode == 'v')
-   return for_each_tag_name(argv, verify_tag);
+   return for_each_tag_name(argv, delete_tag, NULL);
+   if (cmdmode == 'v') {
+   if (format)
+   verify_ref_format(format);
+   return for_each_tag_name(argv, verify_tag, format);
+   }
 
if (msg.given || msgfile) {
if (msg.given && msgfile)
-- 
2.11.0