{"thread":{"id":"50100","subject":"[PATCH] sha1-name.c: Fix handling of revisions that contain paths with brackets","startedAt":"2018-12-23T23:27:26Z","lastAt":"2019-01-02T20:35:55Z","messageCount":7,"participants":["Stan Hu","Duy Nguyen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"365765","messageId":"20181223232716.6339-1-stanhu@gmail.com","threadId":"50100","inReplyTo":null,"subject":"[PATCH] sha1-name.c: Fix handling of revisions that contain paths with brackets","fromName":"Stan Hu","fromEmail":"stanhu@gmail.com","sentAt":"2018-12-23T23:27:16Z","receivedAt":"2018-12-23T23:27:26Z","isPatch":true,"sender":{"key":"stanhu@gmail.com","avatar":"https://avatars.githubusercontent.com/u/963826?v=4"},"body":"Previously, calling ls-tree with a revision such as\n`master^{tree}:foo/{{path}}` would show the root tree instead of the\ncorrect tree pointed by foo/{{path}}. If a colon is present in the revision\nname, peel_onion() should assume that the presence of a bracket\nat the end of the string belongs to the filename.\n\nSigned-off-by: Stan Hu <stanhu@gmail.com>\n---\n sha1-name.c               | 14 +++++++++++++-\n t/t3104-ls-tree-braces.sh | 30 ++++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 1 deletion(-)\n create mode 100644 t/t3104-ls-tree-braces.sh\n\ndiff --git a/sha1-name.c b/sha1-name.c\nindex faa60f69e3..69ecf0a26c 100644\n--- a/sha1-name.c\n+++ b/sha1-name.c\n@@ -1001,9 +1001,21 @@ static int peel_onion(const char *name, int len, struct object_id *oid,\n \t * \"ref^{commit}\".  \"commit^{tree}\" could be used to find the\n \t * top-level tree of the given commit.\n \t */\n-\tif (len < 4 || name[len-1] != '}')\n+\tif (len < 4)\n \t\treturn -1;\n \n+\t/* Check for names in ref:path format in case the path includes\n+\t * brackets (e.g. ref^{type}:foo/{{bar}}).\n+\t */\n+\tfor (sp = name; sp < name + len; sp++) {\n+\t\tif (*sp == ':')\n+\t\t\treturn -1;\n+\t}\n+\n+\tif (sp == name && name[len-1] != '}') {\n+\t\treturn -1;\n+\t}\n+\n \tfor (sp = name + len - 1; name <= sp; sp--) {\n \t\tint ch = *sp;\n \t\tif (ch == '{' && name < sp && sp[-1] == '^')\ndiff --git a/t/t3104-ls-tree-braces.sh b/t/t3104-ls-tree-braces.sh\nnew file mode 100644\nindex 0000000000..3ead86c4fe\n--- /dev/null\n+++ b/t/t3104-ls-tree-braces.sh\n@@ -0,0 +1,30 @@\n+#!/bin/sh\n+\n+test_description='ls-tree with a folder with braces'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tmkdir -p \"newdir/{{curly}}\" &&\n+\ttouch \"newdir/{{curly}}/one\" &&\n+\tgit add \"newdir/{{curly}}/one\" &&\n+\tgit commit -m test\n+'\n+\n+test_expect_success 'ls-tree with curly brace folder' '\n+\tcat >expect <<-EOF &&\n+\t100644 blob $EMPTY_BLOB\tone\n+\tEOF\n+\tgit ls-tree -r \"HEAD:newdir/{{curly}}\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'ls-tree with type restriction and curly brace folder' '\n+\tcat >expect <<-EOF &&\n+\t100644 blob $EMPTY_BLOB\tone\n+\tEOF\n+\tgit ls-tree \"HEAD^{tree}:newdir/{{curly}}\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.19.1\n\n"},{"id":"365767","messageId":"20181223233736.10306-1-stanhu@gmail.com","threadId":"50100","inReplyTo":"20181223232716.6339-1-stanhu@gmail.com","subject":"[PATCH] sha1-name.c: Fix handling of revisions that contain paths with brackets","fromName":"Stan Hu","fromEmail":"stanhu@gmail.com","sentAt":"2018-12-23T23:37:36Z","receivedAt":"2018-12-23T23:37:46Z","isPatch":true,"sender":{"key":"stanhu@gmail.com","avatar":"https://avatars.githubusercontent.com/u/963826?v=4"},"body":"Previously, calling ls-tree with a revision such as\n`master^{tree}:foo/{{path}}` would show the root tree instead of the\ncorrect tree pointed by foo/{{path}}. If a colon is present in the revision\nname, peel_onion() should assume that the presence of a bracket\nat the end of the string belongs to the filename.\n\nSigned-off-by: Stan Hu <stanhu@gmail.com>\n---\n sha1-name.c               | 14 +++++++++++++-\n t/t3104-ls-tree-braces.sh | 30 ++++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 1 deletion(-)\n create mode 100644 t/t3104-ls-tree-braces.sh\n\ndiff --git a/sha1-name.c b/sha1-name.c\nindex faa60f69e3..69ecf0a26c 100644\n--- a/sha1-name.c\n+++ b/sha1-name.c\n@@ -1001,9 +1001,21 @@ static int peel_onion(const char *name, int len, struct object_id *oid,\n \t * \"ref^{commit}\".  \"commit^{tree}\" could be used to find the\n \t * top-level tree of the given commit.\n \t */\n-\tif (len < 4 || name[len-1] != '}')\n+\tif (len < 4)\n \t\treturn -1;\n \n+\t/* Check for names in ref:path format in case the path includes\n+\t * brackets (e.g. ref^{type}:foo/{{bar}}).\n+\t */\n+\tfor (sp = name; sp < name + len; sp++) {\n+\t\tif (*sp == ':')\n+\t\t\treturn -1;\n+\t}\n+\n+\tif (sp == name && name[len-1] != '}') {\n+\t\treturn -1;\n+\t}\n+\n \tfor (sp = name + len - 1; name <= sp; sp--) {\n \t\tint ch = *sp;\n \t\tif (ch == '{' && name < sp && sp[-1] == '^')\ndiff --git a/t/t3104-ls-tree-braces.sh b/t/t3104-ls-tree-braces.sh\nnew file mode 100644\nindex 0000000000..3ead86c4fe\n--- /dev/null\n+++ b/t/t3104-ls-tree-braces.sh\n@@ -0,0 +1,30 @@\n+#!/bin/sh\n+\n+test_description='ls-tree with a folder with braces'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tmkdir -p \"newdir/{{curly}}\" &&\n+\ttouch \"newdir/{{curly}}/one\" &&\n+\tgit add \"newdir/{{curly}}/one\" &&\n+\tgit commit -m test\n+'\n+\n+test_expect_success 'ls-tree with curly brace folder' '\n+\tcat >expect <<-EOF &&\n+\t100644 blob $EMPTY_BLOB\tone\n+\tEOF\n+\tgit ls-tree -r \"HEAD:newdir/{{curly}}\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'ls-tree with type restriction and curly brace folder' '\n+\tcat >expect <<-EOF &&\n+\t100644 blob $EMPTY_BLOB\tone\n+\tEOF\n+\tgit ls-tree \"HEAD^{tree}:newdir/{{curly}}\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.19.0\n\n"},{"id":"365768","messageId":"20181223234058.5834-1-stanhu@gmail.com","threadId":"50100","inReplyTo":"20181223233736.10306-1-stanhu@gmail.com","subject":"[PATCH v2] sha1-name.c: Fix handling of revisions that contain paths with brackets","fromName":"Stan Hu","fromEmail":"stanhu@gmail.com","sentAt":"2018-12-23T23:40:59Z","receivedAt":"2018-12-23T23:41:25Z","isPatch":true,"sender":{"key":"stanhu@gmail.com","avatar":"https://avatars.githubusercontent.com/u/963826?v=4"},"body":"Previously, calling ls-tree with a revision such as\n`master^{tree}:foo/{{path}}` would show the root tree instead of the\ncorrect tree pointed by foo/{{path}}. If a colon is present in the revision\nname, peel_onion() should assume that the presence of a bracket\nat the end of the string belongs to the filename.\n\nSigned-off-by: Stan Hu <stanhu@gmail.com>\n---\n sha1-name.c               | 14 +++++++++++++-\n t/t3104-ls-tree-braces.sh | 30 ++++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 1 deletion(-)\n create mode 100755 t/t3104-ls-tree-braces.sh\n\ndiff --git a/sha1-name.c b/sha1-name.c\nindex faa60f69e3..588b7a53cc 100644\n--- a/sha1-name.c\n+++ b/sha1-name.c\n@@ -1001,9 +1001,21 @@ static int peel_onion(const char *name, int len, struct object_id *oid,\n \t * \"ref^{commit}\".  \"commit^{tree}\" could be used to find the\n \t * top-level tree of the given commit.\n \t */\n-\tif (len < 4 || name[len-1] != '}')\n+\tif (len < 4)\n \t\treturn -1;\n \n+\t/* Check for names in ref:path format in case the path includes\n+\t * brackets (e.g. ref^{type}:foo/{{bar}}).\n+\t */\n+\tfor (sp = name; sp < name + len; sp++) {\n+\t\tif (*sp == ':')\n+\t\t\treturn -1;\n+\t}\n+\n+\tif (name[len-1] != '}') {\n+\t\treturn -1;\n+\t}\n+\n \tfor (sp = name + len - 1; name <= sp; sp--) {\n \t\tint ch = *sp;\n \t\tif (ch == '{' && name < sp && sp[-1] == '^')\ndiff --git a/t/t3104-ls-tree-braces.sh b/t/t3104-ls-tree-braces.sh\nnew file mode 100755\nindex 0000000000..3ead86c4fe\n--- /dev/null\n+++ b/t/t3104-ls-tree-braces.sh\n@@ -0,0 +1,30 @@\n+#!/bin/sh\n+\n+test_description='ls-tree with a folder with braces'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tmkdir -p \"newdir/{{curly}}\" &&\n+\ttouch \"newdir/{{curly}}/one\" &&\n+\tgit add \"newdir/{{curly}}/one\" &&\n+\tgit commit -m test\n+'\n+\n+test_expect_success 'ls-tree with curly brace folder' '\n+\tcat >expect <<-EOF &&\n+\t100644 blob $EMPTY_BLOB\tone\n+\tEOF\n+\tgit ls-tree -r \"HEAD:newdir/{{curly}}\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'ls-tree with type restriction and curly brace folder' '\n+\tcat >expect <<-EOF &&\n+\t100644 blob $EMPTY_BLOB\tone\n+\tEOF\n+\tgit ls-tree \"HEAD^{tree}:newdir/{{curly}}\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.19.0\n\n"},{"id":"365771","messageId":"20181224080651.GA12708@duynguyen.home","threadId":"50100","inReplyTo":"20181223234058.5834-1-stanhu@gmail.com","subject":"Re: [PATCH v2] sha1-name.c: Fix handling of revisions that contain paths with brackets","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-12-24T08:06:51Z","receivedAt":"2018-12-24T08:07:01Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Dec 23, 2018 at 11:40:59PM +0000, Stan Hu wrote:\n> Previously, calling ls-tree with a revision such as\n> `master^{tree}:foo/{{path}}` would show the root tree instead of the\n> correct tree pointed by foo/{{path}}. If a colon is present in the revision\n> name, peel_onion() should assume that the presence of a bracket\n> at the end of the string belongs to the filename.\n> \n> Signed-off-by: Stan Hu <stanhu@gmail.com>\n> ---\n>  sha1-name.c               | 14 +++++++++++++-\n>  t/t3104-ls-tree-braces.sh | 30 ++++++++++++++++++++++++++++++\n>  2 files changed, 43 insertions(+), 1 deletion(-)\n>  create mode 100755 t/t3104-ls-tree-braces.sh\n> \n> diff --git a/sha1-name.c b/sha1-name.c\n> index faa60f69e3..588b7a53cc 100644\n> --- a/sha1-name.c\n> +++ b/sha1-name.c\n> @@ -1001,9 +1001,21 @@ static int peel_onion(const char *name, int len, struct object_id *oid,\n>  \t * \"ref^{commit}\".  \"commit^{tree}\" could be used to find the\n>  \t * top-level tree of the given commit.\n>  \t */\n> -\tif (len < 4 || name[len-1] != '}')\n> +\tif (len < 4)\n>  \t\treturn -1;\n>  \n> +\t/* Check for names in ref:path format in case the path includes\n> +\t * brackets (e.g. ref^{type}:foo/{{bar}}).\n> +\t */\n> +\tfor (sp = name; sp < name + len; sp++) {\n> +\t\tif (*sp == ':')\n> +\t\t\treturn -1;\n> +\t}\n> +\n> +\tif (name[len-1] != '}') {\n> +\t\treturn -1;\n> +\t}\n\nInstead of replacing one loose check (find '}' at the end) with\nanother one, how about tighten the parsing? peel_onion() is supposed\nto consume all \"len\" characters or none so checking something like\nthis may be better.\n\nNote that it also shows another corner case we need to be careful\nabout: master^{/regex} syntax _can_ contain colons in regex. I suppose\ndoing strchr to find the closing '}' here is better than what I did\nbelow.\n\n--8<--\ndiff --git a/sha1-name.c b/sha1-name.c\nindex faa60f69e3..c9e26206ce 100644\n--- a/sha1-name.c\n+++ b/sha1-name.c\n@@ -989,7 +989,7 @@ static int peel_onion(const char *name, int len, struct object_id *oid,\n \t\t      unsigned lookup_flags)\n {\n \tstruct object_id outer;\n-\tconst char *sp;\n+\tconst char *sp, *end;\n \tunsigned int expected_type = 0;\n \tstruct object *o;\n \n@@ -1013,21 +1013,26 @@ static int peel_onion(const char *name, int len, struct object_id *oid,\n \t\treturn -1;\n \n \tsp++; /* beginning of type name, or closing brace for empty */\n-\tif (starts_with(sp, \"commit}\"))\n+\tif (skip_prefix(sp, \"commit}\", &end))\n \t\texpected_type = OBJ_COMMIT;\n-\telse if (starts_with(sp, \"tag}\"))\n+\telse if (skip_prefix(sp, \"tag}\", &end))\n \t\texpected_type = OBJ_TAG;\n-\telse if (starts_with(sp, \"tree}\"))\n+\telse if (skip_prefix(sp, \"tree}\", &end))\n \t\texpected_type = OBJ_TREE;\n-\telse if (starts_with(sp, \"blob}\"))\n+\telse if (skip_prefix(sp, \"blob}\", &end))\n \t\texpected_type = OBJ_BLOB;\n-\telse if (starts_with(sp, \"object}\"))\n+\telse if (skip_prefix(sp, \"object}\", &end))\n \t\texpected_type = OBJ_ANY;\n-\telse if (sp[0] == '}')\n+\telse if (sp[0] == '}') {\n \t\texpected_type = OBJ_NONE;\n-\telse if (sp[0] == '/')\n+\t\tend = sp + 1;\n+\t} else if (sp[0] == '/') {\n \t\texpected_type = OBJ_COMMIT;\n-\telse\n+\t\tend = name + len;\n+\t} else\n+\t\treturn -1;\n+\n+\tif (end != name + len)\n \t\treturn -1;\n \n \tlookup_flags &= ~GET_OID_DISAMBIGUATORS;\n--8<--\n\n--\nDuy\n"},{"id":"365914","messageId":"xmqqtvixo219.fsf@gitster-ct.c.googlers.com","threadId":"50100","inReplyTo":"20181224080651.GA12708@duynguyen.home","subject":"Re: [PATCH v2] sha1-name.c: Fix handling of revisions that contain paths with brackets","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-28T19:59:46Z","receivedAt":"2018-12-28T19:59:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> Note that it also shows another corner case we need to be careful\n> about: master^{/regex} syntax _can_ contain colons in regex. I suppose\n> doing strchr to find the closing '}' here is better than what I did\n> below.\n\nYes, the posted patch breaks master^{/foo:bar}, I suppose.\n"},{"id":"365941","messageId":"20181229054357.11716-1-stanhu@gmail.com","threadId":"50100","inReplyTo":"20181224080651.GA12708@duynguyen.home","subject":"[PATCH v3] sha1-name.c: Fix handling of revisions that contain paths with brackets","fromName":"Stan Hu","fromEmail":"stanhu@gmail.com","sentAt":"2018-12-29T05:43:58Z","receivedAt":"2018-12-29T05:44:28Z","isPatch":true,"sender":{"key":"stanhu@gmail.com","avatar":"https://avatars.githubusercontent.com/u/963826?v=4"},"body":"Previously, calling ls-tree with a revision such as\n`master^{tree}:foo/{{path}}` would show the root tree instead of the\ncorrect tree pointed by foo/{{path}}. We improve this by ensuring\npeel_onion() consumes all \"len\" characters or none.\n\nNote that it's also possible to have directories named ^{tree} and other\npatterns that look like type restrictions. To handle those cases, scan\nfrom the beginning of the name instead of the end.\n\nSigned-off-by: Stan Hu <stanhu@gmail.com>\n---\n sha1-name.c               | 35 ++++++++++--------\n t/t3104-ls-tree-braces.sh | 75 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 96 insertions(+), 14 deletions(-)\n create mode 100755 t/t3104-ls-tree-braces.sh\n\ndiff --git a/sha1-name.c b/sha1-name.c\nindex faa60f69e3..ab372a90be 100644\n--- a/sha1-name.c\n+++ b/sha1-name.c\n@@ -989,7 +989,7 @@ static int peel_onion(const char *name, int len, struct object_id *oid,\n \t\t      unsigned lookup_flags)\n {\n \tstruct object_id outer;\n-\tconst char *sp;\n+\tconst char *sp, *end;\n \tunsigned int expected_type = 0;\n \tstruct object *o;\n \n@@ -1001,33 +1001,40 @@ static int peel_onion(const char *name, int len, struct object_id *oid,\n \t * \"ref^{commit}\".  \"commit^{tree}\" could be used to find the\n \t * top-level tree of the given commit.\n \t */\n-\tif (len < 4 || name[len-1] != '}')\n+\tif (len < 4)\n \t\treturn -1;\n \n-\tfor (sp = name + len - 1; name <= sp; sp--) {\n+\tfor (sp = name; sp < name + len; sp++) {\n \t\tint ch = *sp;\n-\t\tif (ch == '{' && name < sp && sp[-1] == '^')\n+\t\tif (ch == '^' && sp < name + len && sp[1] == '{')\n \t\t\tbreak;\n \t}\n-\tif (sp <= name)\n+\n+\tif (sp == name + len)\n \t\treturn -1;\n \n-\tsp++; /* beginning of type name, or closing brace for empty */\n-\tif (starts_with(sp, \"commit}\"))\n+\tsp += 2; /* beginning of type name, or closing brace for empty */\n+\n+\tif (skip_prefix(sp, \"commit}\", &end))\n \t\texpected_type = OBJ_COMMIT;\n-\telse if (starts_with(sp, \"tag}\"))\n+\telse if (skip_prefix(sp, \"tag}\", &end))\n \t\texpected_type = OBJ_TAG;\n-\telse if (starts_with(sp, \"tree}\"))\n+\telse if (skip_prefix(sp, \"tree}\", &end))\n \t\texpected_type = OBJ_TREE;\n-\telse if (starts_with(sp, \"blob}\"))\n+\telse if (skip_prefix(sp, \"blob}\", &end))\n \t\texpected_type = OBJ_BLOB;\n-\telse if (starts_with(sp, \"object}\"))\n+\telse if (skip_prefix(sp, \"object}\", &end))\n \t\texpected_type = OBJ_ANY;\n-\telse if (sp[0] == '}')\n+\telse if (sp[0] == '}') {\n \t\texpected_type = OBJ_NONE;\n-\telse if (sp[0] == '/')\n+\t\tend = sp + 1;\n+\t} else if (sp[0] == '/') {\n \t\texpected_type = OBJ_COMMIT;\n-\telse\n+\t\tend = name + len;\n+\t} else\n+\t\treturn -1;\n+\n+\tif (end != name + len)\n \t\treturn -1;\n \n \tlookup_flags &= ~GET_OID_DISAMBIGUATORS;\ndiff --git a/t/t3104-ls-tree-braces.sh b/t/t3104-ls-tree-braces.sh\nnew file mode 100755\nindex 0000000000..adeed5bb49\n--- /dev/null\n+++ b/t/t3104-ls-tree-braces.sh\n@@ -0,0 +1,75 @@\n+#!/bin/sh\n+\n+test_description='ls-tree with folders with brackets'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tmkdir -p \"newdir/{{curly}}\" &&\n+\tmkdir -p \"newdir/^{tree}\" &&\n+\ttouch \"newdir/{{curly}}/one\" &&\n+\ttouch \"newdir/^{tree}/two\" &&\n+\tgit add \"newdir/{{curly}}/one\" &&\n+\tgit add \"newdir/^{tree}/two\" &&\n+\tgit commit -m \"Test message: search me\"\n+'\n+\n+test_expect_success 'ls-tree with curly brace folder' '\n+\tcat >expect <<-EOF &&\n+\t100644 blob $EMPTY_BLOB\tone\n+\tEOF\n+\tgit ls-tree -r \"HEAD:newdir/{{curly}}\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'ls-tree with type restriction and curly brace folder' '\n+\tcat >expect <<-EOF &&\n+\t100644 blob $EMPTY_BLOB\tone\n+\tEOF\n+\tgit ls-tree \"HEAD^{tree}:newdir/{{curly}}\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'ls-tree with type restriction and folder named ^{tree}' '\n+\tcat >expect <<-EOF &&\n+\t100644 blob $EMPTY_BLOB\ttwo\n+\tEOF\n+\tgit ls-tree \"HEAD^{tree}:newdir/^{tree}\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'ls-tree with unknown type restriction' '\n+\t(git ls-tree HEAD^{foobar} >actual || true) &&\n+\ttest_must_be_empty actual\n+'\n+\n+test_output () {\n+\tsed -e \"s/ $OID_REGEX\t/ X\t/\" <actual >check\n+\ttest_cmp expect check\n+}\n+\n+test_expect_success 'ls-tree with regex' '\n+\tcat >expect <<-EOF &&\n+\t040000 tree X\tnewdir\n+\tEOF\n+\tgit ls-tree \"HEAD^{/message}\" >actual &&\n+\ttest_output\n+'\n+\n+test_expect_success 'ls-tree with regex with a colon' '\n+\tcat >expect <<-EOF &&\n+\t040000 tree X\tnewdir\n+\tEOF\n+\tgit ls-tree \"HEAD^{/message: search}\" >actual &&\n+\ttest_output\n+'\n+\n+test_expect_success 'ls-tree with regex and curly brace folder' '\n+\tcat >expect <<-EOF &&\n+\t100644 blob $EMPTY_BLOB\tone\n+\tEOF\n+\tgit ls-tree \"HEAD^{/message: search}:newdir/{{curly}}\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.19.1\n\n"},{"id":"366052","messageId":"xmqqftuakdax.fsf@gitster-ct.c.googlers.com","threadId":"50100","inReplyTo":"20181229054357.11716-1-stanhu@gmail.com","subject":"Re: [PATCH v3] sha1-name.c: Fix handling of revisions that contain paths with brackets","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-02T20:35:50Z","receivedAt":"2019-01-02T20:35:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stan Hu <stanhu@gmail.com> writes:\n\n> -\tif (len < 4 || name[len-1] != '}')\n> +\tif (len < 4)\n>  \t\treturn -1;\n\nThe original does not expect any string given after the ^{<type>}\ndereferencer, like :<path>, and that is why this function returns\nvery early for anything when name[len-1] is not a closing brace.\n\nWe do not do that anymore, because...?\n\nThis gives me a nagging feeling that the patch is barking up a wrong\ntree.  Consider what happens when a path that does not have any\nfunny characters, e.g. \"v1.4.0^{tree}:a/b/c\", is given from the top\nlevel of the request chain (e.g. \"rev-parse v1.4.0^{tree}:a/b/c\")?\nThe caller must be feeding this function \"v1.4.0^{tree}\" after\nfinding the colon before the path \"a/b/c\" and setting len to point\nat the colon---otherwise we won't be checking for \"}\" at the end\nlike this.\n\nWhen given \"master^{tree}:a/b/{c}\", wouldn't the caller be doing\nthe same stripping to find the colon and calling this function with\nlen pointing at the colon before the path (i.e. \"master^{tree}\")?\n\nTo put it another way, it is OK if this patch wants to shift the\nresponsibility of finding the colon that separates the tree-ish part\nand the path part from the caller to this function, but then I would\nexpect changes to the caller, because now the caller does not have\nto find \":a/b/c\" in \"v1.4.0^{tree}:a/b/c\" and set up the len before\ncalling this function.  Why can the resulting code after applying\nthis patch be correct without such a matching change? \n\n>  \n> -\tfor (sp = name + len - 1; name <= sp; sp--) {\n> +\tfor (sp = name; sp < name + len; sp++) {\n>  \t\tint ch = *sp;\n> -\t\tif (ch == '{' && name < sp && sp[-1] == '^')\n> +\t\tif (ch == '^' && sp < name + len && sp[1] == '{')\n>  \t\t\tbreak;\n>  \t}\n\nWe used to scan from the tail (as we expected that the caller gives\nus a (name, len) that ends with \"^{<type>}\".  The updated code\ninstead scans from the front, looking for \"^{\".  I do not\nparticularly mind the change of strategy, as long as it is correctly\ndone, but I suspect the function will stay simpler if the callee is\nfixed instead.\n\nThe only troublesome case is the REV^{/...} syntax.  For example,\n\n\tHEAD^{/^Git 2.0}^{tree}:t/\n\nwould want to find the commit \"HEAD^{/^Git 2.0}\", peel it down to a\ntree object with \"^{tree}\" and then take its \"t/\" subtree.  It used\nto be that the caller (get_oid_with_context_1() has a loop that\nfinds a colon outside \"{...}\" and that finds \":t/\" before calling\nget_oid_1()) was responsible to give us \"HEAD^{/^Git 2.0}^{tree}\" by\nstripping \":t/\", and I presume that it is still happening, but the\nabove loop would terminate upon seeing \"^{\" immediately after HEAD,\nwithout even realizing that it has ^{tree} after it.\n\n> -\tif (sp <= name)\n> +\n> +\tif (sp == name + len)\n>  \t\treturn -1;\n\n> -\tsp++; /* beginning of type name, or closing brace for empty */\n> -\tif (starts_with(sp, \"commit}\"))\n> +\tsp += 2; /* beginning of type name, or closing brace for empty */\n\nAnd this comment indicates that the code did not expect to see ^{/^Git 2.0}\nbefore ^{tree}.  \n\nI do not quite follow.  How can this patch be correct?\n\nPuzzled.\n\n$ git rev-parse \"master^{/^Git 2.0}^{tree}\"\nbc6ce29d1ec757d9d036532531a1046db4da0d96\n$ ./git rev-parse \"master^{/^Git 2.0}^{tree}\"\nmaster^{/^Git 2.0}^{tree}\nfatal: ambiguous argument 'master^{/^Git 2.0}^{tree}': unknown revision or path not in the working tree.\n"}]}