Re: [RFC/PATCH] pathspec: allow escaped query values
On Wed, Jun 1, 2016 at 5:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 30 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>> This change allows escaping characters by a backslash, such that the query
>>
>> git ls-files :(attr:whitespace=indent\,trail\,space)
>>
>> will match all path that have the value "indent,trail,space" for the
>> whitespace attribute. To accomplish this, we need to modify two places.
>> First `eat_long_magic` needs to not stop early upon seeing a comma or
>> closing paren that is escaped. As a second step we need to remove any
>> escaping from the attr value. For now we just remove any backslashes.
>>
>> Caveat: This doesn't allow for querying for values that have backslashes
>> in them, e.g.
>>
>> git ls-files :(attr:backslashes=\\)
>>
>> that would ask for matches that have the `backslashes` value set to '\'.
>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>
>> * This applies on top of sb/pathspec-label
>> * Junio does this come close to what you imagine for escaped commas?
>
> I, being lazy, would have used %44 for comma, which would have
> avoided touching higher level of the callchain. But "a backslash
> always quotes the next byte, if you want to express a backslash,
> double it" would probably be a more end-user friendly quoting
> mechanism.
Show 13 quoted lines
>
> I do not offhand understand why the second example does not show the
> paths with backslash attribute set to a single backslash, though.
>
>> - am->value = xstrdup(&attr[attr_len + 1]);
>> + am->value = attr_value_unquote(&attr[attr_len + 1]);
>> if (strchr(am->value, '\\'))
>> die(_("attr spec values must not contain backslashes"));
>
> IOW, the "backslash is forbidden for now" IIUC was added there so
> that we can introduce a quoting like this--once we decided that
> quoting mechanism is via backslashes and have quoting support,
> shouldn't the "values must not have backslash" just go?Show 15 quoted lines
>
>> *copyfrom && *copyfrom != ')';
>> copyfrom = nextat) {
>> size_t len = strcspn(copyfrom, ",)");
>> + while (len > 0 && copyfrom[len - 1] == '\\'
>> + && (copyfrom[len] == ',' || copyfrom[len] == ')'))
>> + len += strcspn(copyfrom + len + 1, ",)") + 1;
>
> Good that we can use ')' in values, too, but I think this gets this
> case wrong:
>
> :(attr:foo=\\,icase)
>
> where the value for 'foo' wants to be a single backslash, and comma
> is to introduce another magic, not part of the value for 'foo'.Right, with this patch this would be interpreted as
foo equal to ',icase'
>
> If you want to do the "backslash unconditionally quotes the next
> byte no matter what is quoted", you'd need to lose the strcspn()
> and iterate over the string yourself, I would think.
Okay, will do that instead.
>
>> if (copyfrom[len] == ',')
>> nextat = copyfrom + len + 1;
>> else
Stefan Beller <sbeller@google.com>