[PATCH] Support \ in non-wildcard .gitignore entries

[PATCH] Support \ in non-wildcard .gitignore entries

[Finn Arne Gangstad]

If you had an exclude-pattern with a backslash in it, e.g. "\#foo",
this would not work, since git would do a strcmp of the exclude pattern
and the filename. Only wildcard patterns were matched with fnmatch,
which does the right thing with backslashes. We now also treat all patterns
containing backslashes as wildcards.

De-escaping the pattern while reading the .gitignore file is error prone,
since that would break patterns with both backslashes and wildcards.
E.g. "\\*.c" would be translated to "\*.c" before fnmatch got it,
and would change the meaning of the rule dramatically.

Signed-off-by: Finn Arne Gangstad <finnag@pvv.org>
---
 dir.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/dir.c b/dir.c
index cfd1ea5..2245749 100644
--- a/dir.c
+++ b/dir.c
@@ -137,7 +137,7 @@ int match_pathspec(const char **pathspec, const char *name, int namelen,
 
 static int no_wildcard(const char *string)
 {
-	return string[strcspn(string, "*?[{")] == '\0';
+	return string[strcspn(string, "*?[{\\")] == '\0';
 }
 
 void add_exclude(const char *string, const char *base,
-- 
1.6.2.rc0.11.g68cbb.dirty

Re: [PATCH] Support \ in non-wildcard .gitignore entries

[Johannes Schindelin]

Hi,

On Tue, 10 Feb 2009, Finn Arne Gangstad wrote:

> If you had an exclude-pattern with a backslash in it, e.g. "\#foo",
> this would not work, since git would do a strcmp of the exclude pattern
> and the filename. Only wildcard patterns were matched with fnmatch,
> which does the right thing with backslashes. We now also treat all patterns
> containing backslashes as wildcards.
> 
> De-escaping the pattern while reading the .gitignore file is error prone,
> since that would break patterns with both backslashes and wildcards.
> E.g. "\\*.c" would be translated to "\*.c" before fnmatch got it,
> and would change the meaning of the rule dramatically.

I am not sure I understand (maybe a test case would help, but that test 
case would have to be disabled on Windows, I guess):

You mean that '\#abc' would match '\#abc', but '\#abc*' would not?

Ciao,
Dscho

Re: [PATCH] Support \ in non-wildcard .gitignore entries

[Finn Arne Gangstad]

On Tue, Feb 10, 2009 at 01:56:36PM +0100, Johannes Schindelin wrote:
> Hi,
> 
> On Tue, 10 Feb 2009, Finn Arne Gangstad wrote:
> 
> > If you had an exclude-pattern with a backslash in it, e.g. "\#foo",
> > this would not work, since git would do a strcmp of the exclude pattern
> > and the filename. Only wildcard patterns were matched with fnmatch,
> > which does the right thing with backslashes. We now also treat all patterns
> > containing backslashes as wildcards.
> > 
> > De-escaping the pattern while reading the .gitignore file is error prone,
> > since that would break patterns with both backslashes and wildcards.
> > E.g. "\\*.c" would be translated to "\*.c" before fnmatch got it,
> > and would change the meaning of the rule dramatically.
> 
> I am not sure I understand (maybe a test case would help, but that test 
> case would have to be disabled on Windows, I guess):
> 
> You mean that '\#abc' would match '\#abc', but '\#abc*' would not?

Currently, \#abc does not match a file named #abc, but \#abc* does.
With the patch, both will match.

- Finn Arne

Re: [PATCH] Support \ in non-wildcard .gitignore entries

[Johannes Schindelin]

Hi,

On Tue, 10 Feb 2009, Finn Arne Gangstad wrote:

> On Tue, Feb 10, 2009 at 01:56:36PM +0100, Johannes Schindelin wrote:
> > Hi,
> > 
> > On Tue, 10 Feb 2009, Finn Arne Gangstad wrote:
> > 
> > > If you had an exclude-pattern with a backslash in it, e.g. "\#foo",
> > > this would not work, since git would do a strcmp of the exclude pattern
> > > and the filename. Only wildcard patterns were matched with fnmatch,
> > > which does the right thing with backslashes. We now also treat all patterns
> > > containing backslashes as wildcards.
> > > 
> > > De-escaping the pattern while reading the .gitignore file is error prone,
> > > since that would break patterns with both backslashes and wildcards.
> > > E.g. "\\*.c" would be translated to "\*.c" before fnmatch got it,
> > > and would change the meaning of the rule dramatically.
> > 
> > I am not sure I understand (maybe a test case would help, but that test 
> > case would have to be disabled on Windows, I guess):
> > 
> > You mean that '\#abc' would match '\#abc', but '\#abc*' would not?
> 
> Currently, \#abc does not match a file named #abc, but \#abc* does.
> With the patch, both will match.

Ah, so I was wrong, and the test case would not have to be disabled on 
Windows.

Thanks,
Dscho

[PATCH v2] Support "\" in non-wildcard exclusion entries

[Finn Arne Gangstad]

"\" was treated differently in exclude rules depending on whether a
wildcard match was done. For wildcard rules, "\" was de-escaped in
fnmatch, but this was not done for other rules since they used strcmp
instead.  A file named "#foo" would not be excluded by "\#foo", but would
be excluded by "\#foo*".

We now treat all rules with "\" as wildcard rules.

Another solution could be to de-escape all non-wildcard rules as we
read them, but we would have to do the de-escaping exactly as fnmatch
does it to avoid inconsistencies.

Signed-off-by: Finn Arne Gangstad <finnag@pvv.org>
---
 dir.c                                       |    2 +-
 t/t3003-ls-files-others-escaped-excludes.sh |   37 +++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 1 deletions(-)
 create mode 100755 t/t3003-ls-files-others-escaped-excludes.sh

diff --git a/dir.c b/dir.c
index cfd1ea5..2245749 100644
--- a/dir.c
+++ b/dir.c
@@ -137,7 +137,7 @@ int match_pathspec(const char **pathspec, const char *name, int namelen,
 
 static int no_wildcard(const char *string)
 {
-	return string[strcspn(string, "*?[{")] == '\0';
+	return string[strcspn(string, "*?[{\\")] == '\0';
 }
 
 void add_exclude(const char *string, const char *base,
diff --git a/t/t3003-ls-files-others-escaped-excludes.sh b/t/t3003-ls-files-others-escaped-excludes.sh
new file mode 100755
index 0000000..bce8741
--- /dev/null
+++ b/t/t3003-ls-files-others-escaped-excludes.sh
@@ -0,0 +1,37 @@
+#!/bin/sh
+#
+# Copyright (c) 2009 Finn Arne Gangstad
+#
+
+test_description='git ls-files --others with escaped excludes
+
+This test tests exclusion patterns with \ in them and makes sure they
+are treated correctly and identically both for normal and wildcard rules.
+'
+
+. ./test-lib.sh
+
+touch \#ignore1 &&
+touch \#ignore2 &&
+touch \#hidden &&
+touch keep
+
+echo keep > expect
+
+cat >.gitignore <<EOF
+.gitignore
+expect
+output
+\#ignore1
+\#ignore2*
+\#hid*n
+EOF
+
+test_expect_success \
+    'git ls-files --others with escaped excludes.' \
+    'git ls-files --others \
+       --exclude-per-directory=.gitignore \
+       >output &&
+     test_cmp expect output'
+
+test_done
-- 
1.6.2.rc0.11.g665ed

Re: [PATCH v2] Support "\" in non-wildcard exclusion entries

[Johannes Schindelin]

Hi,

On Tue, 10 Feb 2009, Finn Arne Gangstad wrote:

> "\" was treated differently in exclude rules depending on whether a
> wildcard match was done. For wildcard rules, "\" was de-escaped in
> fnmatch, but this was not done for other rules since they used strcmp
> instead.  A file named "#foo" would not be excluded by "\#foo", but would
> be excluded by "\#foo*".
> 
> We now treat all rules with "\" as wildcard rules.
> 
> Another solution could be to de-escape all non-wildcard rules as we
> read them, but we would have to do the de-escaping exactly as fnmatch
> does it to avoid inconsistencies.
> 
> Signed-off-by: Finn Arne Gangstad <finnag@pvv.org>
> ---
>  dir.c                                       |    2 +-
>  t/t3003-ls-files-others-escaped-excludes.sh |   37 +++++++++++++++++++++++++++

Thanks, much appreciated!

> diff --git a/t/t3003-ls-files-others-escaped-excludes.sh b/t/t3003-ls-files-others-escaped-excludes.sh
> new file mode 100755
> index 0000000..bce8741
> --- /dev/null
> +++ b/t/t3003-ls-files-others-escaped-excludes.sh
> @@ -0,0 +1,37 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2009 Finn Arne Gangstad
> +#
> +
> +test_description='git ls-files --others with escaped excludes
> +
> +This test tests exclusion patterns with \ in them and makes sure they
> +are treated correctly and identically both for normal and wildcard rules.
> +'
> +
> +. ./test-lib.sh
> +
> +touch \#ignore1 &&

In other tests, we avoid 'touch' (IIRC it is not available everywhere or 
some such), and we write ': > \#ignore1' instead.

BTW we do not need the # in the name, it could be any letter, right?  
(Just for my understanding, not as a request to change it.)

> +touch \#ignore2 &&
> +touch \#hidden &&
> +touch keep
> +
> +echo keep > expect
> +
> +cat >.gitignore <<EOF

You probably want to use \EOF here.

> +.gitignore
> +expect
> +output
> +\#ignore1
> +\#ignore2*
> +\#hid*n
> +EOF
> +
> +test_expect_success \
> +    'git ls-files --others with escaped excludes.' \
> +    'git ls-files --others \
> +       --exclude-per-directory=.gitignore \
> +       >output &&
> +     test_cmp expect output'
> +
> +test_done

Thanks!
Dscho

Re: [PATCH v2] Support "\" in non-wildcard exclusion entries

[Finn Arne Gangstad]

On Tue, Feb 10, 2009 at 03:27:43PM +0100, Johannes Schindelin wrote:

> > +
> > +. ./test-lib.sh
> > +
> > +touch \#ignore1 &&
> 
> In other tests, we avoid 'touch' (IIRC it is not available everywhere or 
> some such), and we write ': > \#ignore1' instead.

Will fix it up after seeing if there are some other comments.

> 
> BTW we do not need the # in the name, it could be any letter, right?  
> (Just for my understanding, not as a request to change it.)

So far I have only had to use \ in exclusion rules starting with #,
since they would otherwise be interpreted as comments, but it could be
any character that \ would not change the meaning of. I am not sure
what fnmatch would do with \t, \r and \n for example.

> > +cat >.gitignore <<EOF
> 
> You probably want to use \EOF here.

I am curious, does it matter? Most of the tests use EOF and not \EOF.

- Finn Arne

Re: [PATCH v2] Support "\" in non-wildcard exclusion entries

[Junio C Hamano]

Finn Arne Gangstad <finnag@pvv.org> writes:

>> > +cat >.gitignore <<EOF
>> 
>> You probably want to use \EOF here.
>
> I am curious, does it matter? Most of the tests use EOF and not \EOF.

If you want the same shell variable expansion and quoting rules as you get
inside double-quote pair, you would say <<EOF without any quotes.  If you
quote the EOF, no such substitutions happen.

In this particular case, you want what you typed there literally in the
file, so <<\EOF would be more correct, even though \# expands to \#
itself.

IOW, your current list of patterns does not happen to have anything like
$var nor \\ that would make a difference, but to protect future breakages
by people adding more patterns there, it is better to say <<\EOF when you
know you are not asking for any funny expansion to be explicit.

Re: [PATCH v2] Support "\" in non-wildcard exclusion entries

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Finn Arne Gangstad <finnag@pvv.org> writes:
>
>>> > +cat >.gitignore <<EOF
>>> 
>>> You probably want to use \EOF here.
>>
>> I am curious, does it matter? Most of the tests use EOF and not \EOF.
>
> If you want the same shell variable expansion and quoting rules as you get
> inside double-quote pair, you would say <<EOF without any quotes.  If you
> quote the EOF, no such substitutions happen.
>
> In this particular case, you want what you typed there literally in the
> file, so <<\EOF would be more correct, even though \# expands to \#
> itself.
>
> IOW, your current list of patterns does not happen to have anything like
> $var nor \\ that would make a difference, but to protect future breakages
> by people adding more patterns there, it is better to say <<\EOF when you
> know you are not asking for any funny expansion to be explicit.

Oh, by the way, do we really want to add a new test script?  I am
wondering why the test is not an update to an existing test for the
exclusion feature, such as t/t3001-ls-files-others-exclude.sh

Re: [PATCH v2] Support "\" in non-wildcard exclusion entries

[Finn Arne Gangstad]

On Tue, Feb 10, 2009 at 08:41:12AM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> Oh, by the way, do we really want to add a new test script?  I am
> wondering why the test is not an update to an existing test for the
> exclusion feature, such as t/t3001-ls-files-others-exclude.sh

I really did not want to add a new test script, but extending 3001
with this test seemed to make the test slightly more opaque. I can try
adding it to 3001 and see what it ends up looking like.

- Finn Arne

[PATCH v3] Support "\" in non-wildcard exclusion entries

[Finn Arne Gangstad]

On Tue, Feb 10, 2009 at 08:41:12AM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> Oh, by the way, do we really want to add a new test script?  I am
> wondering why the test is not an update to an existing test for the
> exclusion feature, such as t/t3001-ls-files-others-exclude.sh

Ok, here is the final version with your suggested test-modification,
which seems to to the trick!

- Finn Arne

--8<--
Support "\" in non-wildcard exclusion entries

"\" was treated differently in exclude rules depending on whether a
wildcard match was done. For wildcard rules, "\" was de-escaped in
fnmatch, but this was not done for other rules since they used strcmp
instead.  A file named "#foo" would not be excluded by "\#foo", but would
be excluded by "\#foo*".

We now treat all rules with "\" as wildcard rules.

Another solution could be to de-escape all non-wildcard rules as we
read them, but we would have to do the de-escaping exactly as fnmatch
does it to avoid inconsistencies.

Signed-off-by: Finn Arne Gangstad <finnag@pvv.org>
---
 dir.c                              |    2 +-
 t/t3001-ls-files-others-exclude.sh |    7 +++++++
 2 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/dir.c b/dir.c
index cfd1ea5..2245749 100644
--- a/dir.c
+++ b/dir.c
@@ -137,7 +137,7 @@ int match_pathspec(const char **pathspec, const char *name, int namelen,
 
 static int no_wildcard(const char *string)
 {
-	return string[strcspn(string, "*?[{")] == '\0';
+	return string[strcspn(string, "*?[{\\")] == '\0';
 }
 
 void add_exclude(const char *string, const char *base,
diff --git a/t/t3001-ls-files-others-exclude.sh b/t/t3001-ls-files-others-exclude.sh
index 85aef12..9be9557 100755
--- a/t/t3001-ls-files-others-exclude.sh
+++ b/t/t3001-ls-files-others-exclude.sh
@@ -19,6 +19,9 @@ do
     >$dir/a.$i
   done
 done
+>"#ignore1"
+>"#ignore2"
+>"#hidden"
 
 cat >expect <<EOF
 a.2
@@ -42,6 +45,9 @@ three/a.8
 EOF
 
 echo '.gitignore
+\#ignore1
+\#ignore2*
+\#hid*n
 output
 expect
 .gitignore
@@ -82,6 +88,7 @@ test_expect_success \
 cat > excludes-file << EOF
 *.[1-8]
 e*
+\#*
 EOF
 
 git config core.excludesFile excludes-file
-- 
1.6.2.rc0.11.g665ed

Re: [PATCH v3] Support "\" in non-wildcard exclusion entries

[Johannes Schindelin]

Hi,

On Thu, 12 Feb 2009, Finn Arne Gangstad wrote:

> @@ -82,6 +88,7 @@ test_expect_success \
>  cat > excludes-file << EOF
>  *.[1-8]
>  e*
> +\#*
>  EOF

You addressed all comments, except the \EOF comment, I guess.

Thanks,
Dscho

Re: [PATCH v3] Support "\" in non-wildcard exclusion entries

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi,
>
> On Thu, 12 Feb 2009, Finn Arne Gangstad wrote:
>
>> @@ -82,6 +88,7 @@ test_expect_success \
>>  cat > excludes-file << EOF
>>  *.[1-8]
>>  e*
>> +\#*
>>  EOF
>
> You addressed all comments, except the \EOF comment, I guess.

Thanks; I'll amend the one queued in 'pu' for the last few days, which
matches this round.