Re: [PATCH] attr: support quoting pathname patterns in C style

Nguyễn Thái Ngọc Duy [off-list ref] writes:

Full pattern must be quoted. So 'pat"t"ern attr' will give exactly
'pat"t"ern', not 'pattern'. Also clarify that leading whitespaces are
not part of the pattern and document comment syntax.

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 Obvious regression: patterns that begin with double quote will
 now work differently.

I'm really hesitant to pursue this route and break people's existing
setups, especially if the only benefit this patch tries to achieve is to
allow somebody to say:

    "Program Files/*.txt" ...some attr...

It is not worth the effort, risk and headache, especially because people
with such paths are probably already using

    Program?Files/*.txt	...some attr..

to match them.

On Nov 5, 2010, at 9:58 AM, Junio C Hamano wrote:

Nguyễn Thái Ngọc Duy [off-list ref] writes:

quoted

Full pattern must be quoted. So 'pat"t"ern attr' will give exactly
'pat"t"ern', not 'pattern'. Also clarify that leading whitespaces are
not part of the pattern and document comment syntax.

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
Obvious regression: patterns that begin with double quote will
now work differently.

I'm really hesitant to pursue this route and break people's existing
setups, especially if the only benefit this patch tries to achieve is to
allow somebody to say:

   "Program Files/*.txt" ...some attr...

It is not worth the effort, risk and headache, especially because people
with such paths are probably already using

   Program?Files/*.txt	...some attr..

to match them.

Would this actually break any existing setups? The only ones that are affected
are ones beginning with ", which I imagine would be rather rare. I personally
am in favor of having an unambiguous way to encode whitespace into the pattern.
Having to use ? has always struck me as being, well, not very good, especially
if you have 2 files that only differ at that character (e.g. file.1 and "file 1").

-Kevin Ballard

quoted

 Obvious regression: patterns that begin with double quote will
 now work differently.

I'm really hesitant to pursue this route and break people's existing
setups

If existing setups are an issue, there could be a config-property
"core.gitAttributesQuoting" to enable quoting which will only be set for
newly created repositories. Personally, I don't think this effort is
necessary. Probably there is not even a single .gitattributes with a
leading quotation mark. And if there is, it's easy to fix.

In any case, I think future git repositories and users will be grateful
for quoting support: after I noticed problems with a tool-generated(!)
.gitattributes files, it took me 5 minutes to try: \-quoting, "-quoting
and octal-quoting, but more than 1 hour of googling, looking at git
sources and finally writing an email to this list :)

Marc.


On 05.11.2010 17:58, Junio C Hamano wrote:

Nguyễn Thái Ngọc Duy [off-list ref] writes:

quoted

Full pattern must be quoted. So 'pat"t"ern attr' will give exactly
'pat"t"ern', not 'pattern'. Also clarify that leading whitespaces are
not part of the pattern and document comment syntax.

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 Obvious regression: patterns that begin with double quote will
 now work differently.

I'm really hesitant to pursue this route and break people's existing
setups, especially if the only benefit this patch tries to achieve is to
allow somebody to say:

    "Program Files/*.txt" ...some attr...

It is not worth the effort, risk and headache, especially because people
with such paths are probably already using

    Program?Files/*.txt	...some attr..

to match them.

Full pattern must be quoted. So 'pat"t"ern attr' will give exactly
'pat"t"ern', not 'pattern'.

If Git fails to unquote it, it warns users and takes the pattern
literally. This keeps existing patterns that begin with a double
quotation mark work until they get annoyed by the warnings and fix
their patterns.

Also clarify that leading whitespaces are not part of the pattern and
document comment syntax.

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 On Fri, Nov 05, 2010 at 09:58:46AM -0700, Junio C Hamano wrote:
 > Nguyễn Thái Ngọc Duy [off-list ref] writes:
 > 
 > > Full pattern must be quoted. So 'pat"t"ern attr' will give exactly
 > > 'pat"t"ern', not 'pattern'. Also clarify that leading whitespaces are
 > > not part of the pattern and document comment syntax.
 > >
 > > Signed-off-by: Nguyễn Thái Ngọc Duy [off-list ref]
 > > ---
 > >  Obvious regression: patterns that begin with double quote will
 > >  now work differently.
 > 
 > I'm really hesitant to pursue this route and break people's existing
 > setups

 How about this? No more breaking current setups.
  
 Documentation/gitattributes.txt |    8 +++++---
 attr.c                          |   32 ++++++++++++++++++++++++++++----
 t/t0003-attributes.sh           |   21 ++++++++++++++++++++-
 3 files changed, 53 insertions(+), 8 deletions(-)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index c80ca5d..f7954dd 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt

@@ -21,9 +21,11 @@ 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
-the path.
+separated by whitespaces. Leading and trailing whitespaces are
+ignored. Lines that begin with '#' are ignored. Patterns
+that begin with a double quotation mark are quoted in C style.
+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 6aff695..fdc4aae 100644
--- a/attr.c
+++ b/attr.c

@@ -2,6 +2,7 @@
 #include "cache.h"
 #include "exec_cmd.h"
 #include "attr.h"
+#include "quote.h"
 
 const char git_attr__true[] = "(builtin)true";
 const char git_attr__false[] = "\0(builtin)false";

@@ -181,21 +182,40 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
 {
 	int namelen;
 	int num_attr;
-	const char *cp, *name;
+	const char *cp, *name, *ep;
 	struct match_attr *res = NULL;
 	int pass;
 	int is_macro;
+	struct strbuf pattern = STRBUF_INIT;
 
 	cp = line + strspn(line, blank);
 	if (!*cp || *cp == '#')
 		return NULL;
 	name = cp;
-	namelen = strcspn(name, blank);
+	if (*cp == '"') {
+		if (unquote_c_style(&pattern, name, &ep)) {
+			fprintf(stderr, "Misquoted pattern at %s:%d\n"
+				"Pattern is taken literally.\n",
+				src, lineno);
+			namelen = strcspn(name, blank);
+			ep = name + namelen;
+		}
+		else {
+			namelen = ep - name;
+			name = pattern.buf;
+		}
+	}
+	else {
+		namelen = strcspn(name, blank);
+		ep = name + namelen;
+	}
+
 	if (strlen(ATTRIBUTE_MACRO_PREFIX) < namelen &&
 	    !prefixcmp(name, ATTRIBUTE_MACRO_PREFIX)) {
 		if (!macro_ok) {
 			fprintf(stderr, "%s not allowed: %s:%d\n",
 				name, src, lineno);
+			strbuf_release(&pattern);
 			return NULL;
 		}
 		is_macro = 1;

@@ -206,6 +226,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
 			fprintf(stderr,
 				"%.*s is not a valid attribute name: %s:%d\n",
 				namelen, name, src, lineno);
+			strbuf_release(&pattern);
 			return NULL;
 		}
 	}

@@ -215,12 +236,14 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
 	for (pass = 0; pass < 2; pass++) {
 		/* pass 0 counts and allocates, pass 1 fills */
 		num_attr = 0;
-		cp = name + namelen;
+		cp = ep;
 		cp = cp + strspn(cp, blank);
 		while (*cp) {
 			cp = parse_attr(src, lineno, cp, &num_attr, res);
-			if (!cp)
+			if (!cp) {
+				strbuf_release(&pattern);
 				return NULL;
+			}
 		}
 		if (pass)
 			break;

@@ -238,6 +261,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
 		res->is_macro = is_macro;
 		res->num_attr = num_attr;
 	}
+	strbuf_release(&pattern);
 	return res;
 }
 

diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 25205ac..7c55482 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh

@@ -10,17 +10,32 @@ attr_check () {
 	expect="$2"
 
 	git check-attr test -- "$path" >actual &&
-	echo "$path: test: $2" >expect &&
+	echo "$path: test: $expect" >expect &&
 	test_cmp expect actual
 
 }
 
+attr_check_quote () {
+
+	path="$1"
+	quoted_path="$2"
+	expect="$3"
+
+	git check-attr test -- "$path" >actual &&
+	echo "\"$quoted_path\": test: $expect" >expect &&
+	test_cmp expect actual
+
+}
 
 test_expect_success 'setup' '
 
 	mkdir -p a/b/d a/c &&
 	(
 		echo "[attr]notest !test"
+		echo "\"c	test=c"
+		echo "\" d \"	test=d"
+		echo " e	test=e"
+		echo " e\"	test=e"
 		echo "f	test=f"
 		echo "a/i test=a/i"
 		echo "onoff test -test"

@@ -44,6 +59,10 @@ test_expect_success 'setup' '
 
 test_expect_success 'attribute test' '
 
+	attr_check_quote \"c \\\"c c &&
+	attr_check " d " d &&
+	attr_check e e &&
+	attr_check_quote e\" e\\\" e &&
 	attr_check f f &&
 	attr_check a/f f &&
 	attr_check a/c/f f &&

-- 
1.7.3.2.210.g045198