.gitattributes escape character?

.gitattributes escape character?

[Marc Strapetz]

Is there an escape character which may be used in .gitattributes to
escape e.g. the space-character? Could octal-escaping help here (I
didn't succeed)? Thanks for any hints.

Marc.

Re: .gitattributes escape character?

[Nguyen Thai Ngoc Duy]

On Wed, Nov 3, 2010 at 7:24 PM, Marc Strapetz <marc.strapetz@syntevo.com> wrote:
> Is there an escape character which may be used in .gitattributes to
> escape e.g. the space-character? Could octal-escaping help here (I
> didn't succeed)? Thanks for any hints.

You mean escape the path part in .gitattributes? Sorry, no.

I think we can teach git about path quoting though. A leading double
quote means the path is quoted, C-style.
-- 
Duy

Re: .gitattributes escape character?

[Marc Strapetz]

>> Is there an escape character which may be used in .gitattributes to
>> escape e.g. the space-character? Could octal-escaping help here (I
>> didn't succeed)? Thanks for any hints.
> 
> You mean escape the path part in .gitattributes? Sorry, no.
> 
> I think we can teach git about path quoting though. A leading double
> quote means the path is quoted, C-style.

But this is not supposed to work already? At least, when quoting the
whole path:

"/a file" -text

git check-attr tells me 'file" is not a valid attribute'.

Marc.

Re: .gitattributes escape character?

[Kevin Ballard]

On Nov 3, 2010, at 8:47 AM, Nguyen Thai Ngoc Duy wrote:

> On Wed, Nov 3, 2010 at 7:24 PM, Marc Strapetz <marc.strapetz@syntevo.com> wrote:
>> Is there an escape character which may be used in .gitattributes to
>> escape e.g. the space-character? Could octal-escaping help here (I
>> didn't succeed)? Thanks for any hints.
> 
> You mean escape the path part in .gitattributes? Sorry, no.
> 
> I think we can teach git about path quoting though. A leading double
> quote means the path is quoted, C-style.

I agree that gitattributes needs to learn about C-style quoting. However, in the
meantime you can just replace a space with ? as it's actually a pattern. In
other words, "test/file with spaces" can be represented as test/file?with?spaces

-Kevin Ballard

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

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

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 <pclouds@gmail.com>
---
 Obvious regression: patterns that begin with double quote will
 now work differently.

 Documentation/gitattributes.txt |    8 +++++---
 attr.c                          |   25 +++++++++++++++++++++----
 t/t0003-attributes.sh           |   25 ++++++++++++++++++++++++-
 3 files changed, 50 insertions(+), 8 deletions(-)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index c80ca5d..bc6c65c 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 quote 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..f3063d8 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,33 @@ 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))
+			die("Broken attribute line: %s", line);
+		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 +219,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 +229,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 +254,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..a57f358 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -10,17 +10,37 @@ 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 'open-quoted pathname' '
+	echo "\"a test=a" >.gitattributes &&
+	test_must_fail attr_check a a
+'
+
 
 test_expect_success 'setup' '
 
 	mkdir -p a/b/d a/c &&
 	(
 		echo "[attr]notest !test"
+		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 +64,9 @@ test_expect_success 'setup' '
 
 test_expect_success 'attribute test' '
 
+	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

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

[Sverre Rabbelier]

Heya,

2010/11/4 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
>  Obvious regression: patterns that begin with double quote will
>  now work differently.

It's a behavior change, not a regression (a regression is an
_unintended_ behavior change).

-- 
Cheers,

Sverre Rabbelier

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

[Eric Sunshine]

On 11/4/2010 9:55 AM, Nguyễn Thái Ngọc Duy wrote:
> 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<pclouds@gmail.com>
> diff --git a/attr.c b/attr.c
> index 6aff695..f3063d8 100644
> --- a/attr.c
> +++ b/attr.c
>   	name = cp;
> -	namelen = strcspn(name, blank);
> +	if (*cp == '"') {
> +		if (unquote_c_style(&pattern, name,&ep))
> +			die("Broken attribute line: %s", line);

The error message is perhaps a bit too generic, indicating only that 
_something_ on the line caused a problem. Mentioning that bad quoting of 
'name' was at fault would help focus the user's attention where it is 
needed.

-- ES

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

[Nguyen Thai Ngoc Duy]

2010/11/5 Eric Sunshine <ericsunshine@gmail.com>:
> On 11/4/2010 9:55 AM, Nguyễn Thái Ngọc Duy wrote:
>>
>> 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<pclouds@gmail.com>
>> diff --git a/attr.c b/attr.c
>> index 6aff695..f3063d8 100644
>> --- a/attr.c
>> +++ b/attr.c
>>        name = cp;
>> -       namelen = strcspn(name, blank);
>> +       if (*cp == '"') {
>> +               if (unquote_c_style(&pattern, name,&ep))
>> +                       die("Broken attribute line: %s", line);
>
> The error message is perhaps a bit too generic, indicating only that
> _something_ on the line caused a problem. Mentioning that bad quoting of
> 'name' was at fault would help focus the user's attention where it is
> needed.

Really.
-- 
Duy

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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> 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 <pclouds@gmail.com>
> ---
>  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.

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

[Kevin Ballard]

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

> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> 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 <pclouds@gmail.com>
>> ---
>> 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

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

[Marc Strapetz]

>>  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 <pclouds@gmail.com> 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 <pclouds@gmail.com>
>> ---
>>  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.
> 
> 

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

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

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 <pclouds@gmail.com>
---
 On Fri, Nov 05, 2010 at 09:58:46AM -0700, Junio C Hamano wrote:
 > Nguyễn Thái Ngọc Duy <pclouds@gmail.com> 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 <pclouds@gmail.com>
 > > ---
 > >  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

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

[Junio C Hamano]

Kevin Ballard <kevin@sb.org> writes:

> Would this actually break any existing setups? The only ones that are affected
> are ones beginning with ", which I imagine would be rather rare.

No regression policy means just that.  We try not to break "rather rare"
people, only to add support for different kinds of "rather rare" setups,
especially when the latter have working workarounds.

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

[Kevin Ballard]

On Nov 8, 2010, at 10:40 AM, Junio C Hamano wrote:

> Kevin Ballard <kevin@sb.org> writes:
> 
>> Would this actually break any existing setups? The only ones that are affected
>> are ones beginning with ", which I imagine would be rather rare.
> 
> No regression policy means just that.  We try not to break "rather rare"
> people, only to add support for different kinds of "rather rare" setups,
> especially when the latter have working workarounds.

I'm still not convinced that the workaround is appropriate. If I have a file
named "file 1" that needs special attributes, then it is impossible to leave
those attributes unspecified for, e.g. "file.1". I'd have to set them on
"file 1" and then immediately unset them for "file.1". And as unspecified
behaves differently than unset, this can be a problem. Especially because if
I add any other files in the future that match "file?1" I have to remember
to go through gitattributes and re-set all of the ones already specified to
something appropriate for this file.

Sure, this is probably a rare case, but so is filenames beginning with a ".

Basically what I'm trying to say is, we already break one particular
"rather rare" setup. I would love to come up with a solution that supports
both setups, but I don't know if one exists outside of using a config
variable to control whether git attribute patterns support quoting (a solution
I am not particularly fond of for this case).

-Kevin Ballard

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

[Johannes Sixt]

Am 11/8/2010 22:56, schrieb Kevin Ballard:
> Basically what I'm trying to say is, we already break one particular
> "rather rare" setup. I would love to come up with a solution that supports
> both setups, but I don't know if one exists outside of using a config
> variable to control whether git attribute patterns support quoting (a solution
> I am not particularly fond of for this case).

Can we perhaps have a pseudo-attribute 'quoted-names' that is to be used
like this:

* quoted-names
"file 1" binary
file.1 -diff

Its meaning would be that the remainder of the current .gitattributes file
is to be parsed with C style path quoting enabled. The glob given with
this attribute is irrelevant and ignored.

I didn't check whether old gits would ignore this unknown attribute.

-- Hannes

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

[Kevin Ballard]

On Nov 8, 2010, at 11:48 PM, Johannes Sixt wrote:

> Am 11/8/2010 22:56, schrieb Kevin Ballard:
>> Basically what I'm trying to say is, we already break one particular
>> "rather rare" setup. I would love to come up with a solution that supports
>> both setups, but I don't know if one exists outside of using a config
>> variable to control whether git attribute patterns support quoting (a solution
>> I am not particularly fond of for this case).
> 
> Can we perhaps have a pseudo-attribute 'quoted-names' that is to be used
> like this:
> 
> * quoted-names
> "file 1" binary
> file.1 -diff
> 
> Its meaning would be that the remainder of the current .gitattributes file
> is to be parsed with C style path quoting enabled. The glob given with
> this attribute is irrelevant and ignored.
> 
> I didn't check whether old gits would ignore this unknown attribute.

This would certainly solve the regression issue, but from the perspective of
a brand new user of git this would make absolutely no sense and would be
a huge wart upon consistency.

-Kevin Ballard

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

[Junio C Hamano]

Kevin Ballard <kevin@sb.org> writes:

> Basically what I'm trying to say is, we already break one particular
> "rather rare" setup.

Let's try again.  One particular "rather rare" setup never worked.  As it
is "rather rare", we do not really care that deeply to make that work.
Another particular "rather rare" setup used to work.  Even though we do
not really care that deeply to keep it working, is it worth breaking it?

> ... I would love to come up with a solution that supports both setups,
> but I don't know if one exists outside of using a config variable to
> control whether git attribute patterns support quoting (a solution I am
> not particularly fond of for this case).

Controlling this with a config would be a disaster.  It would mean that
the same version of updated git would interpret the same .gitattributes
file differently, and the situation will continue forever.  Compared to
that, the idea J6t brought up would be far easier to swallow.  Older
vintage of git will misbehave on "rather rare" paths upon seeing a cquoted
pattern (i.e. the pattern will not match the intended paths, and will
instead match "rather rare" paths that begin with dq) but that is no worse
than what we already have.  And newer vintages of git will interpret the
attribute file written with that magic exactly the same way everywhere,
regardless of the configuration setting.

Having said all that, I actually am in favor of using cquote.  It would
have been what we should have done in the first place.

My preference is to admit that we made a mistake of not using cquote when
we originally introduced .gitattributes, clearly state that the version of
git with this new backward incompatible feature will _break_ rare existing
setups if they had paths whose name begin with a dq and applied attributes
to them, and use cquote unconditionally, perhaps with a version bump.

I just didn't like the tone of saying "Nobody would have used such an
insane path anyway so we don't care".  I am Ok if our message is "Sorry,
this release would break if you used to rely on this; we think it is
unlikely and are hoping that most of you won't be affected".

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

[Kevin Ballard]

On Nov 9, 2010, at 4:07 PM, Junio C Hamano wrote:

> Kevin Ballard <kevin@sb.org> writes:
> 
>> Basically what I'm trying to say is, we already break one particular
>> "rather rare" setup.
> 
> Let's try again.  One particular "rather rare" setup never worked.  As it
> is "rather rare", we do not really care that deeply to make that work.
> Another particular "rather rare" setup used to work.  Even though we do
> not really care that deeply to keep it working, is it worth breaking it?

My feeling is yes. My suspicion is that paths that begin with a dq are extremely
rare, and ones that have custom git attributes set on them are rarer still.
I would rather make the change to support an unambiguous format to specify
any possible path and simply apologize if anyone is actually hit by this.

>> ... I would love to come up with a solution that supports both setups,
>> but I don't know if one exists outside of using a config variable to
>> control whether git attribute patterns support quoting (a solution I am
>> not particularly fond of for this case).
> 
> Controlling this with a config would be a disaster.  It would mean that
> the same version of updated git would interpret the same .gitattributes
> file differently, and the situation will continue forever.

Precisely why I was not fond of this suggestion.

> Compared to
> that, the idea J6t brought up would be far easier to swallow.  Older
> vintage of git will misbehave on "rather rare" paths upon seeing a cquoted
> pattern (i.e. the pattern will not match the intended paths, and will
> instead match "rather rare" paths that begin with dq) but that is no worse
> than what we already have.  And newer vintages of git will interpret the
> attribute file written with that magic exactly the same way everywhere,
> regardless of the configuration setting.
> 
> Having said all that, I actually am in favor of using cquote.  It would
> have been what we should have done in the first place.

Agreed.

> My preference is to admit that we made a mistake of not using cquote when
> we originally introduced .gitattributes, clearly state that the version of
> git with this new backward incompatible feature will _break_ rare existing
> setups if they had paths whose name begin with a dq and applied attributes
> to them, and use cquote unconditionally, perhaps with a version bump.
> 
> I just didn't like the tone of saying "Nobody would have used such an
> insane path anyway so we don't care".  I am Ok if our message is "Sorry,
> this release would break if you used to rely on this; we think it is
> unlikely and are hoping that most of you won't be affected".

Also agreed. I never meant to imply that we didn't care about paths beginning
with a dq, I just thought it was unlikely enough that I would never run into
someone using such a path ;)

In any case, I just took a look at Nguyễn's patch and it looks good to me.

-Kevin Ballard