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

From: Santiago Torres <santiago@nyu.edu>
Date: Feb 27, 2016, 17:45 UTC
Message-ID: <20160227174523.GB11593@LykOS>
In-Reply-To: <20160227043625.GC11604@sigill.intra.peff.net>

Hello Jeff, thanks for going through the patch.

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

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.

Show 8 quoted lines

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