Hello Jeff, thanks for going through the patch.
> > 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html