Subject: Re: [RFC/PATCH] pathspec: allow escaped query values

From: Junio C Hamano <[email-masked]>
Date: Wed, 01 Jun 2016 17:33:13 -0700

Stefan Beller <[email-masked]> 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 <[email-masked]>
> ---
>
>  * 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.

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?

>  	     *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'.

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.

>  		if (copyfrom[len] == ',')
>  			nextat = copyfrom + len + 1;
>  		else