From: Jeff King <peff@peff.net>
Date: Wed, 19 Oct 2016 09:10:44 GMT
Subject: Re: [PATCH v4 5/7] builtin/tag: add --format argument for tag -v
Message-ID: <20161019091044.k3ewbfxjj6ihv5o5@sigill.intra.peff.net>
In-Reply-To: <20161007210721.20437-6-santiago@nyu.edu>

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

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

> +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?

>  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