threads / discuss / 42513

[PATCHv2] pathspec: allow escaped query values

Subject: [PATCHv2] pathspec: allow escaped query values

## tl;dr

5 messages between Jun 2, 2016 and Jun 2, 2016.

replies: 4people: 2as markdown or json

Stefan Beller· Jun 2, 2016, 21:30 UTC · lore
In our own .gitattributes file we have attributes such as:
    *.[ch] whitespace=indent,trail,space

When querying for attributes we want to be able to ask for the exact value, i.e.

    git ls-files :(attr:whitespace=indent,trail,space)

should work, but the commas are used in the attr magic to introduce the next attr, such that this query currently fails with

fatal: Invalid pathspec magic 'trail' in ':(attr:whitespace=indent,trail,space)'
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.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

So here is the "escaping only, but escaping done right" version. (It goes on top of sb/pathspec-label)

Thanks, Stefan

 pathspec.c                      | 44 +++++++++++++++++++++++++++++++++++++----
 t/t6134-pathspec-with-labels.sh | 23 +++++++++++++++++++++
 2 files changed, 63 insertions(+), 4 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 326863a..45bd775 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -89,6 +89,43 @@ static void prefix_short_magic(struct strbuf *sb, int prefixlen,
 	strbuf_addf(sb, ",prefix:%d)", prefixlen);
 }
 
+static size_t strcspn_escaped(const char *s, const char *reject)
+{
+	const char *i, *j;
+
+	for (i = s; *i; i++) {
+		/* skip escaped the character */
+		if (i[0] == '\\' && i[1]) {
+			i++;
+			continue;
+		}
+		/* see if any of the chars matches the current character */
+		for (j = reject; *j; j++)
+			if (!*i || *i == *j)
+				return i - s;
+	}
+	return i - s;
+}
+
+static char *attr_value_unescape(const char *value)
+{
+	char *i, *ret = xstrdup(value);
+
+	for (i = ret; *i; i++) {
+		if (i[0] == '\\') {
+			if (!i[1])
+				die(_("Escape character '\\' not allowed as "
+				      "last character in attr value"));
+
+			/* remove the backslash */
+			memmove(i, i + 1, strlen(i));
+			/* and ignore the character after that */
+			i++;
+		}
+	}
+	return ret;
+}
+
 static void parse_pathspec_attr_match(struct pathspec_item *item, const char *value)
 {
 	struct string_list_item *si;
@@ -131,10 +168,9 @@ static void parse_pathspec_attr_match(struct pathspec_item *item, const char *va
 			if (attr[attr_len] != '=')
 				am->match_mode = MATCH_SET;
 			else {
+				const char *v = &attr[attr_len + 1];
 				am->match_mode = MATCH_VALUE;
-				am->value = xstrdup(&attr[attr_len + 1]);
-				if (strchr(am->value, '\\'))
-					die(_("attr spec values must not contain backslashes"));
+				am->value = attr_value_unescape(v);
 			}
 			break;
 		}
@@ -166,7 +202,7 @@ static void eat_long_magic(struct pathspec_item *item, const char *elt,
 	for (copyfrom = elt + 2;
 	     *copyfrom && *copyfrom != ')';
 	     copyfrom = nextat) {
-		size_t len = strcspn(copyfrom, ",)");
+		size_t len = strcspn_escaped(copyfrom, ",)");
 		if (copyfrom[len] == ',')
 			nextat = copyfrom + len + 1;
 		else
diff --git a/t/t6134-pathspec-with-labels.sh b/t/t6134-pathspec-with-labels.sh
index a5c9632..cbea858 100755
--- a/t/t6134-pathspec-with-labels.sh
+++ b/t/t6134-pathspec-with-labels.sh
@@ -163,4 +163,27 @@ test_expect_success 'abort on asking for wrong magic' '
 	test_must_fail git ls-files . ":(attr:!label=foo)"
 '
 
+test_expect_success 'check attribute list' '
+	cat <<-EOF >>.gitattributes &&
+	* whitespace=indent,trail,space
+	EOF
+	cat .gitattributes &&
+	git ls-files ":(attr:whitespace=indent\,trail\,space)" >actual &&
+	git ls-files >expect &&
+	test_cmp expect actual
+'
+test_expect_success 'wrong escaping caught' '
+	# Pass one backslash to git to fail with a missing closing paren
+	test_must_fail git ls-files ":(attr:marked-with-backslash=\\)" 2>actual &&
+	test_i18ngrep Missing actual
+'
+test_expect_success 'check escaped backslash' '
+	cat <<-EOF >>.gitattributes &&
+	/sub/* marked-with-backslash=\\
+	EOF
+	git ls-files ":(attr:marked-with-backslash=\\\\)" >actual &&
+	git ls-files sub/ >expect &&
+	test_cmp expect actual
+'
+
 test_done

-- 
2.8.2.126.gaa5c87d.dirty
Junio C Hamano· Jun 2, 2016, 21:54 UTC · re: Stefan Beller · lore

Re: [PATCHv2] pathspec: allow escaped query values

Stefan Beller <sbeller@google.com> writes:
Show 14 quoted lines
> In our own .gitattributes file we have attributes such as:
>
>     *.[ch] whitespace=indent,trail,space
>
> When querying for attributes we want to be able to ask for the exact
> value, i.e.
>
>     git ls-files :(attr:whitespace=indent,trail,space)
>
> should work, but the commas are used in the attr magic to introduce
> the next attr, ...
> ...
> So here is the "escaping only, but escaping done right" version.
> (It goes on top of sb/pathspec-label)

The phrase "should work" is probably a bit too strong (I'd have said "it would be nice if this worked"), as we do not have to even support comma for our immediately expected use cases. Allowing it merely makes a casual test using our own .gitattributes easier.

> +static size_t strcspn_escaped(const char *s, const char *reject)
Perhaps s/reject/stop/?
Show 13 quoted lines
> +{
> +	const char *i, *j;
> +
> +	for (i = s; *i; i++) {
> +		/* skip escaped the character */
> +		if (i[0] == '\\' && i[1]) {
> +			i++;
> +			continue;
> +		}
> +		/* see if any of the chars matches the current character */
> +		for (j = reject; *j; j++)
> +			if (!*i || *i == *j)
> +				return i - s;

I somehow doubt that *i can be NUL here. In any case, this looks more like

	/* is *i is one of the stop codon? */
        if (strchr(stop, *i))
		break;
> +	}
> +	return i - s;
> +}
Show 19 quoted lines
> +static char *attr_value_unescape(const char *value)
> +{
> +	char *i, *ret = xstrdup(value);
> +
> +	for (i = ret; *i; i++) {
> +		if (i[0] == '\\') {
> +			if (!i[1])
> +				die(_("Escape character '\\' not allowed as "
> +				      "last character in attr value"));
> +
> +			/* remove the backslash */
> +			memmove(i, i + 1, strlen(i));
> +			/* and ignore the character after that */
> +			i++;
> +		}
> +	}
> +	return ret;
> +}
> +

Repeated memmove() and strlen() somehow bothers me. Would there be a more efficient and straight-forward way to do this, perhaps along the lines of this instead?

	const char *src;
        char *dst, *ret;
	ret = xmalloc(strlen(value));
        for (src = value, dst = ret; *src; src++, dst++) {
		if (*src == '\\') {
                	if (!src[1])
                        	die();
			src++;
		}
		if (*src && invalid_value_char(*src))
			die("cannot use '%c' for value matching", *src)
                *dst = *src;
	}
        *dst = '\0'
        return ret;

Even though I earlier said "Now we have an unquote mechanism, we can open the floodgate by lifting the "no backslash in value" check, I now realize that we do want to keep some escape hatch for us to extend the quoting syntax even more later, so perhaps with something like this:

	static inline int invalid_value_char(const char ch)
        {
        	if (isalnum(ch) || strchr(",-_", ch))
                	return 0;
		return -1;
	}
Thanks.
Stefan Beller· Jun 2, 2016, 22:10 UTC · re: Junio C Hamano · lore

Re: [PATCHv2] pathspec: allow escaped query values

On Thu, Jun 2, 2016 at 2:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 25 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>> In our own .gitattributes file we have attributes such as:
>>
>>     *.[ch] whitespace=indent,trail,space
>>
>> When querying for attributes we want to be able to ask for the exact
>> value, i.e.
>>
>>     git ls-files :(attr:whitespace=indent,trail,space)
>>
>> should work, but the commas are used in the attr magic to introduce
>> the next attr, ...
>> ...
>> So here is the "escaping only, but escaping done right" version.
>> (It goes on top of sb/pathspec-label)
>
> The phrase "should work" is probably a bit too strong (I'd have said
> "it would be nice if this worked"), as we do not have to even
> support comma for our immediately expected use cases.  Allowing it
> merely makes a casual test using our own .gitattributes easier.
>
>> +static size_t strcspn_escaped(const char *s, const char *reject)
>
> Perhaps s/reject/stop/?

I took the signature from the strcspn man page, and my version of the man page has `reject` there, `stop` certainly works too

Show 21 quoted lines
>
>> +{
>> +     const char *i, *j;
>> +
>> +     for (i = s; *i; i++) {
>> +             /* skip escaped the character */
>> +             if (i[0] == '\\' && i[1]) {
>> +                     i++;
>> +                     continue;
>> +             }
>> +             /* see if any of the chars matches the current character */
>> +             for (j = reject; *j; j++)
>> +                     if (!*i || *i == *j)
>> +                             return i - s;
>
> I somehow doubt that *i can be NUL here.  In any case, this looks
> more like
>
>         /* is *i is one of the stop codon? */
>         if (strchr(stop, *i))
>                 break;
right, that seems easier.
Show 58 quoted lines
>
>> +     }
>> +     return i - s;
>> +}
>
>> +static char *attr_value_unescape(const char *value)
>> +{
>> +     char *i, *ret = xstrdup(value);
>> +
>> +     for (i = ret; *i; i++) {
>> +             if (i[0] == '\\') {
>> +                     if (!i[1])
>> +                             die(_("Escape character '\\' not allowed as "
>> +                                   "last character in attr value"));
>> +
>> +                     /* remove the backslash */
>> +                     memmove(i, i + 1, strlen(i));
>> +                     /* and ignore the character after that */
>> +                     i++;
>> +             }
>> +     }
>> +     return ret;
>> +}
>> +
>
> Repeated memmove() and strlen() somehow bothers me.  Would there be
> a more efficient and straight-forward way to do this, perhaps along
> the lines of this instead?
>
>         const char *src;
>         char *dst, *ret;
>
>         ret = xmalloc(strlen(value));
>         for (src = value, dst = ret; *src; src++, dst++) {
>                 if (*src == '\\') {
>                         if (!src[1])
>                                 die();
>                         src++;
>                 }
>                 if (*src && invalid_value_char(*src))
>                         die("cannot use '%c' for value matching", *src)
>                 *dst = *src;
>         }
>         *dst = '\0'
>         return ret;
>
> Even though I earlier said "Now we have an unquote mechanism, we can
> open the floodgate by lifting the "no backslash in value" check, I
> now realize that we do want to keep some escape hatch for us to
> extend the quoting syntax even more later, so perhaps with something
> like this:
>
>         static inline int invalid_value_char(const char ch)
>         {
>                 if (isalnum(ch) || strchr(",-_", ch))
>                         return 0;
>                 return -1;
>         }
Makes sense.

Later on we could have : or ; for an and/or of the values and parens and such, so the invalid_value_char makes sense.

>
> Thanks.

Thanks, Stefan

Stefan Beller· Jun 2, 2016, 22:53 UTC · re: Junio C Hamano · lore

Re: [PATCHv2] pathspec: allow escaped query values

On Thu, Jun 2, 2016 at 2:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 74 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>> In our own .gitattributes file we have attributes such as:
>>
>>     *.[ch] whitespace=indent,trail,space
>>
>> When querying for attributes we want to be able to ask for the exact
>> value, i.e.
>>
>>     git ls-files :(attr:whitespace=indent,trail,space)
>>
>> should work, but the commas are used in the attr magic to introduce
>> the next attr, ...
>> ...
>> So here is the "escaping only, but escaping done right" version.
>> (It goes on top of sb/pathspec-label)
>
> The phrase "should work" is probably a bit too strong (I'd have said
> "it would be nice if this worked"), as we do not have to even
> support comma for our immediately expected use cases.  Allowing it
> merely makes a casual test using our own .gitattributes easier.
>
>> +static size_t strcspn_escaped(const char *s, const char *reject)
>
> Perhaps s/reject/stop/?
>
>> +{
>> +     const char *i, *j;
>> +
>> +     for (i = s; *i; i++) {
>> +             /* skip escaped the character */
>> +             if (i[0] == '\\' && i[1]) {
>> +                     i++;
>> +                     continue;
>> +             }
>> +             /* see if any of the chars matches the current character */
>> +             for (j = reject; *j; j++)
>> +                     if (!*i || *i == *j)
>> +                             return i - s;
>
> I somehow doubt that *i can be NUL here.  In any case, this looks
> more like
>
>         /* is *i is one of the stop codon? */
>         if (strchr(stop, *i))
>                 break;
>
>> +     }
>> +     return i - s;
>> +}
>
>> +static char *attr_value_unescape(const char *value)
>> +{
>> +     char *i, *ret = xstrdup(value);
>> +
>> +     for (i = ret; *i; i++) {
>> +             if (i[0] == '\\') {
>> +                     if (!i[1])
>> +                             die(_("Escape character '\\' not allowed as "
>> +                                   "last character in attr value"));
>> +
>> +                     /* remove the backslash */
>> +                     memmove(i, i + 1, strlen(i));
>> +                     /* and ignore the character after that */
>> +                     i++;
>> +             }
>> +     }
>> +     return ret;
>> +}
>> +
>
> Repeated memmove() and strlen() somehow bothers me.  Would there be
> a more efficient and straight-forward way to do this, perhaps along
> the lines of this instead?

Thinking about efficiency, I have the believe that memmove can be faster than a `*src=*dst` thing we do ourselves as it may have access to specialized assembly instructions to move larger chunks of memory or such.

So I think ideally we would do a block copy between the escape characters (sketched as:)

    last = input
    while input not ended:
        current = find next escape character in input
        memcpy from input value in the range of last to current
        last = current + 1
    copy remaining parts if no further escape is found
It doesn't seem worth the effort to get it right though.
Show 5 quoted lines
>
>         const char *src;
>         char *dst, *ret;
>
>         ret = xmalloc(strlen(value));
xmallocz at least?
Show 13 quoted lines
>         for (src = value, dst = ret; *src; src++, dst++) {
>                 if (*src == '\\') {
>                         if (!src[1])
>                                 die();
>                         src++;
>                 }
>                 if (*src && invalid_value_char(*src))
>                         die("cannot use '%c' for value matching", *src)
>                 *dst = *src;
>         }
>         *dst = '\0'
>         return ret;
>

Thanks, Stefan

Junio C Hamano· Jun 2, 2016, 23:01 UTC · re: Stefan Beller · lore

Re: [PATCHv2] pathspec: allow escaped query values

Stefan Beller <sbeller@google.com> writes:
> Thinking about efficiency, I have the believe that memmove can be faster
> than a `*src=*dst` thing we do ourselves as it may have access to specialized
> assembly instructions to move larger chunks of memory or such.
Show 9 quoted lines
> So I think ideally we would do a block copy between the escape characters
> (sketched as:)
>
>     last = input
>     while input not ended:
>         current = find next escape character in input
>         memcpy from input value in the range of last to current
>         last = current + 1
>     copy remaining parts if no further escape is found

That would be true _only_ when "find next escape and copy up to that byte" aka "scanning once with optimized strchr(), and copying once with optimized memmove()" is faster than "scanning once and copying" loop.

I was merely reacting to your use of memmove() in a very different loop, where if you unescape "a\b\c\defghijk", your memmove() would move "efghijk" many times.

>>         ret = xmalloc(strlen(value));
>
> xmallocz at least?

Yes. Also the handling of the terminating NUL may need to be updated. That is why I did not say "replace yours with this", but merely "along the lines of this" ;-)

← back to recent threads