Skip to contentSkip to conversation

Support \ in non-wildcard .gitignore entries

PATCH In review Finn Arne Gangstad 3 participants 13 messages Jun 15, 2016

lore
Share & tools
lore

Finn Arne Gangstad

author
#0 lore raw

PATCH 1/1 Support \ in non-wildcard .gitignore entries

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 <[email protected]>
---
 dir.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
dir.c +1 -1
index cfd1ea5..2245749 100644
@@ -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,
Signature · 1 line
1.6.2.rc0.11.g68cbb.dirty
#1 lore raw

Hi,

Quoted 10 lines · 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.

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

author
#2 lore raw
Quoted 15 lines · Johannes Schindelin

Hi,

Quoted 10 lines · 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.

Quoted 4 lines

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

#3 lore raw

Hi,

Quoted 16 lines · Finn Arne Gangstad
Quoted 15 lines · Johannes Schindelin

Hi,

Quoted 10 lines · 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.

Quoted 7 lines
Quoted 4 lines

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

author
#4 lore raw

PATCH v2 1/1 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 <[email protected]>
---
 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
dir.c +1 -1
index cfd1ea5..2245749 100644
@@ -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,
t/t3003-ls-files-others-escaped-excludes.sh new file +37 -0
new file mode 100755
index 0000000..bce8741
@@ -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
Signature · 1 line
1.6.2.rc0.11.g665ed
#5 lore raw

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

Hi,

Quoted 8 lines · 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.

Quoted 8 lines

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 <[email protected]>
---
 dir.c                                       |    2 +-
 t/t3003-ls-files-others-escaped-excludes.sh |   37 +++++++++++++++++++++++++++

Thanks, much appreciated!

Quoted 10 lines
t/t3003-ls-files-others-escaped-excludes.sh new file +4 -0
new file mode 100755
index 0000000..bce8741
@@ -0,0 +1,37 @@
+#!/bin/sh
+#
+# Copyright (c) 2009 Finn Arne Gangstad
+#

t/t3003-ls-files-others-escaped-excludes.sh line 14

Quoted 10 lines
t/t3003-ls-files-others-escaped-excludes.sh new file +10 -0
+
+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.)

t/t3003-ls-files-others-escaped-excludes.sh line 21

Quoted 7 lines
t/t3003-ls-files-others-escaped-excludes.sh new file +7 -0
+touch \#ignore2 &&
+touch \#hidden &&
+touch keep
+
+echo keep > expect
+
+cat >.gitignore <<EOF

You probably want to use \EOF here.

Quoted 6 lines
t/t3003-ls-files-others-escaped-excludes.sh new file +6 -0
+.gitignore
+expect
+output
+\#ignore1
+\#ignore2*
+\#hid*n

t/t3003-ls-files-others-escaped-excludes.sh line 37

Quoted 10 lines
t/t3003-ls-files-others-escaped-excludes.sh new file +10 -0
+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

author
#6 lore raw

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

Quoted 7 lines · Johannes Schindelin

t/t3003-ls-files-others-escaped-excludes.sh line 14

Quoted 4 lines
t/t3003-ls-files-others-escaped-excludes.sh new file +4 -0
+
+. ./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.

Quoted 3 lines

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.

Quoted 3 lines
Quoted 1 line

+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

#7 lore raw

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

Quoted 5 lines · Finn Arne Gangstad
Quoted 3 lines
Quoted 1 line

+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.

#8 lore raw

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

Quoted 12 lines · Junio C Hamano
Quoted 5 lines · Finn Arne Gangstad
Quoted 3 lines
Quoted 1 line

+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.

Quoted 8 lines

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

author
#9 lore raw

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

Quoted 5 lines · Junio C Hamano

Junio C Hamano <[email protected]> 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

author
#10 lore raw

PATCH v3 1/1 Support "\" in non-wildcard exclusion entries

In reply to Junio C Hamano
Quoted 5 lines · Junio C Hamano

Junio C Hamano <[email protected]> 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 <[email protected]>
---
 dir.c                              |    2 +-
 t/t3001-ls-files-others-exclude.sh |    7 +++++++
 2 files changed, 8 insertions(+), 1 deletions(-)
dir.c +1 -1
index cfd1ea5..2245749 100644
@@ -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,
t/t3001-ls-files-others-exclude.sh +7 -0
index 85aef12..9be9557 100755
@@ -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
Signature · 1 line
1.6.2.rc0.11.g665ed
#11 lore raw

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

Hi,

t/t3001-ls-files-others-exclude.sh test_expect_success \ · line 92

Quoted 6 lines · Finn Arne Gangstad
t/t3001-ls-files-others-exclude.sh +1 -0
@@ -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

#12 lore raw

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

Quoted 4 lines · Johannes Schindelin

Hi,

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

Quoted 8 lines

t/t3001-ls-files-others-exclude.sh test_expect_success \ · line 92

Quoted 6 lines
t/t3001-ls-files-others-exclude.sh +1 -0
@@ -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.