patch, 6 partsref-filter: Expose wrappers for ref_item functions

> 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);
> +}

> @@ -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);
> +}

> @@ -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);
> +	}

> @@ -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);
> +	}

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

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

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

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

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

> @@ -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);
> +	}

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

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

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

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

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

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