Re: [PATCH/RFC] builtin/tag: Changes argument format for verify
From: Jeff King <hidden>
Date: 2016-06-15 23:08:30
On Fri, Feb 26, 2016 at 07:27:44PM -0500, santiago@nyu.edu wrote:
From: Santiago Torres <redacted> 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...
quoted hunk ↗ jump to hunk
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