[PATCH v2 0/5] Add --format to tag verification

[PATCH v2 0/5] Add --format to tag verification

[santiago]

From: Santiago Torres <santiago@nyu.edu>

This is the second iteration of [1], and as a result of the discussion
in [2].

In this re-roll we:

* Dropped the commit to move the format string parameter to a global
  variable on builtin/tag. We had to change the signature of
  for_each_name_fn to do this.

* Fixed the signed-off-by lines to match the author/committer

[0001]
* Moved the GPG_VERIFY_QUIET flag check into tag.c instead of
  gpg_interface

[0002] 
* Changed the exported function to "format_ref". This way, we can print
  a single formatted ref element. This also made the patch
  cleaner/easier to read.

[0003] 
* We fixed style/formatting errors that we had introduced

This patch applies to 2.10.0 and master.

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

Lukas P (4):
  gpg-interface, tag: add GPG_VERIFY_QUIET flag
  ref-filter: add function to print single ref_array_item
  tag: add format specifier to gpg_verify_tag
  builtin/tag: add --format argument for tag -v

Santiago Torres (1):
  builtin/verify-tag: add --format to verify-tag

 builtin/tag.c        | 30 ++++++++++++++++++++----------
 builtin/verify-tag.c | 16 ++++++++++++++--
 gpg-interface.h      |  1 +
 ref-filter.c         | 10 ++++++++++
 ref-filter.h         |  4 ++++
 tag.c                | 22 +++++++++++++++-------
 tag.h                |  4 ++--
 7 files changed, 66 insertions(+), 21 deletions(-)

-- 
2.10.0

[PATCH v2 1/5] gpg-interface, tag: add GPG_VERIFY_QUIET flag

[santiago]

From: Lukas P <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 P <luk.puehringer@gmail.com>
---
 gpg-interface.h | 1 +
 tag.c           | 5 ++++-
 2 files changed, 5 insertions(+), 1 deletion(-)

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

Re: [PATCH v2 1/5] gpg-interface, tag: add GPG_VERIFY_QUIET flag

[Junio C Hamano]

santiago@nyu.edu writes:

> From: Lukas P <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 P <luk.puehringer@gmail.com>

Are you and Lukas sure that "Lukas P" is how luk.puehringer wants to
be known by the world?  Just checking.

Re: [PATCH v2 1/5] gpg-interface, tag: add GPG_VERIFY_QUIET flag

[Lukas Pühringer]

Thanks for checking. I am fine with Lukas P, unless git prefers full last names. In that case I am fine with changing too.

Best,
Lukas P

> On Sep 27, 2016, at 1:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 
> santiago@nyu.edu writes:
> 
>> From: Lukas P <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 P <luk.puehringer@gmail.com>
> 
> Are you and Lukas sure that "Lukas P" is how luk.puehringer wants to
> be known by the world?  Just checking.
> 

Re: [PATCH v2 1/5] gpg-interface, tag: add GPG_VERIFY_QUIET flag

[Junio C Hamano]

Lukas Pühringer <luk.puehringer@gmail.com> writes:

> Thanks for checking. I am fine with Lukas P, unless git prefers
> full last names. In that case I am fine with changing too.

We do prefer full names, so that it would be consistent with court
document when you are involved in copyright inflingement case ;-)

Re: [PATCH v2 1/5] gpg-interface, tag: add GPG_VERIFY_QUIET flag

[Lukas Pühringer]

Good, I will change it to 'Lukas Puehringer' then, when we send you the updated batch of patches, that address your latest comments.

Thanks,
Lukas

> On Sep 27, 2016, at 2:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Lukas Pühringer <luk.puehringer@gmail.com> writes:
> 
>> Thanks for checking. I am fine with Lukas P, unless git prefers
>> full last names. In that case I am fine with changing too.
> 
> We do prefer full names, so that it would be consistent with court
> document when you are involved in copyright inflingement case ;-)

Re: [PATCH v2 1/5] gpg-interface, tag: add GPG_VERIFY_QUIET flag

[Stefan Beller]

On Tue, Sep 27, 2016 at 11:25 AM, Lukas Pühringer
<luk.puehringer@gmail.com> wrote:
> Good, I will change it to 'Lukas Puehringer' then, when we send you the updated batch of patches, that address your latest comments.

No need to stay full ASCII. German umlauts are fine.
(See `git shortlog -s` for all the contributor names, there are also
other alphabets in use)

Stefan

Re: [PATCH v2 1/5] gpg-interface, tag: add GPG_VERIFY_QUIET flag

[Lukas Pühringer]

I made it a habit to use ‘ue’ instead of ‘ü' outside of German speaking countries and in coding. It makes my life easier.
But thanks for the hint.

Lukas
> On Sep 27, 2016, at 2:31 PM, Stefan Beller <sbeller@google.com> wrote:
> 
> On Tue, Sep 27, 2016 at 11:25 AM, Lukas Pühringer
> <luk.puehringer@gmail.com> wrote:
>> Good, I will change it to 'Lukas Puehringer' then, when we send you the updated batch of patches, that address your latest comments.
> 
> No need to stay full ASCII. German umlauts are fine.
> (See `git shortlog -s` for all the contributor names, there are also
> other alphabets in use)
> 
> Stefan

[PATCH v2 2/5] ref-filter: add function to print single ref_array_item

[santiago]

From: Lukas P <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 format_ref function to create, pretty print and free individual
ref-items.

Signed-off-by: Lukas P <luk.puehringer@gmail.com>
---
 ref-filter.c | 10 ++++++++++
 ref-filter.h |  4 ++++
 2 files changed, 14 insertions(+)

diff --git a/ref-filter.c b/ref-filter.c
index bc551a7..e0aaf5f 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1655,6 +1655,16 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
 	putchar('\n');
 }
 
+void format_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..1ef7999 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -107,4 +107,8 @@ 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);
 
+/* Pretty-print a single ref */
+void format_ref(const char *name, const unsigned char *sha1, const char *format,
+		unsigned kind);
+
 #endif /*  REF_FILTER_H  */
-- 
2.10.0

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

[Junio C Hamano]

santiago@nyu.edu writes:

> From: Lukas P <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 format_ref function to create, pretty print and free individual
> ref-items.
>
> Signed-off-by: Lukas P <luk.puehringer@gmail.com>
> ---
> +void format_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..1ef7999 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -107,4 +107,8 @@ 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);
>  
> +/* Pretty-print a single ref */
> +void format_ref(const char *name, const unsigned char *sha1, const char *format,
> +		unsigned kind);

The fact that you felt a need for comment before its name is a
strong sign that the name is not sufficiently descriptive and
understandable for readers to tell what the function is for.

Would pretty_print_ref() or show_ref_pretty() better names, perhaps?

[PATCH v2 3/5] tag: add format specifier to gpg_verify_tag

[santiago]

From: Lukas P <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 P <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(-)

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..65e1a96 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)
+		format_ref(name, sha1, fmt_pretty, FILTER_REFS_TAGS);
+
 	return ret;
 }
 
diff --git a/tag.h b/tag.h
index a5721b6..0b6e458 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

[PATCH v2 4/5] builtin/verify-tag: add --format to verify-tag

[santiago]

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>
---
 builtin/verify-tag.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index de10198..a941053 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -12,12 +12,15 @@
 #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 char *fmt_pretty;
+
 static int git_verify_tag_config(const char *var, const char *value, void *cb)
 {
 	int status = git_gpg_config(var, value, cb);
@@ -33,6 +36,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 	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

Re: [PATCH v2 4/5] builtin/verify-tag: add --format to verify-tag

[Philip Oakley]

From: <santiago@nyu.edu>
> 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>
> ---
> builtin/verify-tag.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> index de10198..a941053 100644
> --- a/builtin/verify-tag.c
> +++ b/builtin/verify-tag.c
> @@ -12,12 +12,15 @@
> #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>..."),

Does this require a corresponding documentation change? (also 5/5)

>  NULL
> };
>
> +static char *fmt_pretty;
> +
> static int git_verify_tag_config(const char *var, const char *value, void 
> *cb)
> {
>  int status = git_gpg_config(var, value, cb);
> @@ -33,6 +36,7 @@ int cmd_verify_tag(int argc, const char **argv, const 
> char *prefix)
>  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
>
--
Philip 

Re: [PATCH v2 4/5] builtin/verify-tag: add --format to verify-tag

[Santiago Torres]

[-- Attachment #1: Type: text/plain, Size: 342 bytes --]

> > static const char * const verify_tag_usage[] = {
> > - N_("git verify-tag [-v | --verbose] <tag>..."),
> > + N_("git verify-tag [-v | --verbose] [--format=<format>] <tag>..."),
> 
> Does this require a corresponding documentation change? (also 5/5)
> 

Yes, I'll work on this while I wait for more reviews.

Thanks!
-Santiago.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 4/5] builtin/verify-tag: add --format to verify-tag

[Junio C Hamano]

santiago@nyu.edu writes:

> 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>
> ---
>  builtin/verify-tag.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> index de10198..a941053 100644
> --- a/builtin/verify-tag.c
> +++ b/builtin/verify-tag.c
> @@ -12,12 +12,15 @@
>  #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 char *fmt_pretty;
> +

I'd suggest to remove this, and then ...

>  static int git_verify_tag_config(const char *var, const char *value, void *cb)
>  {
>  	int status = git_gpg_config(var, value, cb);
> @@ -33,6 +36,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)

... instead add

	char *fmt_pretty = NULL;

here, imitating the way "&flags" is handled.  You do not need it to
be visible outside the function.

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

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

[santiago]

From: Lukas P <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.

Caveat: The change adds a format specifier argument to the
(*each_tag_name_fn) function pointer, i.e. delete_tag now receives this
too, although it does not need it.

Signed-off-by: Lukas P <luk.puehringer@gmail.com>
---
 builtin/tag.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 14f3b48..f53227e 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,9 +66,10 @@ 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, const char *fmt_pretty);
 
-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,
+		const char *fmt_pretty)
 {
 	const char **p;
 	char ref[PATH_MAX];
@@ -87,14 +88,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, fmt_pretty))
 			had_error = 1;
 	}
 	return had_error;
 }
 
 static int delete_tag(const char *name, const char *ref,
-				const unsigned char *sha1)
+				const unsigned char *sha1, const char *fmt_pretty)
 {
 	if (delete_ref(ref, sha1, 0))
 		return 1;
@@ -103,9 +104,15 @@ 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, const char *fmt_pretty)
 {
-	return verify_and_format_tag(sha1, name, NULL, GPG_VERIFY_VERBOSE);
+	int flags;
+	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)
@@ -424,9 +431,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

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

[Junio C Hamano]

santiago@nyu.edu writes:

> From: Lukas P <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.
>
> Caveat: The change adds a format specifier argument to the
> (*each_tag_name_fn) function pointer, i.e. delete_tag now receives this
> too, although it does not need it.

That's an interesting "caveat".

Generally it is a good idea to give an additional opaque pointer to
callback functions of iteration API so that code that uses the
iteration can pass custom data to its callback.

Looking at the way you enhanced each_tag_name_fn, however, you added
a specific argument instead; that is the only reason why you need a
"caveat".  If it were "void *", it would have been in line with the
usual practice, not worth mentioning as a "caveat", but could even
be advertised as a feature, replacing the last "Caveat" paragraph
with something like this:

	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 P <luk.puehringer@gmail.com>
> ---
>  builtin/tag.c | 30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 14f3b48..f53227e 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,9 +66,10 @@ 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, const char *fmt_pretty);

You'd replace "const char *fmt_pretty" with "void *cb_data" here, and...
>  
> -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,
> +		const char *fmt_pretty)

... also here.  Then introduce fmt_pretty as an auto variable in the
function ...

>  {
>  	const char **p;

... by adding this line here:

	const char *fmt_pretty = cb_data;

>  	char ref[PATH_MAX];
> @@ -87,14 +88,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, fmt_pretty))
>  			had_error = 1;
>  	}
>  	return had_error;
>  }
>  
>  static int delete_tag(const char *name, const char *ref,
> -				const unsigned char *sha1)
> +				const unsigned char *sha1, const char *fmt_pretty)

And this "const char *fmt_pretty" also becomes "void *cb_data"...

>  {
>  	if (delete_ref(ref, sha1, 0))
>  		return 1;
> @@ -103,9 +104,15 @@ 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, const char *fmt_pretty)

... and here.  Reintroduce fmt_pretty as a name local to the
function by doing the same thing as for_each_tag_name() above.

>  {
> -	return verify_and_format_tag(sha1, name, NULL, GPG_VERIFY_VERBOSE);
> +	int flags;
> +	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)
> @@ -424,9 +431,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);
> +	}

Thanks.