{"thread":{"id":"42516","subject":"[PATCHv3] pathspec: allow escaped query values","startedAt":"2016-06-02T23:14:13Z","lastAt":"2016-06-02T23:41:20Z","messageCount":3,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"288180","messageId":"20160602231413.9028-1-sbeller@google.com","threadId":"42516","inReplyTo":null,"subject":"[PATCHv3] pathspec: allow escaped query values","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-06-02T23:14:13Z","receivedAt":"2016-06-02T23:14:13Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In our own .gitattributes file we have attributes such as:\n\n    *.[ch] whitespace=indent,trail,space\n\nWhen querying for attributes we want to be able to ask for the exact\nvalue, i.e.\n\n    git ls-files :(attr:whitespace=indent,trail,space)\n\nshould work, but the commas are used in the attr magic to introduce\nthe next attr, such that this query currently fails with\n\nfatal: Invalid pathspec magic 'trail' in ':(attr:whitespace=indent,trail,space)'\n\nThis change allows escaping characters by a backslash, such that the query\n\n    git ls-files :(attr:whitespace=indent\\,trail\\,space)\n\nwill match all path that have the value \"indent,trail,space\" for the\nwhitespace attribute. To accomplish this, we need to modify two places.\nFirst `eat_long_magic` needs to not stop early upon seeing a comma or\nclosing paren that is escaped. As a second step we need to remove any\nescaping from the attr value.\n\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n\n> That would be true _only_ when \"find next escape and copy up to that\n> byte\" aka \"scanning once with optimized strchr(), and copying once\n> with optimized memmove()\" is faster than \"scanning once and copying\"\n> loop.\n\nOh right. that would work for larger strings that don't fit into a cache\nvery well, but in our expected case this will do.\n\n> I was merely reacting to your use of memmove() in a very different\n> loop, where if you unescape \"a\\b\\c\\defghijk\", your memmove() would\n> move \"efghijk\" many times.\n\nRight, at the time of writing I didn't like it, but was ok with it as\nwe only have a few escapes.\n\n> Also the handling of the terminating NUL may need to be\n> updated.\n\nI don't think so.\n\n> That is why I did not say \"replace yours with this\", but\n> merely \"along the lines of this\" ;-)\n\nThis is pretty much your code modulo nits (xmalloz, semicolons) now,\nand I am convinced it works by the test.\n\nHowever if we add a value restriction here, we need to be as strict in the\n.gitattributes parsing as well and put a warning there (similar to\ninvalid_attr_name_message) I would think.\n\nThanks,\nStefan\n\n pathspec.c                      | 52 +++++++++++++++++++++++++++++++++++++----\n t/t6134-pathspec-with-labels.sh | 10 ++++++++\n 2 files changed, 58 insertions(+), 4 deletions(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex 326863a..fe53ddf 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -89,6 +89,51 @@ static void prefix_short_magic(struct strbuf *sb, int prefixlen,\n \tstrbuf_addf(sb, \",prefix:%d)\", prefixlen);\n }\n \n+static size_t strcspn_escaped(const char *s, const char *stop)\n+{\n+\tconst char *i;\n+\n+\tfor (i = s; *i; i++) {\n+\t\t/* skip the escaped character */\n+\t\tif (i[0] == '\\\\' && i[1]) {\n+\t\t\ti++;\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tif (strchr(stop, *i))\n+\t\t\tbreak;\n+\t}\n+\treturn i - s;\n+}\n+\n+static inline int invalid_value_char(const char ch)\n+{\n+\tif (isalnum(ch) || strchr(\",-_\", ch))\n+\t\treturn 0;\n+\treturn -1;\n+}\n+\n+static char *attr_value_unescape(const char *value)\n+{\n+\tconst char *src;\n+\tchar *dst, *ret;\n+\n+\tret = xmallocz(strlen(value));\n+\tfor (src = value, dst = ret; *src; src++, dst++) {\n+\t\tif (*src == '\\\\') {\n+\t\t\tif (!src[1])\n+\t\t\t\tdie(_(\"Escape character '\\\\' not allowed as \"\n+\t\t\t\t      \"last character in attr value\"));\n+\t\t\tsrc++;\n+\t\t}\n+\t\tif (*src && invalid_value_char(*src))\n+\t\t\tdie(\"cannot use '%c' for value matching\", *src);\n+\t\t*dst = *src;\n+\t}\n+\t*dst = '\\0';\n+\treturn ret;\n+}\n+\n static void parse_pathspec_attr_match(struct pathspec_item *item, const char *value)\n {\n \tstruct string_list_item *si;\n@@ -131,10 +176,9 @@ static void parse_pathspec_attr_match(struct pathspec_item *item, const char *va\n \t\t\tif (attr[attr_len] != '=')\n \t\t\t\tam->match_mode = MATCH_SET;\n \t\t\telse {\n+\t\t\t\tconst char *v = &attr[attr_len + 1];\n \t\t\t\tam->match_mode = MATCH_VALUE;\n-\t\t\t\tam->value = xstrdup(&attr[attr_len + 1]);\n-\t\t\t\tif (strchr(am->value, '\\\\'))\n-\t\t\t\t\tdie(_(\"attr spec values must not contain backslashes\"));\n+\t\t\t\tam->value = attr_value_unescape(v);\n \t\t\t}\n \t\t\tbreak;\n \t\t}\n@@ -166,7 +210,7 @@ static void eat_long_magic(struct pathspec_item *item, const char *elt,\n \tfor (copyfrom = elt + 2;\n \t     *copyfrom && *copyfrom != ')';\n \t     copyfrom = nextat) {\n-\t\tsize_t len = strcspn(copyfrom, \",)\");\n+\t\tsize_t len = strcspn_escaped(copyfrom, \",)\");\n \t\tif (copyfrom[len] == ',')\n \t\t\tnextat = copyfrom + len + 1;\n \t\telse\ndiff --git a/t/t6134-pathspec-with-labels.sh b/t/t6134-pathspec-with-labels.sh\nindex a5c9632..f1e355f 100755\n--- a/t/t6134-pathspec-with-labels.sh\n+++ b/t/t6134-pathspec-with-labels.sh\n@@ -163,4 +163,14 @@ test_expect_success 'abort on asking for wrong magic' '\n \ttest_must_fail git ls-files . \":(attr:!label=foo)\"\n '\n \n+test_expect_success 'check attribute list' '\n+\tcat <<-EOF >>.gitattributes &&\n+\t* whitespace=indent,trail,space\n+\tEOF\n+\tcat .gitattributes &&\n+\tgit ls-files \":(attr:whitespace=indent\\,trail\\,space)\" >actual &&\n+\tgit ls-files >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.8.2.126.g9068a9d.dirty\n"},{"id":"288183","messageId":"xmqqwpm7qio2.fsf@gitster.mtv.corp.google.com","threadId":"42516","inReplyTo":"20160602231413.9028-1-sbeller@google.com","subject":"Re: [PATCHv3] pathspec: allow escaped query values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-02T23:23:41Z","receivedAt":"2016-06-02T23:23:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> However if we add a value restriction here, we need to be as strict in the\n> .gitattributes parsing as well and put a warning there (similar to\n> invalid_attr_name_message) I would think.\n\nRemember, the attribute system is used for many purposes other than\nthis new \"further limit pathspec\".  While the (hopefully temporary)\nlimitation hurts here, users will limit the attributes they use for\n\":(attr:VAR=VAL)\" pathspec magic, and it is unlikely that either the\nVAR part or its VALue are something the core Git currently uses\nanyway.\n\nSo I do not think it is necessary or even beneficial to add such a\nwarning.\n\n> +static char *attr_value_unescape(const char *value)\n> +{\n> +\tconst char *src;\n> +\tchar *dst, *ret;\n> +\n> +\tret = xmallocz(strlen(value));\n> +\tfor (src = value, dst = ret; *src; src++, dst++) {\n> +\t\tif (*src == '\\\\') {\n> +\t\t\tif (!src[1])\n> +\t\t\t\tdie(_(\"Escape character '\\\\' not allowed as \"\n> +\t\t\t\t      \"last character in attr value\"));\n> +\t\t\tsrc++;\n> +\t\t}\n> +\t\tif (*src && invalid_value_char(*src))\n> +\t\t\tdie(\"cannot use '%c' for value matching\", *src);\n> +\t\t*dst = *src;\n> +\t}\n> +\t*dst = '\\0';\n> +\treturn ret;\n> +}\n\nPlease sanity-check me.  Just like I said to your original \"I doubt\n*i could be NUL here\", I now doubt *src could be NUL there where\ninvalid_value_char() gets called.\n\nIf *src could be NUL there, then *dst gets NUL once, and then after\nloop exits (presumably after incrementing dst), *dst gets another\nNUL, which was the terminating NUL condition being iffy I mentioned,\nbut as you said, I do not think it would happen, so we can lose the\n\"*src && \" before invalid_value_char() is called.\n\nThanks.\n"},{"id":"288184","messageId":"CAGZ79kas0EdQd4MJUOVFJYfm0-yaDQhJgCBpKQk5m4UrsjPBwg@mail.gmail.com","threadId":"42516","inReplyTo":"xmqqwpm7qio2.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3] pathspec: allow escaped query values","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-06-02T23:41:20Z","receivedAt":"2016-06-02T23:41:20Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jun 2, 2016 at 4:23 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> However if we add a value restriction here, we need to be as strict in the\n>> .gitattributes parsing as well and put a warning there (similar to\n>> invalid_attr_name_message) I would think.\n>\n> Remember, the attribute system is used for many purposes other than\n> this new \"further limit pathspec\".\n\nRight, and in the past we followed a rigid pattern of non-crazy values.\nThe crazy values will show up once users realize they can\nput anything they want into the .gitattributes to query for files.\n\nBefore this labeling change, there was no need to put anything\nother the officially documented values into the attribute files.\n\n> So I do not think it is necessary or even beneficial to add such a\n> warning.\n\nOk.\n\n>\n>> +static char *attr_value_unescape(const char *value)\n>> +{\n>> +     const char *src;\n>> +     char *dst, *ret;\n>> +\n>> +     ret = xmallocz(strlen(value));\n>> +     for (src = value, dst = ret; *src; src++, dst++) {\n>> +             if (*src == '\\\\') {\n>> +                     if (!src[1])\n>> +                             die(_(\"Escape character '\\\\' not allowed as \"\n>> +                                   \"last character in attr value\"));\n>> +                     src++;\n>> +             }\n>> +             if (*src && invalid_value_char(*src))\n>> +                     die(\"cannot use '%c' for value matching\", *src);\n>> +             *dst = *src;\n>> +     }\n>> +     *dst = '\\0';\n>> +     return ret;\n>> +}\n>\n> Please sanity-check me.  Just like I said to your original \"I doubt\n> *i could be NUL here\", I now doubt *src could be NUL there where\n> invalid_value_char() gets called.\n>\n> If *src could be NUL there, then *dst gets NUL once, and then after\n> loop exits (presumably after incrementing dst), *dst gets another\n> NUL, which was the terminating NUL condition being iffy I mentioned,\n> but as you said, I do not think it would happen, so we can lose the\n> \"*src && \" before invalid_value_char() is called.\n\nRight, we can lose the *src check before invalid_value_char\nas that ought to be caught in  if (!src[1]) die(...) before.\n\nThanks,\nStefan\n\n>\n> Thanks.\n"}]}