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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 27, 2016, 17:50 UTC
Message-ID: <xmqq37klp6yp.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160926224233.32702-6-santiago@nyu.edu>

santiago@nyu.edu writes:

Show 10 quoted lines

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

Show 24 quoted lines

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

Show 15 quoted lines

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

Show 9 quoted lines

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

Show 25 quoted lines

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