{"thread":{"id":"41546","subject":"[PATCH/RFC] builtin/tag: Changes argument format for verify","startedAt":"2016-02-27T00:27:44Z","lastAt":"2016-03-03T22:26:36Z","messageCount":6,"participants":["santiago@nyu.edu","Jeff King","Santiago Torres"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"279594","messageId":"1456532864-30327-1-git-send-email-santiago@nyu.edu","threadId":"41546","inReplyTo":null,"subject":"[PATCH/RFC] builtin/tag: Changes argument format for verify","fromName":"","fromEmail":"santiago@nyu.edu","sentAt":"2016-02-27T00:27:44Z","receivedAt":"2016-02-27T00:27:44Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"From: Santiago Torres <santiago@nyu.edu>\n\nThe verify tag function converts the commit sha1 to hex and passes it as\na command-line argument to builtin/verify-tag. Given that builtin/verify-tag\nalready resolves the ref name sha1 equivalent, the sha1 to\nhex_sha1 conversion is unnecessary and the ref-name can be used instead.\n\nSigned-off-by: Santiago Torres <santiago@nyu.edu>\n---\n builtin/tag.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 1705c94..5de1161 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -105,8 +105,7 @@ static int verify_tag(const char *name, const char *ref,\n \t\t\t\tconst unsigned char *sha1)\n {\n \tconst char *argv_verify_tag[] = {\"verify-tag\",\n-\t\t\t\t\t\"-v\", \"SHA1_HEX\", NULL};\n-\targv_verify_tag[2] = sha1_to_hex(sha1);\n+\t\t\t\t\t\"-v\", name, NULL};\n \n \tif (run_command_v_opt(argv_verify_tag, RUN_GIT_CMD))\n \t\treturn error(_(\"could not verify the tag '%s'\"), name);\n-- \n2.7.0.435.g70bd996.dirty\n"},{"id":"279603","messageId":"20160227043625.GC11604@sigill.intra.peff.net","threadId":"41546","inReplyTo":"1456532864-30327-1-git-send-email-santiago@nyu.edu","subject":"Re: [PATCH/RFC] builtin/tag: Changes argument format for verify","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-27T04:36:25Z","receivedAt":"2016-02-27T04:36:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 26, 2016 at 07:27:44PM -0500, santiago@nyu.edu wrote:\n\n> From: Santiago Torres <santiago@nyu.edu>\n> \n> The verify tag function converts the commit sha1 to hex and passes it as\n> a command-line argument to builtin/verify-tag. Given that builtin/verify-tag\n> already resolves the ref name sha1 equivalent, the sha1 to\n> hex_sha1 conversion is unnecessary and the ref-name can be used instead.\n\nHrm. This is potentially racy, if git-tag is going to say something\nabout the ref, but git-verify-tag may have actually verified another tag\nentirely.\n\nAFAICT, though, git-tag doesn't say anything, and is just purely\nforwarding work to verify-tag. So I can't see a real downside to passing\nin the ref name, except that it is slightly less efficient (because\nverify_tag has to re-resolve it). But...\n\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index 1705c94..5de1161 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -105,8 +105,7 @@ static int verify_tag(const char *name, const char *ref,\n>  \t\t\t\tconst unsigned char *sha1)\n>  {\n>  \tconst char *argv_verify_tag[] = {\"verify-tag\",\n> -\t\t\t\t\t\"-v\", \"SHA1_HEX\", NULL};\n> -\targv_verify_tag[2] = sha1_to_hex(sha1);\n> +\t\t\t\t\t\"-v\", name, NULL};\n\nYou are passing in \"name\" here, not \"ref\". git-tag knows it is operating\nspecifically on tags, and completes a name like \"foo\" to\n\"refs/tags/foo\". Whereas verify-tag is plumbing that can operate on any\nref, and will do the usual lookup for \"foo\", \"refs/heads/foo\",\n\"refs/tags/foo\", etc.\n\nSo by passing the unqualified name, we may end up finding something\nentirely different, generating \"ambiguous name\" errors, etc. So if we\n_were_ to go this route, I think we'd need to use \"ref\" here, not\n\"name\".\n\nBut I'm not really sure I see the upside.\n\nA much more interesting change in this area, I think, would be to skip\nverify-tag entirely. Once upon a time it had a lot of logic itself, but\nthese days it is a thin wrapper over run_gpg_verify(), and we could\nimprove the efficiency quite a bit by eliminates the sub-process\nentirely.\n\n-Peff\n"},{"id":"279653","messageId":"20160227174523.GB11593@LykOS","threadId":"41546","inReplyTo":"20160227043625.GC11604@sigill.intra.peff.net","subject":"Re: [PATCH/RFC] builtin/tag: Changes argument format for verify","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2016-02-27T17:45:24Z","receivedAt":"2016-02-27T17:45:24Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"Hello Jeff, thanks for going through the patch.\n \n> > diff --git a/builtin/tag.c b/builtin/tag.c\n> > index 1705c94..5de1161 100644\n> > --- a/builtin/tag.c\n> > +++ b/builtin/tag.c\n> > @@ -105,8 +105,7 @@ static int verify_tag(const char *name, const char *ref,\n> >  \t\t\t\tconst unsigned char *sha1)\n> >  {\n> >  \tconst char *argv_verify_tag[] = {\"verify-tag\",\n> > -\t\t\t\t\t\"-v\", \"SHA1_HEX\", NULL};\n> > -\targv_verify_tag[2] = sha1_to_hex(sha1);\n> > +\t\t\t\t\t\"-v\", name, NULL};\n> \n> You are passing in \"name\" here, not \"ref\". git-tag knows it is operating\n> specifically on tags, and completes a name like \"foo\" to\n> \"refs/tags/foo\". Whereas verify-tag is plumbing that can operate on any\n> ref, and will do the usual lookup for \"foo\", \"refs/heads/foo\",\n> \"refs/tags/foo\", etc.\n> \n> So by passing the unqualified name, we may end up finding something\n> entirely different, generating \"ambiguous name\" errors, etc. So if we\n> _were_ to go this route, I think we'd need to use \"ref\" here, not\n> \"name\".\n\nYeah, you are right. I found this little detail while going through the\ncode yesterday, and I thought it was odd at first and \"fixed\" it. Given\nthat it worked for me (and tests pass) I thought I was actually removing\none function call. Howerver, as you point out, it is less efficient\nbecause the resolution is done twice.\n\nI read the log regarding this file and I didn't quite get what was all\nthe issue with disambiguation when I was submitting. After reading your\nemail, it's clear why things are done in this way now.\n\n> \n> But I'm not really sure I see the upside.\n> \n> A much more interesting change in this area, I think, would be to skip\n> verify-tag entirely. Once upon a time it had a lot of logic itself, but\n> these days it is a thin wrapper over run_gpg_verify(), and we could\n> improve the efficiency quite a bit by eliminates the sub-process\n> entirely.\n\nI agree here too. while going through gdb to follow the logic on this I saw that\nthis code forks three times (git, tag-verify and gpg). I'm sure that\nremoving one layer should be good efficiencly-wise.\n\nIs it ok if I give this a shot?\n\nThanks!\n-Santiago.\n"},{"id":"279660","messageId":"20160227183133.GB12822@sigill.intra.peff.net","threadId":"41546","inReplyTo":"20160227174523.GB11593@LykOS","subject":"Re: [PATCH/RFC] builtin/tag: Changes argument format for verify","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-27T18:31:33Z","receivedAt":"2016-02-27T18:31:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 27, 2016 at 12:45:24PM -0500, Santiago Torres wrote:\n\n> > A much more interesting change in this area, I think, would be to skip\n> > verify-tag entirely. Once upon a time it had a lot of logic itself, but\n> > these days it is a thin wrapper over run_gpg_verify(), and we could\n> > improve the efficiency quite a bit by eliminates the sub-process\n> > entirely.\n> \n> I agree here too. while going through gdb to follow the logic on this I saw that\n> this code forks three times (git, tag-verify and gpg). I'm sure that\n> removing one layer should be good efficiencly-wise.\n> \n> Is it ok if I give this a shot?\n\nSure.\n\nI suspect the extra process is there for historical reasons; git-tag was\noriginally a shell script that called out to git-verify-tag, and the\nconversion to C retained the separate call.\n\nI cannot think of a reason that it would be a bad thing to do it all in\na single process. Do note the trickery with SIGPIPE in verify-tag,\nthough. We probably need to do the same here (in fact, I wonder if that\nshould be pushed down into the code that calls gpg).\n\n-Peff\n"},{"id":"280152","messageId":"20160303220502.GA2234@LykOS","threadId":"41546","inReplyTo":"20160227183133.GB12822@sigill.intra.peff.net","subject":"Re: [PATCH/RFC] builtin/tag: Changes argument format for verify","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2016-03-03T22:05:03Z","receivedAt":"2016-03-03T22:05:03Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"Hi Peff.\n\nI've been trying to shape these changes into sensible patch, but it is\nnot as trivial as I originally thought. I think the issue lies in the\ntag desambiguation aspect of the git-tag command.\n\nIt seems that verify-tag can take either the refname or the hash of the\nobject. However, git tag --verify takes only the refname, so it doesn't\nresolve the tag-sha1 if that's specified as an argument. \n\nI'm wondering if this is what we would want in the updated patch (accept\nsha1's also). If so, does the following make sense?\n\n1) if arg not in .git/tag/refs\n2) then try to resolve using get_sha1(name, sha1) take it from there.\n\nAlso, would it make sense to remove the verify-tag command altogether? \nOn the same line, it seems that there used to be a --raw flag on the\nverify-tag command, should I propagate this to git tag --verify?\n\nThanks!\n-Santiago.\n\n\n\nOn Sat, Feb 27, 2016 at 01:31:33PM -0500, Jeff King wrote:\n> On Sat, Feb 27, 2016 at 12:45:24PM -0500, Santiago Torres wrote:\n> \n> > > A much more interesting change in this area, I think, would be to skip\n> > > verify-tag entirely. Once upon a time it had a lot of logic itself, but\n> > > these days it is a thin wrapper over run_gpg_verify(), and we could\n> > > improve the efficiency quite a bit by eliminates the sub-process\n> > > entirely.\n> > \n> > I agree here too. while going through gdb to follow the logic on this I saw that\n> > this code forks three times (git, tag-verify and gpg). I'm sure that\n> > removing one layer should be good efficiencly-wise.\n> > \n> > Is it ok if I give this a shot?\n> \n> Sure.\n> \n> I suspect the extra process is there for historical reasons; git-tag was\n> originally a shell script that called out to git-verify-tag, and the\n> conversion to C retained the separate call.\n> \n> I cannot think of a reason that it would be a bad thing to do it all in\n> a single process. Do note the trickery with SIGPIPE in verify-tag,\n> though. We probably need to do the same here (in fact, I wonder if that\n> should be pushed down into the code that calls gpg).\n> \n> -Peff\n"},{"id":"280156","messageId":"20160303222636.GA26712@sigill.intra.peff.net","threadId":"41546","inReplyTo":"20160303220502.GA2234@LykOS","subject":"Re: [PATCH/RFC] builtin/tag: Changes argument format for verify","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-03T22:26:36Z","receivedAt":"2016-03-03T22:26:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 03, 2016 at 05:05:03PM -0500, Santiago Torres wrote:\n\n> I've been trying to shape these changes into sensible patch, but it is\n> not as trivial as I originally thought. I think the issue lies in the\n> tag desambiguation aspect of the git-tag command.\n> \n> It seems that verify-tag can take either the refname or the hash of the\n> object. However, git tag --verify takes only the refname, so it doesn't\n> resolve the tag-sha1 if that's specified as an argument.\n\nRight. Git-tag's arguments are tag-names, _not_ general sha1\nexpressions. So we look at \"refs/tags/<whatever>\", and nothing else. I\nthink this should remain the case. Even though it may seem like a\nconvenience to fall back to resolving the sha1, I think it introduces\nunexpected corner cases.\n\n> Also, would it make sense to remove the verify-tag command altogether?\n\nNo, I don't think so, for two reasons.\n\nOne is simply that it would break backwards compatibility. Verify-tag is\nthe advertised \"plumbing\" command for scripts to use, and we do not want\nto break them. So even if its features were totally subsumed by \"git tag\n--verify\", we would keep it anyway.\n\nThe second is that I don't think it is quite the same thing as \"tag\n--verify\". Verify-tag is plumbing for operating on a tag object; that's\nwhy it takes an arbitrary sha1 expression. But git-tag is a general\ncommand for operating on tag-names defined in refs/tags. We've already\nseen one difference there (how we resolve the arguments), but as time\ngoes on, there may be others. E.g., \"tag --verify\" may learn to validate\nadditional elements of the tag, like whether the refname matches what is\nin the signed object (that's just an example; I don't know if it's a\ngood idea or not, but I just meant to illustrate the conceptual\ndifference between the two).\n\n> On the same line, it seems that there used to be a --raw flag on the\n> verify-tag command, should I propagate this to git tag --verify?\n\nI'm not sure if it is necessary. It's primarily for machine consumption,\nand in that case, I'd expect people to use the verify-tag plumbing.\n\n-Peff\n"}]}