[RFC/PATCH 0/6] Add --format to tag verification

[RFC/PATCH 0/6] Add --format to tag verification

[santiago]

From: Santiago Torres <santiago@nyu.edu>

Hello everyone,

This is a followup on [1]. There we discussed what would be the best way
to provide automated scripts with mechanisms to inspect the contents of
a tag upon verification.

We struggled a little bit with how to make this fit the current git
codebase in the best way. Specifically, we are not sure if adding the
GPG_QUIET flags and/or exposing the primitives to allocate individual
git_ref_item's would be the best way forward.

This applies on the current HEAD as well as v 2.9.10.

Thanks!
-Santiago.

P.S. Gmane seems to be broken for git after it was rebooted. Should we ping
them about it?

[1] http://lists-archives.com/git/869122-verify-tag-add-check-name-flag.html

Lukas P (4):
  gpg-interface: add GPG_VERIFY_QUIET flag
  ref-filter: Expose wrappers for ref_item functions
  tag: add format specifier to gpg_verify_tag
  builtin/tag: add --format argument for tag -v

Santiago Torres (2):
  builtin/tag: move format specifier to global var
  builtin/verify-tag: Add --format to verify-tag

 builtin/tag.c        | 33 +++++++++++++++++++++------------
 builtin/verify-tag.c | 17 ++++++++++++++---
 gpg-interface.c      |  3 +++
 gpg-interface.h      |  1 +
 ref-filter.c         | 20 ++++++++++++++++++++
 ref-filter.h         | 10 ++++++++++
 tag.c                | 14 ++++++++++++--
 tag.h                |  4 ++--
 8 files changed, 83 insertions(+), 19 deletions(-)

-- 
2.10.0

[PATCH 1/6] builtin/tag: move format specifier to global var

[santiago]

From: Santiago Torres <santiago@nyu.edu>

The format specifier will be likely used in other functions throughout
git tag. One likely candidate to require format strings in the future is
the gpg_verify_tag function. However, changing the signature of
functions such as for_each_ref or verify_tag would be quite burdensome.

Instead, we move the format string specifier to a static global variable
that modules can access in the same way git-log and other modules handle
this case.

Signed-off-by: Santiago Torres <santiago@nyu.edu>
---
 builtin/tag.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 50e4ae5..dbf271f 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -30,8 +30,9 @@ static const char * const git_tag_usage[] = {
 
 static unsigned int colopts;
 static int force_sign_annotate;
+static const char *fmt_pretty;
 
-static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)
+static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting)
 {
 	struct ref_array array;
 	char *to_free = NULL;
@@ -42,23 +43,23 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con
 	if (filter->lines == -1)
 		filter->lines = 0;
 
-	if (!format) {
+	if (!fmt_pretty) {
 		if (filter->lines) {
 			to_free = xstrfmt("%s %%(contents:lines=%d)",
 					  "%(align:15)%(refname:strip=2)%(end)",
 					  filter->lines);
-			format = to_free;
+			fmt_pretty = to_free;
 		} else
-			format = "%(refname:strip=2)";
+			fmt_pretty = "%(refname:strip=2)";
 	}
 
-	verify_ref_format(format);
+	verify_ref_format(fmt_pretty);
 	filter->with_commit_tag_algo = 1;
 	filter_refs(&array, filter, FILTER_REFS_TAGS);
 	ref_array_sort(sorting, &array);
 
 	for (i = 0; i < array.nr; i++)
-		show_ref_array_item(array.items[i], format, 0);
+		show_ref_array_item(array.items[i], fmt_pretty, 0);
 	ref_array_clear(&array);
 	free(to_free);
 
@@ -334,7 +335,6 @@ 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;
 	struct option options[] = {
 		OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
 		{ OPTION_INTEGER, 'n', NULL, &filter.lines, N_("n"),
@@ -369,7 +369,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			OPTION_CALLBACK, 0, "points-at", &filter.points_at, N_("object"),
 			N_("print only tags of the object"), 0, parse_opt_object_name
 		},
-		OPT_STRING(  0 , "format", &format, N_("format"), N_("format to use for the output")),
+		OPT_STRING(  0 , "format", &fmt_pretty, N_("format"), N_("format to use for the output")),
 		OPT_END()
 	};
 
@@ -410,7 +410,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			run_column_filter(colopts, &copts);
 		}
 		filter.name_patterns = argv;
-		ret = list_tags(&filter, sorting, format);
+		ret = list_tags(&filter, sorting);
 		if (column_active(colopts))
 			stop_column_filter();
 		return ret;
-- 
2.10.0

Re: [PATCH 1/6] builtin/tag: move format specifier to global var

[Junio C Hamano]

santiago@nyu.edu writes:

> From: Santiago Torres <santiago@nyu.edu>
>
> The format specifier will be likely used in other functions throughout
> git tag. One likely candidate to require format strings in the future is
> the gpg_verify_tag function. However, changing the signature of
> functions such as for_each_ref or verify_tag would be quite burdensome.

I do not understand the above excuse.  for-each-ref takes a
callback data pointer exactly because it wants you to be able to
extend what data the callback function gets without changing its
signature.  builtin/tag.c::verify_tag() is a helper static to the
file--why should it be "burdensome" to change it to fit your needs?

Adding technical debt by going backwards is never a good idea
especially done to add a new feature that is not desperately needed.

[PATCH 2/6] gpg-interface: 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 a GPG_VERIFY_QUIET to prevent
functions such as `print_signature_buffer` from printing any output and
only return whether signature verification passed or not.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
---
 gpg-interface.c | 3 +++
 gpg-interface.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/gpg-interface.c b/gpg-interface.c
index 8672eda..b82bc50 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -88,6 +88,9 @@ int check_signature(const char *payload, size_t plen, const char *signature,
 
 void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
 {
+	if (flags & GPG_VERIFY_QUIET)
+		return;
+
 	const char *output = flags & GPG_VERIFY_RAW ?
 		sigc->gpg_status : sigc->gpg_output;
 
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;
-- 
2.10.0

Re: [PATCH 2/6] gpg-interface: add GPG_VERIFY_QUIET flag

[Junio C Hamano]

santiago@nyu.edu writes:

> diff --git a/gpg-interface.c b/gpg-interface.c
> index 8672eda..b82bc50 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -88,6 +88,9 @@ int check_signature(const char *payload, size_t plen, const char *signature,
>  
>  void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
>  {
> +	if (flags & GPG_VERIFY_QUIET)
> +		return;
> +
>  	const char *output = flags & GPG_VERIFY_RAW ?
>  		sigc->gpg_status : sigc->gpg_output;

This will not compile with -Werror=declaration-after-statement.

Re: [PATCH 2/6] gpg-interface: add GPG_VERIFY_QUIET flag

[Junio C Hamano]

santiago@nyu.edu writes:

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

Please match this with S-o-b: below.

>
> Functions that print git object information may require that the
> gpg-interface functions be silent. Add a GPG_VERIFY_QUIET to prevent
> functions such as `print_signature_buffer` from printing any output and
> only return whether signature verification passed or not.
>
> Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
> ---
>  gpg-interface.c | 3 +++
>  gpg-interface.h | 1 +
>  2 files changed, 4 insertions(+)
>
> diff --git a/gpg-interface.c b/gpg-interface.c
> index 8672eda..b82bc50 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -88,6 +88,9 @@ int check_signature(const char *payload, size_t plen, const char *signature,
>  
>  void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
>  {
> +	if (flags & GPG_VERIFY_QUIET)
> +		return;
> +
>  	const char *output = flags & GPG_VERIFY_RAW ?
>  		sigc->gpg_status : sigc->gpg_output;

This has only two callsites, which both know what flags they are
passing.  Doesn't it make more sense to drop this patch (and
possibly addition of GPG_VERIFY_QUIET flag as well) and teach them
not to call this function in the first place?

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

[PATCH 3/6] ref-filter: Expose wrappers for ref_item functions

[santiago]

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

Ref-filter functions are useful for printing git object information
without a format specifier. However, some functions may not want to use
a complete ref-array, and just a single item instead. Expose
create/show/free functions for ref_array_items through wrappers around
the original functions.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
---
 ref-filter.c | 20 ++++++++++++++++++++
 ref-filter.h | 10 ++++++++++
 2 files changed, 30 insertions(+)

diff --git a/ref-filter.c b/ref-filter.c
index 9adbb8a..b013799 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1329,6 +1329,14 @@ static struct ref_array_item *new_ref_array_item(const char *refname,
 	return ref;
 }
 
+/* Wrapper: Create ref_array_item w/o referencing container in function name */
+struct ref_array_item *new_ref_item(const char *refname,
+						 const unsigned char *objectname,
+						 int flag)
+{
+	return new_ref_array_item(refname, objectname, flag);
+}
+
 static int filter_ref_kind(struct ref_filter *filter, const char *refname)
 {
 	unsigned int i;
@@ -1426,6 +1434,12 @@ static void free_array_item(struct ref_array_item *item)
 	free(item);
 }
 
+/* Wrapper: Free ref_array_item w/o referencing container in function name */
+void free_ref_item(struct ref_array_item *ref_item)
+{
+	free_array_item(ref_item);
+}
+
 /* Free all memory allocated for ref_array */
 void ref_array_clear(struct ref_array *array)
 {
@@ -1637,6 +1651,12 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
 	putchar('\n');
 }
 
+/* Wrapper: Show ref_array_item w/o referencing container in function name */
+void show_ref_item(struct ref_array_item *ref_item, const char *format, int quote_style)
+{
+	show_ref_array_item(ref_item, format, quote_style);
+}
+
 /*  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..0f0ffe9 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -107,4 +107,14 @@ 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);
 
+/*
+ * Wrappers exposing the ref_array_item data structure independently
+ * of the container ref_array, e.g. to format-print individual refs.
+ */
+struct ref_array_item *new_ref_item(const char *refname,
+		const unsigned char *objectname, int flag);
+void show_ref_item(struct ref_array_item *ref_item, const char *format,
+		int quote_style);
+void free_ref_item(struct ref_array_item *ref_item);
+
 #endif /*  REF_FILTER_H  */
-- 
2.10.0

Re: [PATCH 3/6] ref-filter: Expose wrappers for ref_item functions

[Junio C Hamano]

santiago@nyu.edu writes:

> From: Lukas P <luk.puehringer@gmail.com>
>
> Ref-filter functions are useful for printing git object information
> without a format specifier. However, some functions may not want to use
> a complete ref-array, and just a single item instead. Expose
> create/show/free functions for ref_array_items through wrappers around
> the original functions.
>
> Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
> ---
>  ref-filter.c | 20 ++++++++++++++++++++
>  ref-filter.h | 10 ++++++++++
>  2 files changed, 30 insertions(+)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 9adbb8a..b013799 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1329,6 +1329,14 @@ static struct ref_array_item *new_ref_array_item(const char *refname,
>  	return ref;
>  }
>  
> +/* Wrapper: Create ref_array_item w/o referencing container in function name */
> +struct ref_array_item *new_ref_item(const char *refname,
> +						 const unsigned char *objectname,
> +						 int flag)
> +{
> +	return new_ref_array_item(refname, objectname, flag);
> +}

Why?  As a public function name, new_ref_item() is a horrible one,
as there are other structures about "ref" elsewhere in the system.
If a new caller needs to be able to get a new ref_array_item, you
are better off just exposing it, not an ill-named wrapper.

>  static int filter_ref_kind(struct ref_filter *filter, const char *refname)
>  {
>  	unsigned int i;
> @@ -1426,6 +1434,12 @@ static void free_array_item(struct ref_array_item *item)
>  	free(item);
>  }
>  
> +/* Wrapper: Free ref_array_item w/o referencing container in function name */
> +void free_ref_item(struct ref_array_item *ref_item)
> +{
> +	free_array_item(ref_item);
> +}

Again, why?  free_array_item() is a horrible name for a public
function, and it is OK to rename it to free_ref_array_item() while
giving external callers an access to it, so that their names are
descriptive enough to convey that they are about ref_array_item
structure used in ref-filter API while at the same time making it
clear to readers that the two functions with related names are
indeed related.

> @@ -1637,6 +1651,12 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
>  	putchar('\n');
>  }
>  
> +/* Wrapper: Show ref_array_item w/o referencing container in function name */
> +void show_ref_item(struct ref_array_item *ref_item, const char *format, int quote_style)
> +{
> +	show_ref_array_item(ref_item, format, quote_style);
> +}

Ditto.

[PATCH 4/6] 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 Puehringer <lukas.puehringer@nyu.edu>
---
 builtin/tag.c        |  2 +-
 builtin/verify-tag.c |  6 ++++--
 tag.c                | 14 ++++++++++++--
 tag.h                |  4 ++--
 4 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index dbf271f..94ed8a2 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -106,7 +106,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..7a1121b 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -51,8 +51,10 @@ 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))
-			had_error = 1;
+		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 d1dcd18..e08da51 100644
--- a/tag.c
+++ b/tag.c
@@ -3,6 +3,7 @@
 #include "commit.h"
 #include "tree.h"
 #include "blob.h"
+#include "ref-filter.h"
 
 const char *tag_type = "tag";
 
@@ -30,8 +31,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_to_report,
+		const char *fmt_pretty, unsigned flags)
 {
 	enum object_type type;
 	char *buf;
@@ -56,6 +57,15 @@ int gpg_verify_tag(const unsigned char *sha1, const char *name_to_report,
 	ret = run_gpg_verify(buf, size, flags);
 
 	free(buf);
+
+	if (fmt_pretty) {
+		struct ref_array_item *ref_item;
+		ref_item = new_ref_item(name_to_report, sha1, 0);
+		ref_item->kind = FILTER_REFS_TAGS;
+		show_ref_item(ref_item, fmt_pretty, 0);
+		free_ref_item(ref_item);
+	}
+
 	return ret;
 }
 
diff --git a/tag.h b/tag.h
index a5721b6..460b6f9 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_to_report, const char *fmt_pretty, unsigned flags);
 
 #endif /* TAG_H */
-- 
2.10.0

Re: [PATCH 4/6] tag: add format specifier to gpg_verify_tag

[Junio C Hamano]

santiago@nyu.edu writes:

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

> diff --git a/builtin/tag.c b/builtin/tag.c
> index dbf271f..94ed8a2 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -106,7 +106,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..7a1121b 100644
> --- a/builtin/verify-tag.c
> +++ b/builtin/verify-tag.c
> @@ -51,8 +51,10 @@ 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))
> -			had_error = 1;
> +		else {
> +			if (verify_and_format_tag(sha1, name, NULL, flags))
> +				had_error = 1;
> +		}

Revert the unnecessary reformatting here.

> @@ -56,6 +57,15 @@ int gpg_verify_tag(const unsigned char *sha1, const char *name_to_report,
>  	ret = run_gpg_verify(buf, size, flags);
>  
>  	free(buf);
> +
> +	if (fmt_pretty) {
> +		struct ref_array_item *ref_item;
> +		ref_item = new_ref_item(name_to_report, sha1, 0);
> +		ref_item->kind = FILTER_REFS_TAGS;
> +		show_ref_item(ref_item, fmt_pretty, 0);
> +		free_ref_item(ref_item);
> +	}

I haven't seen 5/6 and 6/6, but if this is the only user of the 3/6,
it would be much better to have a single function to format a ref
exported from ref-filter.[ch] so that this one can say

	if (fmt_pretty)
        	format_ref(name_to_report, sha1, FILTER_REFS_TAGS);

or something like that, instead of doing three that will always be
used together in quick succession in the above pattern.

Re: [PATCH 4/6] tag: add format specifier to gpg_verify_tag

[Santiago Torres]

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

On Thu, Sep 22, 2016 at 01:58:06PM -0700, Junio C Hamano wrote:
> santiago@nyu.edu writes:
> 
> > 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.
> 
> > diff --git a/builtin/tag.c b/builtin/tag.c
> > index dbf271f..94ed8a2 100644
> > --- a/builtin/tag.c
> > +++ b/builtin/tag.c
> > @@ -106,7 +106,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..7a1121b 100644
> > --- a/builtin/verify-tag.c
> > +++ b/builtin/verify-tag.c
> > @@ -51,8 +51,10 @@ 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))
> > -			had_error = 1;
> > +		else {
> > +			if (verify_and_format_tag(sha1, name, NULL, flags))
> > +				had_error = 1;
> > +		}
> 
> Revert the unnecessary reformatting here.
> 
> > @@ -56,6 +57,15 @@ int gpg_verify_tag(const unsigned char *sha1, const char *name_to_report,
> >  	ret = run_gpg_verify(buf, size, flags);
> >  
> >  	free(buf);
> > +
> > +	if (fmt_pretty) {
> > +		struct ref_array_item *ref_item;
> > +		ref_item = new_ref_item(name_to_report, sha1, 0);
> > +		ref_item->kind = FILTER_REFS_TAGS;
> > +		show_ref_item(ref_item, fmt_pretty, 0);
> > +		free_ref_item(ref_item);
> > +	}
> 
> I haven't seen 5/6 and 6/6, but if this is the only user of the 3/6,
> it would be much better to have a single function to format a ref
> exported from ref-filter.[ch] so that this one can say
> 
> 	if (fmt_pretty)
>         	format_ref(name_to_report, sha1, FILTER_REFS_TAGS);
> 
> or something like that, instead of doing three that will always be
> used together in quick succession in the above pattern.

Oh, this sounds like a better alternative. This would be instead of 0003
right? 

Thanks,
-Santiago.

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

[PATCH 5/6] 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 7a1121b..319d469 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
 };
 
+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,13 +50,18 @@ 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))
+			if (verify_and_format_tag(sha1, name, fmt_pretty, flags))
 				had_error = 1;
 		}
 	}
-- 
2.10.0

Re: [PATCH 5/6] 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 7a1121b..319d469 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
>  };
>  
> +char *fmt_pretty;

Does this have to be extern?  I do not think so; prepend "static "
in front of it.

>  	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))
> +			if (verify_and_format_tag(sha1, name, fmt_pretty, flags))

OK.  The callchain from here is

    verify_and_format_tag()
    -> run_gpg_verify()
      -> print_signature_buffer()

so not cramming QUIET into the flags parameter that is already
passed is cumbersome.  As I said in my earlier review, it would make
more sense to have the conditional NOT in print_signature_buffer()
but in its caller, but it still is OK to add GPG_VERIFY_QUIET bit
to the flag, which you would check in run_gpg_verify() to decide not
to call print_signature_buffer().

Re: [PATCH 5/6] builtin/verify-tag: Add --format to verify-tag

[Santiago Torres]

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

On Thu, Sep 22, 2016 at 02:16:21PM -0700, Junio C Hamano wrote:
> 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 7a1121b..319d469 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
> >  };
> >  
> > +char *fmt_pretty;
> 
> Does this have to be extern?  I do not think so; prepend "static "
> in front of it.
> 
> >  	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))
> > +			if (verify_and_format_tag(sha1, name, fmt_pretty, flags))
> 
> OK.  The callchain from here is
> 
>     verify_and_format_tag()
>     -> run_gpg_verify()
>       -> print_signature_buffer()
> 
> so not cramming QUIET into the flags parameter that is already
> passed is cumbersome.  As I said in my earlier review, it would make
> more sense to have the conditional NOT in print_signature_buffer()
> but in its caller, but it still is OK to add GPG_VERIFY_QUIET bit
> to the flag, which you would check in run_gpg_verify() to decide not
> to call print_signature_buffer().
> 

Yeah, in retrospect, this sounds like a more reasonable approach than
doing it on gpg-nterface. I'll keep the QUIET bit then.

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

[PATCH 6/6] 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.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
---
 builtin/tag.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 94ed8a2..3dd1e65 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
 };
 
@@ -106,7 +106,13 @@ 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 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)
@@ -425,8 +431,11 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		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')
+	if (cmdmode == 'v') {
+		if (fmt_pretty)
+			verify_ref_format(fmt_pretty);
 		return for_each_tag_name(argv, verify_tag);
+	}
 
 	if (msg.given || msgfile) {
 		if (msg.given && msgfile)
-- 
2.10.0

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

[Junio C Hamano]

santiago@nyu.edu writes:

> @@ -425,8 +431,11 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  		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')
> +	if (cmdmode == 'v') {
> +		if (fmt_pretty)
> +			verify_ref_format(fmt_pretty);
>  		return for_each_tag_name(argv, verify_tag);
> +	}

OK, you said something about for_each_ref() in an earlier commit,
but what you meant was this one, which takes each_tag_name_fn.

The function for_each_tag_name(), the type each_tag_name_fn, and the
function of that type verify_tag(), are ALL file-scope static in
this single file, builtin/tag.c.  It seems to me that it is not
necessary to make the format string global at all.

> @@ -425,8 +431,11 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  		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')
> +	if (cmdmode == 'v') {
> +		if (fmt_pretty)
> +			verify_ref_format(fmt_pretty);
>  		return for_each_tag_name(argv, verify_tag);
> +	}

There are minor implementation and design issues I spotted, but
overall I think the feature the series attempts to add may be a good
thing to have.

Thanks.

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

[Santiago Torres]

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

> OK, you said something about for_each_ref() in an earlier commit,
> but what you meant was this one, which takes each_tag_name_fn.

Oh yeah, sorry for the confusion.

> 
> The function for_each_tag_name(), the type each_tag_name_fn, and the
> function of that type verify_tag(), are ALL file-scope static in
> this single file, builtin/tag.c.  It seems to me that it is not
> necessary to make the format string global at all.

Oh, ok. I was thinking that this was preferred over changing the
signature of those functions. (I drew my conclusion from log.c). I'll
take this other road then.
> 
> ... 
> 
> There are minor implementation and design issues I spotted, but
> overall I think the feature the series attempts to add may be a good
> thing to have.
> 

Thanks for the review! I'll re-roll shortly.
-Santiago. 


> Thanks.

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

Re: [RFC/PATCH 0/6] Add --format to tag verification

[Stefan Beller]

On Thu, Sep 22, 2016 at 11:53 AM,  <santiago@nyu.edu> wrote:

>
> P.S. Gmane seems to be broken for git after it was rebooted. Should we ping
> them about it?

I think most of the git developers have moved on and reference emails by
message id. An archive of all messages of the mailing list is found at

    public-inbox.org/git/

(You can git-clone it to have a distributed copy of the whole archive)

    public-inbox.org/git/<message-id>/
    public-inbox.org/git/<message-id>/raw

is a good point to link to.

However, feel free to ping gmane. :)

Thanks,
Stefan

Re: [RFC/PATCH 0/6] Add --format to tag verification

[Jakub Narębski]

W dniu 22.09.2016 o 21:01, Stefan Beller pisze:
> On Thu, Sep 22, 2016 at 11:53 AM,  <santiago@nyu.edu> wrote:
> 
>>
>> P.S. Gmane seems to be broken for git after it was rebooted. Should we ping
>> them about it?
> 
> I think most of the git developers have moved on and reference emails by
> message id. An archive of all messages of the mailing list is found at
> 
>     public-inbox.org/git/
> 
> (You can git-clone it to have a distributed copy of the whole archive)
> 
>     public-inbox.org/git/<message-id>/
>     public-inbox.org/git/<message-id>/raw
> 
> is a good point to link to.
> 
> However, feel free to ping gmane. :)

Two relevant articles are the following:
 [1]: https://lwn.net/Articles/695695/
 [2]: https://lwn.net/Articles/699704/

In short: Gmane creator and maintainer wanted to retire from being
maintainer[1] because of DDoS attack against its web interface, so he
stopped supporting wen interface (NNTP continues working). Thus
public-inbox.org was created (or just advertised more). Later
Gmane got new maintainer[2], but without code for web interface
(I don't know why it is).

So there is hope that Gmane web interface would be up some time in
the future; in meantime one can use public-inbox URLs.

HTH,
-- 
Jakub Narębski