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

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

[santiago]

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.

Signed-off-by: Santiago Torres <santiago@nyu.edu>
---
 builtin/tag.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

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};
 
 	if (run_command_v_opt(argv_verify_tag, RUN_GIT_CMD))
 		return error(_("could not verify the tag '%s'"), name);
-- 
2.7.0.435.g70bd996.dirty

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

[Jeff King]

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

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

> 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

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

[Santiago Torres]

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.

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

[Jeff King]

On Sat, Feb 27, 2016 at 12:45:24PM -0500, Santiago Torres wrote:

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

Sure.

I suspect the extra process is there for historical reasons; git-tag was
originally a shell script that called out to git-verify-tag, and the
conversion to C retained the separate call.

I cannot think of a reason that it would be a bad thing to do it all in
a single process. Do note the trickery with SIGPIPE in verify-tag,
though. We probably need to do the same here (in fact, I wonder if that
should be pushed down into the code that calls gpg).

-Peff

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

[Santiago Torres]

Hi Peff.

I've been trying to shape these changes into sensible patch, but it is
not as trivial as I originally thought. I think the issue lies in the
tag desambiguation aspect of the git-tag command.

It seems that verify-tag can take either the refname or the hash of the
object. However, git tag --verify takes only the refname, so it doesn't
resolve the tag-sha1 if that's specified as an argument. 

I'm wondering if this is what we would want in the updated patch (accept
sha1's also). If so, does the following make sense?

1) if arg not in .git/tag/refs
2) then try to resolve using get_sha1(name, sha1) take it from there.

Also, would it make sense to remove the verify-tag command altogether? 
On the same line, it seems that there used to be a --raw flag on the
verify-tag command, should I propagate this to git tag --verify?

Thanks!
-Santiago.



On Sat, Feb 27, 2016 at 01:31:33PM -0500, Jeff King wrote:
> On Sat, Feb 27, 2016 at 12:45:24PM -0500, Santiago Torres wrote:
> 
> > > 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?
> 
> Sure.
> 
> I suspect the extra process is there for historical reasons; git-tag was
> originally a shell script that called out to git-verify-tag, and the
> conversion to C retained the separate call.
> 
> I cannot think of a reason that it would be a bad thing to do it all in
> a single process. Do note the trickery with SIGPIPE in verify-tag,
> though. We probably need to do the same here (in fact, I wonder if that
> should be pushed down into the code that calls gpg).
> 
> -Peff

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

[Jeff King]

On Thu, Mar 03, 2016 at 05:05:03PM -0500, Santiago Torres wrote:

> I've been trying to shape these changes into sensible patch, but it is
> not as trivial as I originally thought. I think the issue lies in the
> tag desambiguation aspect of the git-tag command.
> 
> It seems that verify-tag can take either the refname or the hash of the
> object. However, git tag --verify takes only the refname, so it doesn't
> resolve the tag-sha1 if that's specified as an argument.

Right. Git-tag's arguments are tag-names, _not_ general sha1
expressions. So we look at "refs/tags/<whatever>", and nothing else. I
think this should remain the case. Even though it may seem like a
convenience to fall back to resolving the sha1, I think it introduces
unexpected corner cases.

> Also, would it make sense to remove the verify-tag command altogether?

No, I don't think so, for two reasons.

One is simply that it would break backwards compatibility. Verify-tag is
the advertised "plumbing" command for scripts to use, and we do not want
to break them. So even if its features were totally subsumed by "git tag
--verify", we would keep it anyway.

The second is that I don't think it is quite the same thing as "tag
--verify". Verify-tag is plumbing for operating on a tag object; that's
why it takes an arbitrary sha1 expression. But git-tag is a general
command for operating on tag-names defined in refs/tags. We've already
seen one difference there (how we resolve the arguments), but as time
goes on, there may be others. E.g., "tag --verify" may learn to validate
additional elements of the tag, like whether the refname matches what is
in the signed object (that's just an example; I don't know if it's a
good idea or not, but I just meant to illustrate the conceptual
difference between the two).

> On the same line, it seems that there used to be a --raw flag on the
> verify-tag command, should I propagate this to git tag --verify?

I'm not sure if it is necessary. It's primarily for machine consumption,
and in that case, I'd expect people to use the verify-tag plumbing.

-Peff