{"thread":{"id":"42513","subject":"[PATCHv2] pathspec: allow escaped query values","startedAt":"2016-06-02T21:30:15Z","lastAt":"2016-06-02T23:01:33Z","messageCount":5,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"288160","messageId":"20160602213015.21712-1-sbeller@google.com","threadId":"42513","inReplyTo":null,"subject":"[PATCHv2] pathspec: allow escaped query values","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-06-02T21:30:15Z","receivedAt":"2016-06-02T21:30:15Z","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\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nSo here is the \"escaping only, but escaping done right\" version.\n(It goes on top of sb/pathspec-label)\n\nThanks,\nStefan\n\n pathspec.c                      | 44 +++++++++++++++++++++++++++++++++++++----\n t/t6134-pathspec-with-labels.sh | 23 +++++++++++++++++++++\n 2 files changed, 63 insertions(+), 4 deletions(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex 326863a..45bd775 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -89,6 +89,43 @@ 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 *reject)\n+{\n+\tconst char *i, *j;\n+\n+\tfor (i = s; *i; i++) {\n+\t\t/* skip escaped the character */\n+\t\tif (i[0] == '\\\\' && i[1]) {\n+\t\t\ti++;\n+\t\t\tcontinue;\n+\t\t}\n+\t\t/* see if any of the chars matches the current character */\n+\t\tfor (j = reject; *j; j++)\n+\t\t\tif (!*i || *i == *j)\n+\t\t\t\treturn i - s;\n+\t}\n+\treturn i - s;\n+}\n+\n+static char *attr_value_unescape(const char *value)\n+{\n+\tchar *i, *ret = xstrdup(value);\n+\n+\tfor (i = ret; *i; i++) {\n+\t\tif (i[0] == '\\\\') {\n+\t\t\tif (!i[1])\n+\t\t\t\tdie(_(\"Escape character '\\\\' not allowed as \"\n+\t\t\t\t      \"last character in attr value\"));\n+\n+\t\t\t/* remove the backslash */\n+\t\t\tmemmove(i, i + 1, strlen(i));\n+\t\t\t/* and ignore the character after that */\n+\t\t\ti++;\n+\t\t}\n+\t}\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 +168,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 +202,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..cbea858 100755\n--- a/t/t6134-pathspec-with-labels.sh\n+++ b/t/t6134-pathspec-with-labels.sh\n@@ -163,4 +163,27 @@ 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+test_expect_success 'wrong escaping caught' '\n+\t# Pass one backslash to git to fail with a missing closing paren\n+\ttest_must_fail git ls-files \":(attr:marked-with-backslash=\\\\)\" 2>actual &&\n+\ttest_i18ngrep Missing actual\n+'\n+test_expect_success 'check escaped backslash' '\n+\tcat <<-EOF >>.gitattributes &&\n+\t/sub/* marked-with-backslash=\\\\\n+\tEOF\n+\tgit ls-files \":(attr:marked-with-backslash=\\\\\\\\)\" >actual &&\n+\tgit ls-files sub/ >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.8.2.126.gaa5c87d.dirty\n"},{"id":"288165","messageId":"xmqqr3cfs1dp.fsf@gitster.mtv.corp.google.com","threadId":"42513","inReplyTo":"20160602213015.21712-1-sbeller@google.com","subject":"Re: [PATCHv2] pathspec: allow escaped query values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-02T21:54:10Z","receivedAt":"2016-06-02T21:54:10Z","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> In our own .gitattributes file we have attributes such as:\n>\n>     *.[ch] whitespace=indent,trail,space\n>\n> When querying for attributes we want to be able to ask for the exact\n> value, i.e.\n>\n>     git ls-files :(attr:whitespace=indent,trail,space)\n>\n> should work, but the commas are used in the attr magic to introduce\n> the next attr, ...\n> ...\n> So here is the \"escaping only, but escaping done right\" version.\n> (It goes on top of sb/pathspec-label)\n\nThe phrase \"should work\" is probably a bit too strong (I'd have said\n\"it would be nice if this worked\"), as we do not have to even\nsupport comma for our immediately expected use cases.  Allowing it\nmerely makes a casual test using our own .gitattributes easier.\n\n> +static size_t strcspn_escaped(const char *s, const char *reject)\n\nPerhaps s/reject/stop/?\n\n> +{\n> +\tconst char *i, *j;\n> +\n> +\tfor (i = s; *i; i++) {\n> +\t\t/* skip escaped the character */\n> +\t\tif (i[0] == '\\\\' && i[1]) {\n> +\t\t\ti++;\n> +\t\t\tcontinue;\n> +\t\t}\n> +\t\t/* see if any of the chars matches the current character */\n> +\t\tfor (j = reject; *j; j++)\n> +\t\t\tif (!*i || *i == *j)\n> +\t\t\t\treturn i - s;\n\nI somehow doubt that *i can be NUL here.  In any case, this looks\nmore like\n\n\t/* is *i is one of the stop codon? */\n        if (strchr(stop, *i))\n\t\tbreak;\n\n> +\t}\n> +\treturn i - s;\n> +}\n\n> +static char *attr_value_unescape(const char *value)\n> +{\n> +\tchar *i, *ret = xstrdup(value);\n> +\n> +\tfor (i = ret; *i; i++) {\n> +\t\tif (i[0] == '\\\\') {\n> +\t\t\tif (!i[1])\n> +\t\t\t\tdie(_(\"Escape character '\\\\' not allowed as \"\n> +\t\t\t\t      \"last character in attr value\"));\n> +\n> +\t\t\t/* remove the backslash */\n> +\t\t\tmemmove(i, i + 1, strlen(i));\n> +\t\t\t/* and ignore the character after that */\n> +\t\t\ti++;\n> +\t\t}\n> +\t}\n> +\treturn ret;\n> +}\n> +\n\nRepeated memmove() and strlen() somehow bothers me.  Would there be\na more efficient and straight-forward way to do this, perhaps along\nthe lines of this instead?\n\n\tconst char *src;\n        char *dst, *ret;\n\n\tret = xmalloc(strlen(value));\n        for (src = value, dst = ret; *src; src++, dst++) {\n\t\tif (*src == '\\\\') {\n                \tif (!src[1])\n                        \tdie();\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                *dst = *src;\n\t}\n        *dst = '\\0'\n        return ret;\n\nEven though I earlier said \"Now we have an unquote mechanism, we can\nopen the floodgate by lifting the \"no backslash in value\" check, I\nnow realize that we do want to keep some escape hatch for us to\nextend the quoting syntax even more later, so perhaps with something\nlike this:\n\n\tstatic inline int invalid_value_char(const char ch)\n        {\n        \tif (isalnum(ch) || strchr(\",-_\", ch))\n                \treturn 0;\n\t\treturn -1;\n\t}\n\nThanks.\n"},{"id":"288169","messageId":"CAGZ79kaxqK=kL3=NdaHk+qo94fyiDE0LZOrKVVHtDJisKmABfQ@mail.gmail.com","threadId":"42513","inReplyTo":"xmqqr3cfs1dp.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv2] pathspec: allow escaped query values","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-06-02T22:10:32Z","receivedAt":"2016-06-02T22:10:32Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jun 2, 2016 at 2:54 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> In our own .gitattributes file we have attributes such as:\n>>\n>>     *.[ch] whitespace=indent,trail,space\n>>\n>> When querying for attributes we want to be able to ask for the exact\n>> value, i.e.\n>>\n>>     git ls-files :(attr:whitespace=indent,trail,space)\n>>\n>> should work, but the commas are used in the attr magic to introduce\n>> the next attr, ...\n>> ...\n>> So here is the \"escaping only, but escaping done right\" version.\n>> (It goes on top of sb/pathspec-label)\n>\n> The phrase \"should work\" is probably a bit too strong (I'd have said\n> \"it would be nice if this worked\"), as we do not have to even\n> support comma for our immediately expected use cases.  Allowing it\n> merely makes a casual test using our own .gitattributes easier.\n>\n>> +static size_t strcspn_escaped(const char *s, const char *reject)\n>\n> Perhaps s/reject/stop/?\n\nI took the signature from the strcspn man page, and my version of the\nman page has `reject` there, `stop` certainly works too\n\n>\n>> +{\n>> +     const char *i, *j;\n>> +\n>> +     for (i = s; *i; i++) {\n>> +             /* skip escaped the character */\n>> +             if (i[0] == '\\\\' && i[1]) {\n>> +                     i++;\n>> +                     continue;\n>> +             }\n>> +             /* see if any of the chars matches the current character */\n>> +             for (j = reject; *j; j++)\n>> +                     if (!*i || *i == *j)\n>> +                             return i - s;\n>\n> I somehow doubt that *i can be NUL here.  In any case, this looks\n> more like\n>\n>         /* is *i is one of the stop codon? */\n>         if (strchr(stop, *i))\n>                 break;\n\nright, that seems easier.\n\n\n>\n>> +     }\n>> +     return i - s;\n>> +}\n>\n>> +static char *attr_value_unescape(const char *value)\n>> +{\n>> +     char *i, *ret = xstrdup(value);\n>> +\n>> +     for (i = ret; *i; i++) {\n>> +             if (i[0] == '\\\\') {\n>> +                     if (!i[1])\n>> +                             die(_(\"Escape character '\\\\' not allowed as \"\n>> +                                   \"last character in attr value\"));\n>> +\n>> +                     /* remove the backslash */\n>> +                     memmove(i, i + 1, strlen(i));\n>> +                     /* and ignore the character after that */\n>> +                     i++;\n>> +             }\n>> +     }\n>> +     return ret;\n>> +}\n>> +\n>\n> Repeated memmove() and strlen() somehow bothers me.  Would there be\n> a more efficient and straight-forward way to do this, perhaps along\n> the lines of this instead?\n>\n>         const char *src;\n>         char *dst, *ret;\n>\n>         ret = xmalloc(strlen(value));\n>         for (src = value, dst = ret; *src; src++, dst++) {\n>                 if (*src == '\\\\') {\n>                         if (!src[1])\n>                                 die();\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> Even though I earlier said \"Now we have an unquote mechanism, we can\n> open the floodgate by lifting the \"no backslash in value\" check, I\n> now realize that we do want to keep some escape hatch for us to\n> extend the quoting syntax even more later, so perhaps with something\n> like this:\n>\n>         static inline int invalid_value_char(const char ch)\n>         {\n>                 if (isalnum(ch) || strchr(\",-_\", ch))\n>                         return 0;\n>                 return -1;\n>         }\n\nMakes sense.\n\nLater on we could have : or ; for an and/or of the values and\nparens and such, so the invalid_value_char makes sense.\n\n>\n> Thanks.\n\nThanks,\nStefan\n"},{"id":"288173","messageId":"CAGZ79kYL_47ptjK1S++Z=JUBOQtG1MJS=h0i=9f3fzRmbZDf-g@mail.gmail.com","threadId":"42513","inReplyTo":"xmqqr3cfs1dp.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv2] pathspec: allow escaped query values","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-06-02T22:53:14Z","receivedAt":"2016-06-02T22:53:14Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jun 2, 2016 at 2:54 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> In our own .gitattributes file we have attributes such as:\n>>\n>>     *.[ch] whitespace=indent,trail,space\n>>\n>> When querying for attributes we want to be able to ask for the exact\n>> value, i.e.\n>>\n>>     git ls-files :(attr:whitespace=indent,trail,space)\n>>\n>> should work, but the commas are used in the attr magic to introduce\n>> the next attr, ...\n>> ...\n>> So here is the \"escaping only, but escaping done right\" version.\n>> (It goes on top of sb/pathspec-label)\n>\n> The phrase \"should work\" is probably a bit too strong (I'd have said\n> \"it would be nice if this worked\"), as we do not have to even\n> support comma for our immediately expected use cases.  Allowing it\n> merely makes a casual test using our own .gitattributes easier.\n>\n>> +static size_t strcspn_escaped(const char *s, const char *reject)\n>\n> Perhaps s/reject/stop/?\n>\n>> +{\n>> +     const char *i, *j;\n>> +\n>> +     for (i = s; *i; i++) {\n>> +             /* skip escaped the character */\n>> +             if (i[0] == '\\\\' && i[1]) {\n>> +                     i++;\n>> +                     continue;\n>> +             }\n>> +             /* see if any of the chars matches the current character */\n>> +             for (j = reject; *j; j++)\n>> +                     if (!*i || *i == *j)\n>> +                             return i - s;\n>\n> I somehow doubt that *i can be NUL here.  In any case, this looks\n> more like\n>\n>         /* is *i is one of the stop codon? */\n>         if (strchr(stop, *i))\n>                 break;\n>\n>> +     }\n>> +     return i - s;\n>> +}\n>\n>> +static char *attr_value_unescape(const char *value)\n>> +{\n>> +     char *i, *ret = xstrdup(value);\n>> +\n>> +     for (i = ret; *i; i++) {\n>> +             if (i[0] == '\\\\') {\n>> +                     if (!i[1])\n>> +                             die(_(\"Escape character '\\\\' not allowed as \"\n>> +                                   \"last character in attr value\"));\n>> +\n>> +                     /* remove the backslash */\n>> +                     memmove(i, i + 1, strlen(i));\n>> +                     /* and ignore the character after that */\n>> +                     i++;\n>> +             }\n>> +     }\n>> +     return ret;\n>> +}\n>> +\n>\n> Repeated memmove() and strlen() somehow bothers me.  Would there be\n> a more efficient and straight-forward way to do this, perhaps along\n> the lines of this instead?\n\nThinking about efficiency, I have the believe that memmove can be faster\nthan a `*src=*dst` thing we do ourselves as it may have access to specialized\nassembly instructions to move larger chunks of memory or such.\n\nSo I think ideally we would do a block copy between the escape characters\n(sketched as:)\n\n    last = input\n    while input not ended:\n        current = find next escape character in input\n        memcpy from input value in the range of last to current\n        last = current + 1\n    copy remaining parts if no further escape is found\n\nIt doesn't seem worth the effort to get it right though.\n\n>\n>         const char *src;\n>         char *dst, *ret;\n>\n>         ret = xmalloc(strlen(value));\n\nxmallocz at least?\n\n>         for (src = value, dst = ret; *src; src++, dst++) {\n>                 if (*src == '\\\\') {\n>                         if (!src[1])\n>                                 die();\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\nThanks,\nStefan\n"},{"id":"288174","messageId":"xmqq60trry9e.fsf@gitster.mtv.corp.google.com","threadId":"42513","inReplyTo":"CAGZ79kYL_47ptjK1S++Z=JUBOQtG1MJS=h0i=9f3fzRmbZDf-g@mail.gmail.com","subject":"Re: [PATCHv2] pathspec: allow escaped query values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-02T23:01:33Z","receivedAt":"2016-06-02T23:01:33Z","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> Thinking about efficiency, I have the believe that memmove can be faster\n> than a `*src=*dst` thing we do ourselves as it may have access to specialized\n> assembly instructions to move larger chunks of memory or such.\n\n> So I think ideally we would do a block copy between the escape characters\n> (sketched as:)\n>\n>     last = input\n>     while input not ended:\n>         current = find next escape character in input\n>         memcpy from input value in the range of last to current\n>         last = current + 1\n>     copy remaining parts if no further escape is found\n\nThat would be true _only_ when \"find next escape and copy up to that\nbyte\" aka \"scanning once with optimized strchr(), and copying once\nwith optimized memmove()\" is faster than \"scanning once and copying\"\nloop.\n\nI was merely reacting to your use of memmove() in a very different\nloop, where if you unescape \"a\\b\\c\\defghijk\", your memmove() would\nmove \"efghijk\" many times.\n\n>>         ret = xmalloc(strlen(value));\n>\n> xmallocz at least?\n\nYes.  Also the handling of the terminating NUL may need to be\nupdated.  That is why I did not say \"replace yours with this\", but\nmerely \"along the lines of this\" ;-)\n"}]}