[PATCH] attr: allow pattern escape using backslashes

[PATCH] attr: allow pattern escape using backslashes

[Nguyễn Thái Ngọc Duy]

Patterns in .gitattributes are separated by whitespaces, which makes
it impossible to specify exact spaces in the pattern. '?' can be used
as a workaround, but it matches other characters too. This patch makes
a space following a backslash part of the pattern, not a pattern
separator.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 A similar patch was posted twice (during freeze time iirc). I think
 this is a good change, so I will keep reposting until someone turns
 it down.

 We could use wildmatch for parsing here, which would support patterns
 like "Hello[ ]world". But that's not going to happen until wildmatch
 graduates and somebody brings it up again.

 Documentation/gitattributes.txt | 6 +++---
 attr.c                          | 8 +++++++-
 t/t0003-attributes.sh           | 5 +++++
 3 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 2698f63..113b1f8 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -20,9 +20,9 @@ Each line in `gitattributes` file is of form:
 
 	pattern	attr1 attr2 ...
 
-That is, a pattern followed by an attributes list,
-separated by whitespaces.  When the pattern matches the
-path in question, the attributes listed on the line are given to
+That is, a pattern followed by an attributes list, separated by
+whitespaces that are not quoted by a backslash. When the pattern matches
+the path in question, the attributes listed on the line are given to
 the path.
 
 Each attribute can be in one of these states for a given path:
diff --git a/attr.c b/attr.c
index 097ae87..776f34e 100644
--- a/attr.c
+++ b/attr.c
@@ -209,7 +209,13 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
 	if (!*cp || *cp == '#')
 		return NULL;
 	name = cp;
-	namelen = strcspn(name, blank);
+	namelen = 0;
+	while (name[namelen] != '\0' && !strchr(blank, name[namelen])) {
+		if (name[namelen] == '\\' && name[namelen + 1] != '\0')
+			namelen += 2;
+		else
+			namelen++;
+	}
 	if (strlen(ATTRIBUTE_MACRO_PREFIX) < namelen &&
 	    !prefixcmp(name, ATTRIBUTE_MACRO_PREFIX)) {
 		if (!macro_ok) {
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 807b8b8..6a5d8ab 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -24,6 +24,7 @@ test_expect_success 'setup' '
 		echo "offon -test test"
 		echo "no notest"
 		echo "A/e/F test=A/e/F"
+		echo "A\\ b test=space"
 	) >.gitattributes &&
 	(
 		echo "g test=a/g" &&
@@ -196,6 +197,10 @@ test_expect_success 'root subdir attribute test' '
 	attr_check subdir/a/i unspecified
 '
 
+test_expect_success 'quoting in pattern' '
+	attr_check "A b" space
+'
+
 test_expect_success 'negative patterns' '
 	echo "!f test=bar" >.gitattributes &&
 	test_must_fail git check-attr test -- f
-- 
1.8.0.rc2.23.g1fb49df

Re: [PATCH] attr: allow pattern escape using backslashes

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Patterns in .gitattributes are separated by whitespaces, which makes
> it impossible to specify exact spaces in the pattern. '?' can be used
> as a workaround, but it matches other characters too. This patch makes
> a space following a backslash part of the pattern, not a pattern
> separator.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>
>  A similar patch was posted twice (during freeze time iirc). I think
>  this is a good change, so I will keep reposting until someone turns
>  it down.
>
>  We could use wildmatch for parsing here, which would support patterns
>  like "Hello[ ]world". But that's not going to happen until wildmatch
>  graduates and somebody brings it up again.

Do people have any comment on this?  Do we do something similar in
the .gitignore files already?

Speaking of .gitignore, I recall that there was a hanging discussion
on allowing a pattern to name the directory that the .gitignore file
appears in, which I do not think we currently support.  With such a
feature, instead of listing "/junk" in the .gitignore file at the
top-level to say that everything inside the "junk" directory is
ignored by default, we could instead say "<this>" at the beginning
of the .gitignore file in the "junk" directory.  I think "* <attr>"
in the .gitattributes file in a directory causes "git check-attr ."
in that directory to report the <attr>, and we may want to have some
way to allow "git check-ignore ." to be affected by the .gitignore
file in the same directory in a similar way, but I do not have a
very strong opinion.  Do people have any comment on this?

It is obviously too late for this cycle to do anything on the
'master' front on this topic (or the .gitignore), but it would be
good to reach concensus early so that we know what we want to do in
the next cycle.

>  Documentation/gitattributes.txt | 6 +++---
>  attr.c                          | 8 +++++++-
>  t/t0003-attributes.sh           | 5 +++++
>  3 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
> index 2698f63..113b1f8 100644
> --- a/Documentation/gitattributes.txt
> +++ b/Documentation/gitattributes.txt
> @@ -20,9 +20,9 @@ Each line in `gitattributes` file is of form:
>  
>  	pattern	attr1 attr2 ...
>  
> -That is, a pattern followed by an attributes list,
> -separated by whitespaces.  When the pattern matches the
> -path in question, the attributes listed on the line are given to
> +That is, a pattern followed by an attributes list, separated by
> +whitespaces that are not quoted by a backslash. When the pattern matches
> +the path in question, the attributes listed on the line are given to
>  the path.
>  
>  Each attribute can be in one of these states for a given path:
> diff --git a/attr.c b/attr.c
> index 097ae87..776f34e 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -209,7 +209,13 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
>  	if (!*cp || *cp == '#')
>  		return NULL;
>  	name = cp;
> -	namelen = strcspn(name, blank);
> +	namelen = 0;
> +	while (name[namelen] != '\0' && !strchr(blank, name[namelen])) {
> +		if (name[namelen] == '\\' && name[namelen + 1] != '\0')
> +			namelen += 2;
> +		else
> +			namelen++;
> +	}
>  	if (strlen(ATTRIBUTE_MACRO_PREFIX) < namelen &&
>  	    !prefixcmp(name, ATTRIBUTE_MACRO_PREFIX)) {
>  		if (!macro_ok) {
> diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
> index 807b8b8..6a5d8ab 100755
> --- a/t/t0003-attributes.sh
> +++ b/t/t0003-attributes.sh
> @@ -24,6 +24,7 @@ test_expect_success 'setup' '
>  		echo "offon -test test"
>  		echo "no notest"
>  		echo "A/e/F test=A/e/F"
> +		echo "A\\ b test=space"
>  	) >.gitattributes &&
>  	(
>  		echo "g test=a/g" &&
> @@ -196,6 +197,10 @@ test_expect_success 'root subdir attribute test' '
>  	attr_check subdir/a/i unspecified
>  '
>  
> +test_expect_success 'quoting in pattern' '
> +	attr_check "A b" space
> +'
> +
>  test_expect_success 'negative patterns' '
>  	echo "!f test=bar" >.gitattributes &&
>  	test_must_fail git check-attr test -- f

Re: [PATCH] attr: allow pattern escape using backslashes

[Duy Nguyen]

On Sun, Feb 24, 2013 at 4:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Speaking of .gitignore, I recall that there was a hanging discussion
> on allowing a pattern to name the directory that the .gitignore file
> appears in, which I do not think we currently support.  With such a
> feature, instead of listing "/junk" in the .gitignore file at the
> top-level to say that everything inside the "junk" directory is
> ignored by default, we could instead say "<this>" at the beginning
> of the .gitignore file in the "junk" directory.

Shouldn't "/" alone in junk/.gitignore express that? It does not work,
but I think it's a natural interpretation of the syntax.

> I think "* <attr>"
> in the .gitattributes file in a directory causes "git check-attr ."
> in that directory to report the <attr>, and we may want to have some
> way to allow "git check-ignore ." to be affected by the .gitignore
> file in the same directory in a similar way, but I do not have a
> very strong opinion.  Do people have any comment on this?
-- 
Duy

Re: [PATCH] attr: allow pattern escape using backslashes

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Sun, Feb 24, 2013 at 4:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Speaking of .gitignore, I recall that there was a hanging discussion
>> on allowing a pattern to name the directory that the .gitignore file
>> appears in, which I do not think we currently support.  With such a
>> feature, instead of listing "/junk" in the .gitignore file at the
>> top-level to say that everything inside the "junk" directory is
>> ignored by default, we could instead say "<this>" at the beginning
>> of the .gitignore file in the "junk" directory.
>
> Shouldn't "/" alone in junk/.gitignore express that? It does not work,
> but I think it's a natural interpretation of the syntax.

Yup, there is nothing that you can plug to the "<this>" in the above
to make it mean "junk" directory is ignored.  A trailing "/" is
removed after noting the fact that the entry is about a directory
and leaves an empty string, and it would be OK to define that an
empty string matches the directory the gitignore file appears in.