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

Hello Jeff, thanks for going through the patch.
 

quoted

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

Yeah, you are right. I found this little detail while going through the
code yesterday, and I thought it was odd at first and "fixed" it. Given
that it worked for me (and tests pass) I thought I was actually removing
one function call. Howerver, as you point out, it is less efficient
because the resolution is done twice.

I read the log regarding this file and I didn't quite get what was all
the issue with disambiguation when I was submitting. After reading your
email, it's clear why things are done in this way now.

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.

I agree here too. while going through gdb to follow the logic on this I saw that
this code forks three times (git, tag-verify and gpg). I'm sure that
removing one layer should be good efficiencly-wise.

Is it ok if I give this a shot?

Thanks!
-Santiago.