{"thread":{"id":"34844","subject":"[PATCH v4] peel_onion(): add support for ^{tag}","startedAt":"2013-09-03T19:50:16Z","lastAt":"2013-09-03T20:46:05Z","messageCount":5,"participants":["Richard Hansen","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"226662","messageId":"1378237816-28671-1-git-send-email-rhansen@bbn.com","threadId":"34844","inReplyTo":null,"subject":"[PATCH v4] peel_onion(): add support for ^{tag}","fromName":"Richard Hansen","fromEmail":"rhansen@bbn.com","sentAt":"2013-09-03T19:50:16Z","receivedAt":"2013-09-03T19:50:16Z","isPatch":true,"sender":{"key":"rhansen@rhansen.org","avatar":null},"body":"Complete the ^{} family of object descriptors by having\n^{tag} dereference until a tag object is found (or fail if\nunable).\n\nAt first glance this may not seem very useful, as commits, trees, and\nblobs cannot be peeled to a tag, and a tag would just peel to itself.\nHowever, this can be used to ensure that names a tag object:\n\n $ git rev-parse --verify v1.8.4^{tag}\n 04f013dc38d7512eadb915eba22efc414f18b869\n $ git rev-parse --verify master^{tag}\n error: master^{tag}: expected tag type, but the object dereferences to tree type\n fatal: Needed a single revision\n\nUsers can already ensure that is a tag object by checking the\noutput of 'git cat-file -t ', but:\n * users may expect ^{tag} to exist given that ^{commit},\n ^{tree}, and ^{blob} all exist\n * this syntax is more convenient/natural in some circumstances\n\nSigned-off-by: Richard Hansen \n---\nChanges from v3 (2013-09-03, see\n):\n * Use Peff's simpler test case.\n\n Documentation/revisions.txt | 3 +++\n sha1_name.c | 2 ++\n t/t1511-rev-parse-caret.sh | 7 +++++++\n 3 files changed, 12 insertions(+)\n\ndiff --git a/Documentation/revisions.txt b/Documentation/revisions.txt\nindex d477b3f..b3322ad 100644\n--- a/Documentation/revisions.txt\n+++ b/Documentation/revisions.txt\n@@ -121,6 +121,9 @@ some output processing may assume ref names in UTF-8.\n object that exists, without requiring 'rev' to be a tag, and\n without dereferencing 'rev'; because a tag is already an object,\n it does not have to be dereferenced even once to get to an object.\n++\n+'rev{caret}\\{tag\\}' can be used to ensure that 'rev' identifies an\n+existing tag object.\n \n '{caret}\\{\\}', e.g. 'v0.99.8{caret}\\{\\}'::\n A suffix '{caret}' followed by an empty brace pair\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 65ad066..6dc496d 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -679,6 +679,8 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)\n \tsp++; /* beginning of type name, or closing brace for empty */\n \tif (!strncmp(commit_type, sp, 6) && sp[6] == '}')\n \t\texpected_type = OBJ_COMMIT;\n+\telse if (!strncmp(tag_type, sp, 3) && sp[3] == '}')\n+\t\texpected_type = OBJ_TAG;\n \telse if (!strncmp(tree_type, sp, 4) && sp[4] == '}')\n \t\texpected_type = OBJ_TREE;\n \telse if (!strncmp(blob_type, sp, 4) && sp[4] == '}')\ndiff --git a/t/t1511-rev-parse-caret.sh b/t/t1511-rev-parse-caret.sh\nindex eaefc77..15973f2 100755\n--- a/t/t1511-rev-parse-caret.sh\n+++ b/t/t1511-rev-parse-caret.sh\n@@ -54,6 +54,13 @@ test_expect_success 'ref^{tree}' '\n \ttest_must_fail git rev-parse blob-tag^{tree}\n '\n \n+test_expect_success 'ref^{tag}' '\n+\ttest_must_fail git rev-parse HEAD^{tag} &&\n+\tgit rev-parse commit-tag >expected &&\n+\tgit rev-parse commit-tag^{tag} >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'ref^{/.}' '\n \tgit rev-parse master >expected &&\n \tgit rev-parse master^{/.} >actual &&\n-- \n1.8.4\n"},{"id":"226669","messageId":"xmqq7gexr7i3.fsf@gitster.dls.corp.google.com","threadId":"34844","inReplyTo":"1378237816-28671-1-git-send-email-rhansen@bbn.com","subject":"Re: [PATCH v4] peel_onion(): add support for ^{tag}","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-03T20:10:44Z","receivedAt":"2013-09-03T20:10:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Richard Hansen writes:\n\n> diff --git a/t/t1511-rev-parse-caret.sh b/t/t1511-rev-parse-caret.sh\n> index eaefc77..15973f2 100755\n> --- a/t/t1511-rev-parse-caret.sh\n> +++ b/t/t1511-rev-parse-caret.sh\n> @@ -54,6 +54,13 @@ test_expect_success 'ref^{tree}' '\n> \ttest_must_fail git rev-parse blob-tag^{tree}\n> '\n> \n> +test_expect_success 'ref^{tag}' '\n> +\ttest_must_fail git rev-parse HEAD^{tag} &&\n> +\tgit rev-parse commit-tag >expected &&\n> +\tgit rev-parse commit-tag^{tag} >actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n\nLooks good to me. Testing both failure case and success case.\n\nThanks; will queue.\n"},{"id":"226671","messageId":"20130903202041.GA7463@sigill.intra.peff.net","threadId":"34844","inReplyTo":"1378237816-28671-1-git-send-email-rhansen@bbn.com","subject":"[PATCH 2/1] peel_onion: do not assume length of x_type globals","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-03T20:20:41Z","receivedAt":"2013-09-03T20:20:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we are parsing \"rev^{foo}\", we check \"foo\" against the\nvarious global type strings, like \"commit_type\",\n\"tree_type\", etc. This is nicely abstracted, but then we\ndestroy the abstraction completely by using magic numbers\nthat must match the length of the type strings.\n\nWe can avoid these magic numbers by using skip_prefix. In an\nideal world, the compiler would compute the magic number\nwithout even calling strlen() at runtime. However, it cannot\ndo so in this case because we hide the definition of\ncommit_type and friends behind an extern declaration.\n\nThis shouldn't matter, though, as this is not a performance\ncritical code path. A few short strlens inside get_sha1()\nwill not even be measurable. And if we care, we can adjust\nthe declaration of commit_type and friends later on.\n\nSigned-off-by: Jeff King \n---\nThis is the potential cleanup we discussed. Having written that commit\nmessage, I wonder if it is really even worth using commit_type at all.\nIt is not like that variable can ever change without the git data\nstructure changing, and even if it did, we would almost certainly want\nto revisit how that change affects the user-visible \"rev^{}\" syntax\nanyway.\n\nSo maybe simply:\n\n if (!prefixcmp(\"commit}\"))\n\nwould be fine, and it is way more readable. That's what we do already\nfor \"rev^{object}\".\n\n sha1_name.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 6dc496d..23d0d71 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -652,7 +652,7 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)\n static int peel_onion(const char *name, int len, unsigned char *sha1)\n {\n \tunsigned char outer[20];\n-\tconst char *sp;\n+\tconst char *sp, *x;\n \tunsigned int expected_type = 0;\n \tunsigned lookup_flags = 0;\n \tstruct object *o;\n@@ -677,13 +677,13 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)\n \t\treturn -1;\n \n \tsp++; /* beginning of type name, or closing brace for empty */\n-\tif (!strncmp(commit_type, sp, 6) && sp[6] == '}')\n+\tif ((x = skip_prefix(sp, commit_type)) && *x == '}')\n \t\texpected_type = OBJ_COMMIT;\n-\telse if (!strncmp(tag_type, sp, 3) && sp[3] == '}')\n+\telse if ((x = skip_prefix(sp, tag_type)) && *x == '}')\n \t\texpected_type = OBJ_TAG;\n-\telse if (!strncmp(tree_type, sp, 4) && sp[4] == '}')\n+\telse if ((x = skip_prefix(sp, tree_type)) && *x == '}')\n \t\texpected_type = OBJ_TREE;\n-\telse if (!strncmp(blob_type, sp, 4) && sp[4] == '}')\n+\telse if ((x = skip_prefix(sp, blob_type)) && *x == '}')\n \t\texpected_type = OBJ_BLOB;\n \telse if (!prefixcmp(sp, \"object}\"))\n \t\texpected_type = OBJ_ANY;\n-- \n1.8.4.2.g87d4a77\n"},{"id":"226672","messageId":"20130903202729.GA26217@sigill.intra.peff.net","threadId":"34844","inReplyTo":"20130903202041.GA7463@sigill.intra.peff.net","subject":"[PATCH 2/1 alt] peel_onion: do not assume length of x_type globals","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-03T20:27:30Z","receivedAt":"2013-09-03T20:27:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we are parsing \"rev^{foo}\", we check \"foo\" against the\nvarious global type strings, like \"commit_type\",\n\"tree_type\", etc. This is nicely abstracted, but then we\ndestroy the abstraction completely by using magic numbers\nthat must match the length of the type strings.\n\nWe could avoid these magic numbers by using skip_prefix. But\ntaking a step back, we can realize that using the\n\"commit_type\" global is not really buying us anything. It is\nnot ever going to change from being \"commit\" without causing\nsevere breakage to existing uses. And even if it did change\nfor some crazy reason, we would want to evaluate its effects\non the \"rev^{}\" syntax, anyway.\n\nLet's just switch these to using a custom string literal, as\nwe do for \"rev^{object}\". The resulting code is more robust\nto changes in the type strings, and is more readable.\n\nSigned-off-by: Jeff King \n---\nIf you _really_ wanted to abstract it, you could make commit_type a\nmacro and use string concatenation along with prefixcmp. But that is\ngoing in the direction of less readable, I think. :)\n\nThis has probably consumed enough brain cycles for such a small and\nprobably unimportant cleanup. I'll let Junio pick from the 2 options (or\nchoose to do nothing at all) as he sees fit.\n\n sha1_name.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 6dc496d..2f6e5ab 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -677,13 +677,13 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)\n \t\treturn -1;\n \n \tsp++; /* beginning of type name, or closing brace for empty */\n-\tif (!strncmp(commit_type, sp, 6) && sp[6] == '}')\n+\tif (!prefixcmp(sp, \"commit}\"))\n \t\texpected_type = OBJ_COMMIT;\n-\telse if (!strncmp(tag_type, sp, 3) && sp[3] == '}')\n+\telse if (!prefixcmp(sp, \"tag}\"))\n \t\texpected_type = OBJ_TAG;\n-\telse if (!strncmp(tree_type, sp, 4) && sp[4] == '}')\n+\telse if (!prefixcmp(sp, \"tree}\"))\n \t\texpected_type = OBJ_TREE;\n-\telse if (!strncmp(blob_type, sp, 4) && sp[4] == '}')\n+\telse if (!prefixcmp(sp, \"blob}\"))\n \t\texpected_type = OBJ_BLOB;\n \telse if (!prefixcmp(sp, \"object}\"))\n \t\texpected_type = OBJ_ANY;\n-- \n1.8.4.2.g87d4a77\n"},{"id":"226673","messageId":"xmqqsixlpraq.fsf@gitster.dls.corp.google.com","threadId":"34844","inReplyTo":"20130903202729.GA26217@sigill.intra.peff.net","subject":"Re: [PATCH 2/1 alt] peel_onion: do not assume length of x_type globals","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-03T20:46:05Z","receivedAt":"2013-09-03T20:46:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If you _really_ wanted to abstract it, you could make commit_type a\n> macro and use string concatenation along with prefixcmp. But that is\n> going in the direction of less readable, I think. :)\n\n;-) we are on the same wave-length.\n\nThanks, will apply.\n\n>\n> This has probably consumed enough brain cycles for such a small and\n> probably unimportant cleanup. I'll let Junio pick from the 2 options (or\n> choose to do nothing at all) as he sees fit.\n>\n> sha1_name.c | 8 ++++----\n> 1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/sha1_name.c b/sha1_name.c\n> index 6dc496d..2f6e5ab 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -677,13 +677,13 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)\n> \t\treturn -1;\n> \n> \tsp++; /* beginning of type name, or closing brace for empty */\n> -\tif (!strncmp(commit_type, sp, 6) && sp[6] == '}')\n> +\tif (!prefixcmp(sp, \"commit}\"))\n> \t\texpected_type = OBJ_COMMIT;\n> -\telse if (!strncmp(tag_type, sp, 3) && sp[3] == '}')\n> +\telse if (!prefixcmp(sp, \"tag}\"))\n> \t\texpected_type = OBJ_TAG;\n> -\telse if (!strncmp(tree_type, sp, 4) && sp[4] == '}')\n> +\telse if (!prefixcmp(sp, \"tree}\"))\n> \t\texpected_type = OBJ_TREE;\n> -\telse if (!strncmp(blob_type, sp, 4) && sp[4] == '}')\n> +\telse if (!prefixcmp(sp, \"blob}\"))\n> \t\texpected_type = OBJ_BLOB;\n> \telse if (!prefixcmp(sp, \"object}\"))\n> \t\texpected_type = OBJ_ANY;\n"}]}