threads / patch / 44247

v4, 7 partsref-filter: add function to print single ref_array_item

Subject: [PATCH v4 2/7] ref-filter: add function to print single ref_array_item

## tl;dr

16 messages between Oct 7, 2016 and Oct 19, 2016. Diffs are folded; open one to read it.

replies: 15people: 2as markdown or json

santiago@nyu.edu· Oct 7, 2016, 21:07 UTC · lore

[PATCH v4 0/7] Add --format to tag verification

From: Santiago Torres <santiago@nyu.edu>

This is the fourth iteration of the series in [1][2][3], which comes as a result of the discussion in [4]. The main goal of this patch series is to bring --format to git tag verification so that upper-layer tools can inspect the content of a tag and make decisions based on those contents.

In this re-woll we:
* Fixed the author fields and signed off by's throughout the patch
  series
* Added two more patches with unit tests to ensure the format specifier
  behaves as expected
* Fixed a missing initialization for the format specifier in verify-tag which
  caused a crash.
* Fixed an outdated git commit message that had the previous name of a
  function definition.

Thanks, -Santiago

[1] http://public-inbox.org/git/20160930221806.3398-1-santiago@nyu.edu/ [2] http://public-inbox.org/git/20160922185317.349-1-santiago@nyu.edu/ [3] http://public-inbox.org/git/20160926224233.32702-1-santiago@nyu.edu/ [4] http://public-inbox.org/git/20160607195608.16643-1-santiago@nyu.edu/

Lukas Puehringer (4):
  tag: add format specifier to gpg_verify_tag
  gpg-interface, tag: add GPG_VERIFY_QUIET flag
  ref-filter: add function to print single ref_array_item
  builtin/tag: add --format argument for tag -v
Santiago Torres (3):
  builtin/verify-tag: add --format to verify-tag
  t/t7030-verify-tag: Add --format specifier tests
  t/t7004-tag: Add --format specifier tests
 Documentation/git-tag.txt        |  2 +-
 Documentation/git-verify-tag.txt |  2 +-
 builtin/tag.c                    | 34 +++++++++++++++++++++++-----------
 builtin/verify-tag.c             | 13 +++++++++++--
 gpg-interface.h                  |  1 +
 ref-filter.c                     | 10 ++++++++++
 ref-filter.h                     |  3 +++
 t/t7004-tag.sh                   | 16 ++++++++++++++++
 t/t7030-verify-tag.sh            | 16 ++++++++++++++++
 tag.c                            | 22 +++++++++++++++-------
 tag.h                            |  4 ++--
 11 files changed, 99 insertions(+), 24 deletions(-)
-- 
2.10.0
santiago@nyu.edu· Oct 7, 2016, 21:07 UTC · re: santiago@nyu.edu · lore
From: Lukas Puehringer <luk.puehringer@gmail.com>

ref-filter functions are useful for printing git object information using a format specifier. However, some other modules may not want to use this functionality on a ref-array but only print a single item.

Expose a pretty_print_ref function to create, pretty print and free individual ref-items.

Signed-off-by: Lukas Puehringer <luk.puehringer@gmail.com>
---
 ref-filter.c | 10 ++++++++++
 ref-filter.h |  3 +++
 2 files changed, 13 insertions(+)
Show changes to 2 files +13 −0

ref-filter.c, ref-filter.h

diff --git a/ref-filter.c b/ref-filter.c
index 9adbb8a..cfbcd73 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1637,6 +1637,16 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
 	putchar('\n');
 }
 
+void pretty_print_ref(const char *name, const unsigned char *sha1,
+		const char *format, unsigned kind)
+{
+	struct ref_array_item *ref_item;
+	ref_item = new_ref_array_item(name, sha1, 0);
+	ref_item->kind = kind;
+	show_ref_array_item(ref_item, format, 0);
+	free_array_item(ref_item);
+}
+
 /*  If no sorting option is given, use refname to sort as default */
 struct ref_sorting *ref_default_sorting(void)
 {
diff --git a/ref-filter.h b/ref-filter.h
index 14d435e..3d23090 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -107,4 +107,7 @@ struct ref_sorting *ref_default_sorting(void);
 /*  Function to parse --merged and --no-merged options */
 int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset);
 
+void pretty_print_ref(const char *name, const unsigned char *sha1,
+		const char *format, unsigned kind);
+
 #endif /*  REF_FILTER_H  */
-- 
2.10.0
Jeff King· Oct 19, 2016, 08:55 UTC · re: santiago@nyu.edu · lore

Re: [PATCH v4 2/7] ref-filter: add function to print single ref_array_item

On Fri, Oct 07, 2016 at 05:07:16PM -0400, santiago@nyu.edu wrote:
Show 8 quoted lines
> From: Lukas Puehringer <luk.puehringer@gmail.com>
> 
> ref-filter functions are useful for printing git object information
> using a format specifier. However, some other modules may not want to use
> this functionality on a ref-array but only print a single item.
> 
> Expose a pretty_print_ref function to create, pretty print and free
> individual ref-items.
Makes sense.
Show 11 quoted lines
> diff --git a/ref-filter.h b/ref-filter.h
> index 14d435e..3d23090 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -107,4 +107,7 @@ struct ref_sorting *ref_default_sorting(void);
>  /*  Function to parse --merged and --no-merged options */
>  int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset);
>  
> +void pretty_print_ref(const char *name, const unsigned char *sha1,
> +		const char *format, unsigned kind);
> +

What are the possible values for "kind"? I guess these should come from FILTER_REFS_TAGS, BRANCHES, etc. It's probably worth documenting that. Alternatively, is it possible to just determine this from the name? It looks like filter_ref_kind() is how it happens for a normal ref-filter.

-Peff
Jeff King· Oct 19, 2016, 09:16 UTC · re: Jeff King · lore

Re: [PATCH v4 2/7] ref-filter: add function to print single ref_array_item

On Wed, Oct 19, 2016 at 04:55:43AM -0400, Jeff King wrote:
Show 16 quoted lines
> > diff --git a/ref-filter.h b/ref-filter.h
> > index 14d435e..3d23090 100644
> > --- a/ref-filter.h
> > +++ b/ref-filter.h
> > @@ -107,4 +107,7 @@ struct ref_sorting *ref_default_sorting(void);
> >  /*  Function to parse --merged and --no-merged options */
> >  int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset);
> >  
> > +void pretty_print_ref(const char *name, const unsigned char *sha1,
> > +		const char *format, unsigned kind);
> > +
> 
> What are the possible values for "kind"? I guess these should come from
> FILTER_REFS_TAGS, BRANCHES, etc. It's probably worth documenting that.
> Alternatively, is it possible to just determine this from the name? It
> looks like filter_ref_kind() is how it happens for a normal ref-filter.

I guess that may complicate things for the caller you add in this series, which may not have a fully-qualified refname (which is obviously how filter_ref_kind() figures it out). I'd argue that is a bug, though, as things like "%(refname)" are generally expected to print out the fully refname ("git tag --format=%(refname)" does so, and you can use "%(refname:short)" if you want the shorter part).

-Peff
Santiago Torres· Oct 19, 2016, 17:07 UTC · re: Jeff King · lore

Re: [PATCH v4 2/7] ref-filter: add function to print single ref_array_item

On Wed, Oct 19, 2016 at 05:16:42AM -0400, Jeff King wrote:
Show 25 quoted lines
> On Wed, Oct 19, 2016 at 04:55:43AM -0400, Jeff King wrote:
> 
> > > diff --git a/ref-filter.h b/ref-filter.h
> > > index 14d435e..3d23090 100644
> > > --- a/ref-filter.h
> > > +++ b/ref-filter.h
> > > @@ -107,4 +107,7 @@ struct ref_sorting *ref_default_sorting(void);
> > >  /*  Function to parse --merged and --no-merged options */
> > >  int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset);
> > >  
> > > +void pretty_print_ref(const char *name, const unsigned char *sha1,
> > > +		const char *format, unsigned kind);
> > > +
> > 
> > What are the possible values for "kind"? I guess these should come from
> > FILTER_REFS_TAGS, BRANCHES, etc. It's probably worth documenting that.
> > Alternatively, is it possible to just determine this from the name? It
> > looks like filter_ref_kind() is how it happens for a normal ref-filter.
> 
> I guess that may complicate things for the caller you add in this
> series, which may not have a fully-qualified refname (which is obviously
> how filter_ref_kind() figures it out). I'd argue that is a bug, though,
> as things like "%(refname)" are generally expected to print out the
> fully refname ("git tag --format=%(refname)" does so, and you can use
> "%(refname:short)" if you want the shorter part).

Hmm, I hadn't actually noticed that. Do you have any suggestions in how to address this?

In general this feels like a consequence of disambiguating .git/tags/* within builtin/tag.c rather than letting ref-filter figure it out.

Thanks, -Santiago.

santiago@nyu.edu· Oct 7, 2016, 21:07 UTC · re: santiago@nyu.edu · lore

[PATCH v4 4/7] builtin/verify-tag: add --format to verify-tag

From: Santiago Torres <santiago@nyu.edu>

Callers of verify-tag may want to cross-check the tagname from refs/tags with the tagname from the tag object header upon GPG verification. This is to avoid tag refs that point to an incorrect object.

Add a --format parameter to git verify-tag to print the formatted tag object header in addition to or instead of the --verbose or --raw GPG verification output.

Signed-off-by: Santiago Torres <santiago@nyu.edu>
---
 Documentation/git-verify-tag.txt |  2 +-
 builtin/verify-tag.c             | 13 +++++++++++--
 2 files changed, 12 insertions(+), 3 deletions(-)
Show changes to 2 files +12 −3

Documentation/git-verify-tag.txt, builtin/verify-tag.c

diff --git a/Documentation/git-verify-tag.txt b/Documentation/git-verify-tag.txt
index d590edc..0b8075d 100644
--- a/Documentation/git-verify-tag.txt
+++ b/Documentation/git-verify-tag.txt
@@ -8,7 +8,7 @@ git-verify-tag - Check the GPG signature of tags
 SYNOPSIS
 --------
 [verse]
-'git verify-tag' <tag>...
+'git verify-tag' [--format=<format>] <tag>...
 
 DESCRIPTION
 -----------
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index de10198..745b6a6 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -12,12 +12,14 @@
 #include <signal.h>
 #include "parse-options.h"
 #include "gpg-interface.h"
+#include "ref-filter.h"
 
 static const char * const verify_tag_usage[] = {
-		N_("git verify-tag [-v | --verbose] <tag>..."),
+		N_("git verify-tag [-v | --verbose] [--format=<format>] <tag>..."),
 		NULL
 };
 
+
 static int git_verify_tag_config(const char *var, const char *value, void *cb)
 {
 	int status = git_gpg_config(var, value, cb);
@@ -30,9 +32,11 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 {
 	int i = 1, verbose = 0, had_error = 0;
 	unsigned flags = 0;
+	char *fmt_pretty = NULL;
 	const struct option verify_tag_options[] = {
 		OPT__VERBOSE(&verbose, N_("print tag contents")),
 		OPT_BIT(0, "raw", &flags, N_("print raw gpg status output"), GPG_VERIFY_RAW),
+		OPT_STRING(  0 , "format", &fmt_pretty, N_("format"), N_("format to use for the output")),
 		OPT_END()
 	};
 
@@ -46,12 +50,17 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 	if (verbose)
 		flags |= GPG_VERIFY_VERBOSE;
 
+	if (fmt_pretty) {
+		verify_ref_format(fmt_pretty);
+		flags |= GPG_VERIFY_QUIET;
+	}
+
 	while (i < argc) {
 		unsigned char sha1[20];
 		const char *name = argv[i++];
 		if (get_sha1(name, sha1))
 			had_error = !!error("tag '%s' not found.", name);
-		else if (verify_and_format_tag(sha1, name, NULL, flags))
+		else if (verify_and_format_tag(sha1, name, fmt_pretty, flags))
 			had_error = 1;
 	}
 	return had_error;
-- 
2.10.0
Jeff King· Oct 19, 2016, 09:04 UTC · re: santiago@nyu.edu · lore

Re: [PATCH v4 4/7] builtin/verify-tag: add --format to verify-tag

On Fri, Oct 07, 2016 at 05:07:18PM -0400, santiago@nyu.edu wrote:
Show 8 quoted lines
> @@ -46,12 +50,17 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
>  	if (verbose)
>  		flags |= GPG_VERIFY_VERBOSE;
>  
> +	if (fmt_pretty) {
> +		verify_ref_format(fmt_pretty);
> +		flags |= GPG_VERIFY_QUIET;
> +	}

I see why you would want to disable the normal output when there is a custom format. But it seems a shame that the GPG information cannot be retrieved as part of that format.

I think in the long run we'd want something like pretty.c's "%G" placeholders for the ref-filter pretty-printer. But I think we are OK to do this patch without it. It allows at least:

  tag=v2.0.0
  got=$(git verify-tag --format='%(tag)' "$tag") &&
  test "$tag" = "$got" ||
  echo >&2 "verification failed"

which is better than what we have now, and can be extended in the future.

-Peff
santiago@nyu.edu· Oct 7, 2016, 21:07 UTC · re: santiago@nyu.edu · lore

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

From: Lukas Puehringer <luk.puehringer@gmail.com>

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 <luk.puehringer@gmail.com>
---
 builtin/tag.c        |  2 +-
 builtin/verify-tag.c |  2 +-
 tag.c                | 17 +++++++++++------
 tag.h                |  4 ++--
 4 files changed, 15 insertions(+), 10 deletions(-)
Show changes to 4 files +15 −10

builtin/tag.c, builtin/verify-tag.c, tag.c, tag.h

diff --git a/builtin/tag.c b/builtin/tag.c
index 50e4ae5..14f3b48 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 99f8148..de10198 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 291073f..d3512c0 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, &type, &size);
 	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, FILTER_REFS_TAGS);
+
 	return ret;
 }
 
diff --git a/tag.h b/tag.h
index a5721b6..896b9c2 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.10.0
santiago@nyu.edu· Oct 7, 2016, 21:07 UTC · re: santiago@nyu.edu · lore

[PATCH v4 1/7] gpg-interface, tag: add GPG_VERIFY_QUIET flag

From: Lukas Puehringer <luk.puehringer@gmail.com>

Functions that print git object information may require that the gpg-interface functions be silent. Add GPG_VERIFY_QUIET flag and prevent print_signature_buffer from being called if flag is set.

Signed-off-by: Lukas Puehringer <luk.puehringer@gmail.com>
---
 gpg-interface.h | 1 +
 tag.c           | 5 ++++-
 2 files changed, 5 insertions(+), 1 deletion(-)
Show changes to 2 files +5 −1

gpg-interface.h, tag.c

diff --git a/gpg-interface.h b/gpg-interface.h
index ea68885..85dc982 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -3,6 +3,7 @@
 
 #define GPG_VERIFY_VERBOSE	1
 #define GPG_VERIFY_RAW		2
+#define GPG_VERIFY_QUIET	4
 
 struct signature_check {
 	char *payload;
diff --git a/tag.c b/tag.c
index d1dcd18..291073f 100644
--- a/tag.c
+++ b/tag.c
@@ -3,6 +3,7 @@
 #include "commit.h"
 #include "tree.h"
 #include "blob.h"
+#include "gpg-interface.h"
 
 const char *tag_type = "tag";
 
@@ -24,7 +25,9 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
 
 	ret = check_signature(buf, payload_size, buf + payload_size,
 				size - payload_size, &sigc);
-	print_signature_buffer(&sigc, flags);
+
+	if (!(flags & GPG_VERIFY_QUIET))
+		print_signature_buffer(&sigc, flags);
 
 	signature_check_clear(&sigc);
 	return ret;
-- 
2.10.0
Jeff King· Oct 19, 2016, 08:51 UTC · re: santiago@nyu.edu · lore

Re: [PATCH v4 1/7] gpg-interface, tag: add GPG_VERIFY_QUIET flag

On Fri, Oct 07, 2016 at 05:07:15PM -0400, santiago@nyu.edu wrote:
Show 5 quoted lines
> From: Lukas Puehringer <luk.puehringer@gmail.com>
> 
> Functions that print git object information may require that the
> gpg-interface functions be silent. Add GPG_VERIFY_QUIET flag and prevent
> print_signature_buffer from being called if flag is set.

The layering here is a little funny. The gpg-interface code allocates a new flag, but none of its functions do anything with it. Instead, it's acted on only by the run_gpg_verify() command local to tag.c.

I guess this "flags" variable comes to us from other callsites via gpg_verify_tag. That's still in tag.c, but it's arguably part of the gpg interface.

So I think it's OK.
-Peff
santiago@nyu.edu· Oct 7, 2016, 21:07 UTC · re: santiago@nyu.edu · lore

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

From: Lukas Puehringer <luk.puehringer@gmail.com>

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 <luk.puehringer@gmail.com>
---
 Documentation/git-tag.txt |  2 +-
 builtin/tag.c             | 34 +++++++++++++++++++++++-----------
 builtin/verify-tag.c      |  2 +-
 3 files changed, 25 insertions(+), 13 deletions(-)
Show changes to 2 files +24 −12

Documentation/git-tag.txt, builtin/tag.c

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 7ecca8e..3bb5e3c 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -15,7 +15,7 @@ SYNOPSIS
 'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
 	[--column[=<options>] | --no-column] [--create-reflog] [--sort=<key>]
 	[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]
-'git tag' -v <tagname>...
+'git tag' -v [--format=<format>] <tagname>...
 
 DESCRIPTION
 -----------
diff --git a/builtin/tag.c b/builtin/tag.c
index 14f3b48..7730fd0 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -24,7 +24,7 @@ static const char * const git_tag_usage[] = {
 	N_("git tag -d <tagname>..."),
 	N_("git tag -l [-n[<num>]] [--contains <commit>] [--points-at <object>]"
 		"\n\t\t[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]"),
-	N_("git tag -v <tagname>..."),
+	N_("git tag -v [--format=<format>] <tagname>..."),
 	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, name, fmt_pretty, flags);
 }
 
 static int do_sign(struct strbuf *buffer)
@@ -334,7 +343,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	struct strbuf err = STRBUF_INIT;
 	struct ref_filter filter;
 	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
-	const char *format = NULL;
+	char *format = NULL;
 	struct option options[] = {
 		OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
 		{ OPTION_INTEGER, 'n', NULL, &filter.lines, N_("n"),
@@ -424,9 +433,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.10.0
Jeff King· Oct 19, 2016, 09:10 UTC · re: santiago@nyu.edu · lore

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

On Fri, Oct 07, 2016 at 05:07:19PM -0400, santiago@nyu.edu wrote:
> Adding --format to git tag -v mutes the default output of the GPG
> verification and instead prints the formatted tag object.

The same comments apply to "mutes" here, as to the previous patch (which is to say I think we may want something more, but this is probably OK for now).

Show 10 quoted lines
> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> index 7ecca8e..3bb5e3c 100644
> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -15,7 +15,7 @@ SYNOPSIS
>  'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
>  	[--column[=<options>] | --no-column] [--create-reflog] [--sort=<key>]
>  	[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]
> -'git tag' -v <tagname>...
> +'git tag' -v [--format=<format>] <tagname>...

Just thinking out loud, but if we had ref-filter placeholders that triggered GPG verification, you could do this all with the listing mode, like:

  git tag --format='%(gpgstatus) %(tag) %(refname:short)'
and verify multiple tags (or give a single tag to limit it to just one).

I don't think that's any kind of blocker for this series. We already have "-v", and adding --format to it is reasonable, even if we eventually move to a world where people can use the listing mode. Like I said, just thinking out loud.

Show 10 quoted lines
> +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++) {
Extra space?
Show 6 quoted lines
>  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;

Probably doesn't matter much, but these flags are defined as "unsigned" elsewhere.

-Peff
santiago@nyu.edu· Oct 7, 2016, 21:07 UTC · re: santiago@nyu.edu · lore

[PATCH v4 6/7] t/t7030-verify-tag: Add --format specifier tests

From: Santiago Torres <santiago@nyu.edu>

Verify-tag now provides --format specifiers to inspect and ensure the contents of the tag are proper. We add two tests to ensure this functionality works as expected: the return value should indicate if verification passed, and the format specifiers must be respected.

Signed-off-by: Santiago Torres <santiago@nyu.edu>
---
 t/t7030-verify-tag.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
Show changes to t/t7030-verify-tag.sh +16 −0
diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh
index 07079a4..ce37fd9 100755
--- a/t/t7030-verify-tag.sh
+++ b/t/t7030-verify-tag.sh
@@ -125,4 +125,20 @@ test_expect_success GPG 'verify multiple tags' '
 	test_cmp expect.stderr actual.stderr
 '
 
+test_expect_success 'verifying tag with --format' '
+	cat >expect <<-\EOF &&
+	tagname : fourth-signed
+	EOF
+	git verify-tag --format="tagname : %(tag)" "fourth-signed" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'verifying a forged tag with --format fail and format accordingly' '
+	cat >expect <<-\EOF &&
+	tagname : 7th forged-signed
+	EOF
+	test_must_fail git verify-tag --format="tagname : %(tag)" $(cat forged1.tag) >actual-forged &&
+	test_cmp expect actual-forged
+'
+
 test_done
-- 
2.10.0
Jeff King· Oct 19, 2016, 09:13 UTC · re: santiago@nyu.edu · lore

Re: [PATCH v4 6/7] t/t7030-verify-tag: Add --format specifier tests

On Fri, Oct 07, 2016 at 05:07:20PM -0400, santiago@nyu.edu wrote:
> Verify-tag now provides --format specifiers to inspect and ensure the
> contents of the tag are proper. We add two tests to ensure this
> functionality works as expected: the return value should indicate if
> verification passed, and the format specifiers must be respected.
Sounds good.
Show 7 quoted lines
> +test_expect_success 'verifying tag with --format' '
> +	cat >expect <<-\EOF &&
> +	tagname : fourth-signed
> +	EOF
> +	git verify-tag --format="tagname : %(tag)" "fourth-signed" >actual &&
> +	test_cmp expect actual
> +'

I suppose it's a matter of style, but for a single-line expectation I would just do:

  echo "tagname : fourth-signed" >expect &&
which is shorter and saves a process invocation.

Ditto on the next patch (which IMHO could just be squashed into a single patch).

-Peff
santiago@nyu.edu· Oct 7, 2016, 21:07 UTC · re: santiago@nyu.edu · lore

[PATCH v4 7/7] t/t7004-tag: Add --format specifier tests

From: Santiago Torres <santiago@nyu.edu>

tag -v now supports --format specifiers to inspect the contents of a tag upon verification. Add two tests to ensure this behavior is respected in future changes.

Signed-off-by: Santiago Torres <santiago@nyu.edu>
---
 t/t7004-tag.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
Show changes to t/t7004-tag.sh +16 −0
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 8b0f71a..633b089 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -847,6 +847,22 @@ test_expect_success GPG 'verifying a forged tag should fail' '
 	test_must_fail git tag -v forged-tag
 '
 
+test_expect_success 'verifying a proper tag with --format pass and format accordingly' '
+	cat >expect <<-\EOF &&
+	tagname : signed-tag
+	EOF
+	git tag -v --format="tagname : %(tag)" "signed-tag" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'verifying a forged tag with --format fail and format accordingly' '
+	cat >expect <<-\EOF &&
+	tagname : forged-tag
+	EOF
+	test_must_fail git tag -v --format="tagname : %(tag)" "forged-tag" >actual &&
+	test_cmp expect actual
+'
+
 # blank and empty messages for signed tags:
 
 get_tag_header empty-signed-tag $commit commit $time >expect
-- 
2.10.0
Santiago Torres· Oct 11, 2016, 17:43 UTC · re: santiago@nyu.edu · lore

Re: [PATCH v4 0/7] Add --format to tag verification

Hi,

I noticed there were no replies for this thread. I was curious if it got buried because I sent it on the Friday evening before a long weekend.

I don't mean to pressure or anything.

Thanks! -Santiago.

On Fri, Oct 07, 2016 at 05:07:14PM -0400, santiago@nyu.edu wrote:
Show 57 quoted lines
> From: Santiago Torres <santiago@nyu.edu>
> 
> This is the fourth iteration of the series in [1][2][3], which comes as a
> result of the discussion in [4]. The main goal of this patch series is to bring
> --format to git tag verification so that upper-layer tools can inspect the
> content of a tag and make decisions based on those contents.
> 
> In this re-woll we:
> 
> * Fixed the author fields and signed off by's throughout the patch
>   series
> 
> * Added two more patches with unit tests to ensure the format specifier
>   behaves as expected
> 
> * Fixed a missing initialization for the format specifier in verify-tag which
>   caused a crash.
> 
> * Fixed an outdated git commit message that had the previous name of a
>   function definition.
> 
> Thanks,
> -Santiago
> 
> [1] http://public-inbox.org/git/20160930221806.3398-1-santiago@nyu.edu/
> [2] http://public-inbox.org/git/20160922185317.349-1-santiago@nyu.edu/
> [3] http://public-inbox.org/git/20160926224233.32702-1-santiago@nyu.edu/
> [4] http://public-inbox.org/git/20160607195608.16643-1-santiago@nyu.edu/
> 
> 
> Lukas Puehringer (4):
>   tag: add format specifier to gpg_verify_tag
>   gpg-interface, tag: add GPG_VERIFY_QUIET flag
>   ref-filter: add function to print single ref_array_item
>   builtin/tag: add --format argument for tag -v
> 
> Santiago Torres (3):
>   builtin/verify-tag: add --format to verify-tag
>   t/t7030-verify-tag: Add --format specifier tests
>   t/t7004-tag: Add --format specifier tests
> 
>  Documentation/git-tag.txt        |  2 +-
>  Documentation/git-verify-tag.txt |  2 +-
>  builtin/tag.c                    | 34 +++++++++++++++++++++++-----------
>  builtin/verify-tag.c             | 13 +++++++++++--
>  gpg-interface.h                  |  1 +
>  ref-filter.c                     | 10 ++++++++++
>  ref-filter.h                     |  3 +++
>  t/t7004-tag.sh                   | 16 ++++++++++++++++
>  t/t7030-verify-tag.sh            | 16 ++++++++++++++++
>  tag.c                            | 22 +++++++++++++++-------
>  tag.h                            |  4 ++--
>  11 files changed, 99 insertions(+), 24 deletions(-)
> 
> -- 
> 2.10.0
> 

← back to recent threads