{"thread":{"id":"44923","subject":"[PATCH] show-ref: Allow --head to work with --verify","startedAt":"2017-01-20T16:12:53Z","lastAt":"2017-01-21T00:25:44Z","messageCount":11,"participants":["Vladimir Panteleev","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"309840","messageId":"20170120155015.4360-1-git@thecybershadow.net","threadId":"44923","inReplyTo":null,"subject":"[PATCH] show-ref: Allow --head to work with --verify","fromName":"Vladimir Panteleev","fromEmail":"git@thecybershadow.net","sentAt":"2017-01-20T15:50:15Z","receivedAt":"2017-01-20T16:12:53Z","isPatch":true,"sender":{"key":"git@thecybershadow.net","avatar":"https://avatars.githubusercontent.com/u/160894?v=4"},"body":"Previously, when --verify was specified, --head was being ignored, and\n\"show-ref --verify --head HEAD\" would always print \"fatal: 'HEAD' -\nnot a valid ref\". As such, when using show-ref to look up any\nref (including HEAD) precisely by name, one would have to special-case\nlooking up the HEAD ref.\n\nThis patch adds --head support to show-ref's --verify logic, by\nexplicitly checking if the \"HEAD\" ref is specified when --head is\npresent.\n\nSigned-off-by: Vladimir Panteleev <git@thecybershadow.net>\n---\n builtin/show-ref.c  | 2 ++\n t/t1403-show-ref.sh | 8 ++++++++\n 2 files changed, 10 insertions(+)\n\ndiff --git a/builtin/show-ref.c b/builtin/show-ref.c\nindex 6d4e66900..ee5078604 100644\n--- a/builtin/show-ref.c\n+++ b/builtin/show-ref.c\n@@ -207,6 +207,8 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)\n \t\t\t\tif (!quiet)\n \t\t\t\t\tshow_one(*pattern, &oid);\n \t\t\t}\n+\t\t\telse if (show_head && !strcmp(*pattern, \"HEAD\"))\n+\t\t\t\thead_ref(show_ref, NULL);\n \t\t\telse if (!quiet)\n \t\t\t\tdie(\"'%s' - not a valid ref\", *pattern);\n \t\t\telse\ndiff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh\nindex 7e10bcfe3..de64ebfb7 100755\n--- a/t/t1403-show-ref.sh\n+++ b/t/t1403-show-ref.sh\n@@ -164,4 +164,12 @@ test_expect_success 'show-ref --heads, --tags, --head, pattern' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'show-ref --verify --head' '\n+\t{\n+\t\techo $(git rev-parse HEAD) HEAD\n+\t} >expect &&\n+\tgit show-ref --verify --head HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.11.0\n\n"},{"id":"309847","messageId":"xmqqa8aly2o4.fsf@gitster.mtv.corp.google.com","threadId":"44923","inReplyTo":"20170120155015.4360-1-git@thecybershadow.net","subject":"Re: [PATCH] show-ref: Allow --head to work with --verify","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-20T19:03:23Z","receivedAt":"2017-01-20T19:03:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vladimir Panteleev <git@thecybershadow.net> writes:\n\n> This patch adds --head support to show-ref's --verify logic, by\n> explicitly checking if the \"HEAD\" ref is specified when --head is\n> present.\n\n> @@ -207,6 +207,8 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)\n>  \t\t\t\tif (!quiet)\n>  \t\t\t\t\tshow_one(*pattern, &oid);\n>  \t\t\t}\n> +\t\t\telse if (show_head && !strcmp(*pattern, \"HEAD\"))\n> +\t\t\t\thead_ref(show_ref, NULL);\n>  \t\t\telse if (!quiet)\n>  \t\t\t\tdie(\"'%s' - not a valid ref\", *pattern);\n>  \t\t\telse\n\nThe context around here look like this:\n\n\t\twhile (*pattern) {\n\t\t\tstruct object_id oid;\n\n\t\t\tif (starts_with(*pattern, \"refs/\") &&\n\t\t\t    !read_ref(*pattern, oid.hash)) {\n\t\t\t\tif (!quiet)\n\t\t\t\t\tshow_one(*pattern, &oid);\n\t\t\t}\n\t\t\telse if (!quiet)\n\t\t\t\tdie(\"'%s' - not a valid ref\", *pattern);\n\t\t\telse\n\t\t\t\treturn 1;\n\t\t\tpattern++;\n\t\t}\n\nand viewed in the wider context, I notice that quiet is not honored\nin the added code.  I think that is easily fixable by replacing this\nhunk with something like:\n\n-\tif (starts_with(*pattern, \"refs/\") &&\n+\tif (to_show_ref(*pattern) &&\n\nand then another hunk that implements to_show_ref() helper function,\nperhaps like\n\n\tstatic int to_show_ref(const char *pattern)\n\t{\n\t\tif (starts_with(pattern, \"refs/\"))\n\t\t\treturn 1;\n\t\tif (show_head && !strcmp(pattern, \"HEAD\"))\n\t\t\treturn 1;\n\t\treturn 0;\n\t}\n\nor something.\n\nHaving said all that, using --verify on HEAD does not make much\nsense, because if HEAD is missing in .git/, I do not think Git\nconsiders that directory as a Git repository to begin with.  So from\nthat point of view, I am not sure what value this change adds to the\nsystem, even though the change is almost correct (modulo the \"quiet\"\nthing).\n"},{"id":"309856","messageId":"1bf9a446-0b00-f27a-4625-0bc8c25356fe@gmail.com","threadId":"44923","inReplyTo":"xmqqa8aly2o4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] show-ref: Allow --head to work with --verify","fromName":"Vladimir Panteleev","fromEmail":"thecybershadow@gmail.com","sentAt":"2017-01-20T20:26:44Z","receivedAt":"2017-01-20T20:27:44Z","isPatch":true,"sender":{"key":"thecybershadow@gmail.com","avatar":"https://gravatar.com/avatar/3e299714bf51dc14d421243faf5bea230d483e2a82fc049a717ecccaf36accb4?d=mp&s=160"},"body":"Hi Junio,\n\nOn 2017-01-20 19:03, Junio C Hamano wrote:\n> Having said all that, using --verify on HEAD does not make much\n> sense, because if HEAD is missing in .git/, I do not think Git\n> considers that directory as a Git repository to begin with.  So from\n> that point of view, I am not sure what value this change adds to the\n> system, even though the change is almost correct (modulo the \"quiet\"\n> thing).\n\nMy use case was the following series of steps:\n\nQ1: How do I resolve a full ref name to a commit SHA1?\nA1: Use show-ref <full-ref-name>.\n\nQ2: How to make git show-ref also work when HEAD is specified as the \nreference?\nA2: Add --head.\n\nQ3: How do I make git show-ref only look for the exact full ref name \nspecified, instead of doing a pattern/substring search, and thus output \nat most one result?\nA3: Add --verify.\n\nHowever, A2 and A3 are incompatible. Thus, there doesn't seem to be a \nway to e.g. make a simple alias which looks up a ref only by its full \nref name, where said ref might or might not be HEAD. The obvious \nworkaround is to check if the ref is HEAD in the rev-parse caller, \nhowever it seemed more logical to fix it in git instead.\n\nThe documentation for show-ref also makes no mention that --head is \nignored if --verify is specified, and the combination was not covered by \nany tests, therefore this seemed to me as a simple omission in \n--verify's logic.\n\nThere is also rev-parse, which also has a --verify switch, however it \ndoes something else, and I don't see a way to ask rev-parse to only \nconsider full refs.\n\n-- \nBest regards,\n  Vladimir\n"},{"id":"309857","messageId":"cf40c3e4-406d-f39f-ab72-812a88115100@gmail.com","threadId":"44923","inReplyTo":"1bf9a446-0b00-f27a-4625-0bc8c25356fe@gmail.com","subject":"Re: [PATCH] show-ref: Allow --head to work with --verify","fromName":"Vladimir Panteleev","fromEmail":"thecybershadow@gmail.com","sentAt":"2017-01-20T20:51:03Z","receivedAt":"2017-01-20T20:52:42Z","isPatch":true,"sender":{"key":"thecybershadow@gmail.com","avatar":"https://gravatar.com/avatar/3e299714bf51dc14d421243faf5bea230d483e2a82fc049a717ecccaf36accb4?d=mp&s=160"},"body":"And to clarify:\n\nOn 2017-01-20 20:26, Vladimir Panteleev wrote:\n> On 2017-01-20 19:03, Junio C Hamano wrote:\n>> Having said all that, using --verify on HEAD does not make much\n>> sense, because if HEAD is missing in .git/, I do not think Git\n>> considers that directory as a Git repository to begin with.\n\nThe behavior of --verify I am interested in is not to check that the ref \nexists, but to get its SHA1 while avoiding the pattern search. This \navoids accidentally matching refs via substring (e.g. \"git show-ref \n--head HEAD\" will print HEAD, but also e.g. refs/remotes/origin/HEAD), \nand for performance reasons (looking up a ref by exact name can be MUCH \nfaster than matching all refs by pattern search, e.g. in one of my \nprojects where I use git as an object store, --verify makes a difference \nof 21 milliseconds vs. over 5 minutes).\n\n-- \nBest regards,\n  Vladimir\n"},{"id":"309865","messageId":"xmqqefzxwew9.fsf@gitster.mtv.corp.google.com","threadId":"44923","inReplyTo":"1bf9a446-0b00-f27a-4625-0bc8c25356fe@gmail.com","subject":"Re: [PATCH] show-ref: Allow --head to work with --verify","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-20T22:22:14Z","receivedAt":"2017-01-20T22:22:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vladimir Panteleev <thecybershadow@gmail.com> writes:\n\n> Hi Junio,\n>\n> On 2017-01-20 19:03, Junio C Hamano wrote:\n>> Having said all that, using --verify on HEAD does not make much\n>> sense, because if HEAD is missing in .git/, I do not think Git\n>> considers that directory as a Git repository to begin with.  So from\n>> that point of view, I am not sure what value this change adds to the\n>> system, even though the change is almost correct (modulo the \"quiet\"\n>> thing).\n>\n> My use case was the following series of steps:\n>\n> Q1: How do I resolve a full ref name to a commit SHA1?\n> A1: Use show-ref <full-ref-name>.\n>\n> Q2: How to make git show-ref also work when HEAD is specified as the\n> reference?\n> A2: Add --head.\n>\n> Q3: How do I make git show-ref only look for the exact full ref name\n> specified, instead of doing a pattern/substring search, and thus\n> output at most one result?\n> A3: Add --verify.\n>\n> However, A2 and A3 are incompatible. Thus, there doesn't seem to be a\n> way to e.g. make a simple alias which looks up a ref only by its full\n> ref name, where said ref might or might not be HEAD. The obvious\n> workaround is to check if the ref is HEAD in the rev-parse caller,\n> however it seemed more logical to fix it in git instead.\n>\n> The documentation for show-ref also makes no mention that --head is\n> ignored if --verify is specified, and the combination was not covered\n> by any tests, therefore this seemed to me as a simple omission in\n> --verify's logic.\n>\n> There is also rev-parse, which also has a --verify switch, however it\n> does something else, and I don't see a way to ask rev-parse to only\n> consider full refs.\n\nYour log message for the patch needs to be updated by summarizing\nthe above better.  I couldn't read the motivation behind the change\nfully from the original (even though I guessed it correctly).\n"},{"id":"309867","messageId":"xmqqa8alwegk.fsf@gitster.mtv.corp.google.com","threadId":"44923","inReplyTo":"xmqqefzxwew9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] show-ref: Allow --head to work with --verify","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-20T22:31:39Z","receivedAt":"2017-01-20T22:31:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> My use case was the following series of steps:\n>> ... long and readable if a bit too verbose description ...\n> Your log message for the patch needs to be updated by summarizing\n> the above better.\n\nThat raises the number of things to improve in the patch to 3 (so\nfar):\n\n * the log message clarification.\n\n * the code change I mentioned already to hook into the existing\n   \"only read the fully specified ref\" part, sharing the actual\n   action (i.e. read_ref() to read it, honor quiet and show it by\n   calling show_one()), instead of adding another clause that does\n   it differently (i.e. you didn't do read_ref/quiet?/show_one)\n\n * add a test to make sure that the command also works sensibly when\n   --quiet is used.\n\nWith all of the above, I think the change makes sense.\n\n"},{"id":"309868","messageId":"3b1d2717-dd7f-2add-b935-3ace6063b258@gmail.com","threadId":"44923","inReplyTo":"xmqqa8aly2o4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] show-ref: Allow --head to work with --verify","fromName":"Vladimir Panteleev","fromEmail":"thecybershadow@gmail.com","sentAt":"2017-01-20T22:55:12Z","receivedAt":"2017-01-20T22:55:19Z","isPatch":true,"sender":{"key":"thecybershadow@gmail.com","avatar":"https://gravatar.com/avatar/3e299714bf51dc14d421243faf5bea230d483e2a82fc049a717ecccaf36accb4?d=mp&s=160"},"body":"On 2017-01-20 19:03, Junio C Hamano wrote:\n> and viewed in the wider context, I notice that quiet is not honored\n> in the added code.  I think that is easily fixable by replacing this\n> hunk with something like:\n\n--quiet will still work correctly with the current patch, because \nshow_ref already checks quiet. Granted, the original --verify code used \nshow_one and not show_ref; however, I don't see a meaningful difference \nbetween calling show_ref and show_one for HEAD, other than a bit of \noverhead, so adding a new function may not be worthwhile. I will still \nadd tests for this; however, in light of this, would you still like me \nto perform the change you requested?\n\n-- \nBest regards,\n  Vladimir\n"},{"id":"309870","messageId":"xmqqwpdpuxuw.fsf@gitster.mtv.corp.google.com","threadId":"44923","inReplyTo":"xmqqefzxwew9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] show-ref: Allow --head to work with --verify","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-20T23:15:35Z","receivedAt":"2017-01-20T23:15:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Your log message for the patch needs to be updated by summarizing\n> the above better.\n\nHere is my attempt.\n\n     \"git show-ref HEAD\" used with \"--verify\" (because the user is\n     not interested in seeing refs/remotes/origin/HEAD), and used\n     with \"--head\" (because the user does not want HEAD to be\n     filtered out), i.e. \"git show-ref --head --verify HEAD\", did\n     not work as expected.\n\n     Instead of insisting that the input begins with \"refs/\", allow\n     \"HEAD\" when \"--head\" is given in the codepath that handles\n     \"--verify\", so that all valid full refnames including HEAD are\n     passed to the same output machinery.\n"},{"id":"309872","messageId":"xmqqshoduxnj.fsf@gitster.mtv.corp.google.com","threadId":"44923","inReplyTo":"3b1d2717-dd7f-2add-b935-3ace6063b258@gmail.com","subject":"Re: [PATCH] show-ref: Allow --head to work with --verify","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-20T23:20:00Z","receivedAt":"2017-01-20T23:20:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vladimir Panteleev <thecybershadow@gmail.com> writes:\n\n> --quiet will still work correctly with the current patch, because\n> show_ref already checks quiet. Granted, the original --verify code\n> used show_one and not show_ref; however, I don't see a meaningful\n> difference between calling show_ref and show_one for HEAD, other than\n> a bit of overhead, so adding a new function may not be worthwhile. I\n> will still add tests for this; however, in light of this, would you\n> still like me to perform the change you requested?\n\nIf two codepaths are called \"I don't see a meaningful difference\",\nthen it is really better to share the same code.  Today, they may\nhappen to behave identically.  When we need to update the behaviour\nof one, we'd be forced to update the other one to match.\n\nIOW, something along this line, perhaps (not even compile tested so\ntake it with grain of salt).\n\nThanks.\n\n builtin/show-ref.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/show-ref.c b/builtin/show-ref.c\nindex 6d4e669002..57491152b7 100644\n--- a/builtin/show-ref.c\n+++ b/builtin/show-ref.c\n@@ -202,7 +202,8 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)\n \t\twhile (*pattern) {\n \t\t\tstruct object_id oid;\n \n-\t\t\tif (starts_with(*pattern, \"refs/\") &&\n+\t\t\tif (((show_head && !strcmp(*pattern, \"HEAD\")) ||\n+\t\t\t     starts_with(*pattern, \"refs/\")) &&\n \t\t\t    !read_ref(*pattern, oid.hash)) {\n \t\t\t\tif (!quiet)\n \t\t\t\t\tshow_one(*pattern, &oid);\n\n\n\n"},{"id":"309874","messageId":"xmqqo9z1ux7r.fsf@gitster.mtv.corp.google.com","threadId":"44923","inReplyTo":"xmqqshoduxnj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] show-ref: Allow --head to work with --verify","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-20T23:29:28Z","receivedAt":"2017-01-20T23:29:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> If two codepaths are called \"I don't see a meaningful difference\",\n> then it is really better to share the same code.  Today, they may\n> happen to behave identically.  When we need to update the behaviour\n> of one, we'd be forced to update the other one to match.\n>\n> IOW, something along this line, perhaps (not even compile tested so\n> take it with grain of salt).\n\nBy the way, I have no strong preference between \"read-ref, check\nquiet and show-one\" and \"show-ref\", so if you make --verify to\nconsistently call \"show_ref()\" for both refs/heads/master and HEAD,\nthat is also perfectly fine.\n\nI just do not want to see the same feature/codepath to call two\ndifferent implementations that happens to do the same thing today.\n\nThanks.\n"},{"id":"309877","messageId":"daef1407-6f08-147d-e23b-bb69bb8fd1ed@gmail.com","threadId":"44923","inReplyTo":"xmqqo9z1ux7r.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] show-ref: Allow --head to work with --verify","fromName":"Vladimir Panteleev","fromEmail":"thecybershadow@gmail.com","sentAt":"2017-01-21T00:25:36Z","receivedAt":"2017-01-21T00:25:44Z","isPatch":true,"sender":{"key":"thecybershadow@gmail.com","avatar":"https://gravatar.com/avatar/3e299714bf51dc14d421243faf5bea230d483e2a82fc049a717ecccaf36accb4?d=mp&s=160"},"body":"On 2017-01-20 23:29, Junio C Hamano wrote:\n> By the way, I have no strong preference between \"read-ref, check\n> quiet and show-one\" and \"show-ref\", so if you make --verify to\n> consistently call \"show_ref()\" for both refs/heads/master and HEAD,\n> that is also perfectly fine.\n\nThis sounds like a good idea, as it will also allow -d and some other \noptions to work with --verify (whatever use case there might be for \nthat). Will resubmit with that.\n\n> I just do not want to see the same feature/codepath to call two\n> different implementations that happens to do the same thing today.\n\nUnderstood and agreed.\n\n-- \nBest regards,\n  Vladimir\n"}]}