threads / rfc / 41546

RFC patchbuiltin/tag: Changes argument format for verify

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

## tl;dr

6 messages between Feb 27, 2016 and Mar 3, 2016. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

santiago@nyu.edu· Feb 27, 2016, 00:27 UTC · lore
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(-)
Show changes to builtin/tag.c +1 −2
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
Jeff King· Feb 27, 2016, 04:36 UTC · re: santiago@nyu.edu · lore

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

On Fri, Feb 26, 2016 at 07:27:44PM -0500, santiago@nyu.edu wrote:
Show 6 quoted lines
> 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...

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

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
Santiago Torres· Feb 27, 2016, 17:45 UTC · re: Jeff King · lore

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

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.

Jeff King· Feb 27, 2016, 18:31 UTC · re: Santiago Torres · lore

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

On Sat, Feb 27, 2016 at 12:45:24PM -0500, Santiago Torres wrote:
Show 11 quoted lines
> > 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
Santiago Torres· Mar 3, 2016, 22:05 UTC · re: Jeff King · lore

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

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:
Show 26 quoted lines
> 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
Jeff King· Mar 3, 2016, 22:26 UTC · re: Santiago Torres · lore

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

On Thu, Mar 03, 2016 at 05:05:03PM -0500, Santiago Torres wrote:
Show 7 quoted lines
> 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

← back to recent threads