Re: [PATCH/RFC] builtin/tag: Changes argument format for verify

[Jeff King <peff@peff.net>]

On Fri, Feb 26, 2016 at 07:27:44PM -0500, santiago@nyu.edu wrote:

> From: Santiago Torres <santiago@nyu.edu>
> 
> The verify tag function converts the commit sha1 to hex and passes it as
> a command-line argument to builtin/verify-tag. Given that builtin/verify-tag
> already resolves the ref name sha1 equivalent, the sha1 to
> hex_sha1 conversion is unnecessary and the ref-name can be used instead.

Hrm. This is potentially racy, if git-tag is going to say something
about the ref, but git-verify-tag may have actually verified another tag
entirely.

AFAICT, though, git-tag doesn't say anything, and is just purely
forwarding work to verify-tag. So I can't see a real downside to passing
in the ref name, except that it is slightly less efficient (because
verify_tag has to re-resolve it). But...

> diff --git a/builtin/tag.c b/builtin/tag.c
> index 1705c94..5de1161 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -105,8 +105,7 @@ static int verify_tag(const char *name, const char *ref,
>  				const unsigned char *sha1)
>  {
>  	const char *argv_verify_tag[] = {"verify-tag",
> -					"-v", "SHA1_HEX", NULL};
> -	argv_verify_tag[2] = sha1_to_hex(sha1);
> +					"-v", name, NULL};

You are passing in "name" here, not "ref". git-tag knows it is operating
specifically on tags, and completes a name like "foo" to
"refs/tags/foo". Whereas verify-tag is plumbing that can operate on any
ref, and will do the usual lookup for "foo", "refs/heads/foo",
"refs/tags/foo", etc.

So by passing the unqualified name, we may end up finding something
entirely different, generating "ambiguous name" errors, etc. So if we
_were_ to go this route, I think we'd need to use "ref" here, not
"name".

But I'm not really sure I see the upside.

A much more interesting change in this area, I think, would be to skip
verify-tag entirely. Once upon a time it had a lot of logic itself, but
these days it is a thin wrapper over run_gpg_verify(), and we could
improve the efficiency quite a bit by eliminates the sub-process
entirely.

-Peff