threads / patch / 17695

patchSupport \ in non-wildcard .gitignore entries

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

## tl;dr

13 messages between Feb 10, 2009 and Feb 12, 2009. Diffs are folded; open one to read it.

replies: 12people: 3as markdown or json

Finn Arne Gangstad· Feb 10, 2009, 12:11 UTC · lore

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(-)
Show changes to dir.c +1 −1
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
Johannes Schindelin· Feb 10, 2009, 12:56 UTC · re: Finn Arne Gangstad · lore

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

Hi,
On Tue, 10 Feb 2009, Finn Arne Gangstad wrote:
Show 10 quoted lines
> 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

Finn Arne Gangstad· Feb 10, 2009, 12:58 UTC · re: Johannes Schindelin · lore

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

On Tue, Feb 10, 2009 at 01:56:36PM +0100, Johannes Schindelin wrote:
Show 19 quoted lines
> 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
Johannes Schindelin· Feb 10, 2009, 13:02 UTC · re: Finn Arne Gangstad · lore

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

Hi,
On Tue, 10 Feb 2009, Finn Arne Gangstad wrote:
Show 23 quoted lines
> 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

Finn Arne Gangstad· Feb 10, 2009, 14:20 UTC · re: Johannes Schindelin · lore

[PATCH v2] 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/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
Show changes to 2 files +38 −1

dir.c, 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
Johannes Schindelin· Feb 10, 2009, 14:27 UTC · re: Finn Arne Gangstad · lore

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

Hi,
On Tue, 10 Feb 2009, Finn Arne Gangstad wrote:
Show 16 quoted lines
> "\" 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!
Show 20 quoted lines
> 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.)

Show 7 quoted lines
> +touch \#ignore2 &&
> +touch \#hidden &&
> +touch keep
> +
> +echo keep > expect
> +
> +cat >.gitignore <<EOF
You probably want to use \EOF here.
Show 16 quoted lines
> +.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

Finn Arne Gangstad· Feb 10, 2009, 14:37 UTC · re: Johannes Schindelin · lore

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

On Tue, Feb 10, 2009 at 03:27:43PM +0100, Johannes Schindelin wrote:
Show 7 quoted lines
> > +
> > +. ./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
Junio C Hamano· Feb 10, 2009, 15:24 UTC · re: Finn Arne Gangstad · lore

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

Finn Arne Gangstad <finnag@pvv.org> writes:
Show 5 quoted lines
>> > +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.

Junio C Hamano· Feb 10, 2009, 16:41 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 20 quoted lines
> 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

Finn Arne Gangstad· Feb 10, 2009, 17:23 UTC · re: Junio C Hamano · lore

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

On Tue, Feb 10, 2009 at 08:41:12AM -0800, Junio C Hamano wrote:
Show 5 quoted lines
> 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
Finn Arne Gangstad· Feb 12, 2009, 09:32 UTC · re: Junio C Hamano · lore

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

On Tue, Feb 10, 2009 at 08:41:12AM -0800, Junio C Hamano wrote:
Show 5 quoted lines
> 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(-)
Show changes to 2 files +8 −1

dir.c, t/t3001-ls-files-others-exclude.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/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
Johannes Schindelin· Feb 12, 2009, 10:44 UTC · re: Finn Arne Gangstad · lore

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

Hi,
On Thu, 12 Feb 2009, Finn Arne Gangstad wrote:
Show 6 quoted lines
> @@ -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

Junio C Hamano· Feb 12, 2009, 21:03 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 12 quoted lines
> 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.

← back to recent threads