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

From: Jeff King <peff@peff.net>
Date: Feb 27, 2016, 04:36 UTC
Message-ID: <20160227043625.GC11604@sigill.intra.peff.net>
In-Reply-To: <1456532864-30327-1-git-send-email-santiago@nyu.edu>

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

Show 6 quoted lines

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

Show 11 quoted lines

> 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