gitignore documentation inconsistent with actual behaviour

git itself happily accepts these patterns, however, apparently treating the ** like a single * without fnmatch(3)'s FNM_PATHNAME flag set (in other words, it matches / as a regular character, thus crossing directory boundaries).

Hello,

I'm a contributor to ripgrep, which is a grep-like tool that supports using gitignore files to control which files are searched in a repo (or any other directory tree). ripgrep's support for the patterns in these files is based on git's official documentation, as seen here:

https://git-scm.com/docs/gitignore

One of the most common reports on the ripgrep bug tracker is that it does not allow patterns like the following real-world examples, where a ** is used along with other text within the same path component:

**/**$$*.java
**.orig
**local.properties
!**.sha1

The reason it doesn't allow them is that the gitignore documentation explicitly states that they're invalid:

A leading "**" followed by a slash means match in all directories...

A trailing "/**" matches everything inside...

A slash followed by two consecutive asterisks then a slash matches zero or more directories...

Other consecutive asterisks are considered invalid.

git itself happily accepts these patterns, however, apparently treating the ** like a single * without fnmatch(3)'s FNM_PATHNAME flag set (in other words, it matches / as a regular character, thus crossing directory boundaries).

ripgrep's developer is loathe to reverse-engineer this undocumented behaviour, and so the reports keep coming, both for ripgrep itself and for down-stream consumers of it and its ignore crate (including most notably Microsoft's VS Code editor).

My request: Assuming that git's actual handling of these patterns is intended, would it be possible to make it 'official' and explicitly add it to the documentation?

References (the first one is the main bug):

https://github.com/BurntSushi/ripgrep/issues/373
https://github.com/BurntSushi/ripgrep/issues/507
https://github.com/BurntSushi/ripgrep/issues/859
https://github.com/BurntSushi/ripgrep/issues/945
https://github.com/BurntSushi/ripgrep/issues/1080
https://github.com/BurntSushi/ripgrep/issues/1082
https://github.com/Microsoft/vscode/issues/24050

Our matching function comes from rsync originally, whose manpage says:

use ’**’ to match anything, including slashes.

I believe this is accurate as far as the implementation goes. You can also see the rather exhaustive tests here:
https://github.com/git/git/blob/master/t/t3070-wildmatch.sh

The reason I'm on this tangent is to ask whether if such a thing existed, if it's something you can see something like ripgrep using. I.e. ask git given its .gitignore and .gitattributes what thing matches one of its pathspecs instead of carrying your own bug-compatible implementation.

Quoted 21 lines · dana

Hello,

I'm a contributor to ripgrep, which is a grep-like tool that supports using gitignore files to control which files are searched in a repo (or any other directory tree). ripgrep's support for the patterns in these files is based on git's official documentation, as seen here:

https://git-scm.com/docs/gitignore

One of the most common reports on the ripgrep bug tracker is that it does not allow patterns like the following real-world examples, where a ** is used along with other text within the same path component:

**/**$$*.java
**.orig
**local.properties
!**.sha1

The reason it doesn't allow them is that the gitignore documentation explicitly states that they're invalid:

Quoted 8 lines

A leading "**" followed by a slash means match in all directories...

A trailing "/**" matches everything inside...

A slash followed by two consecutive asterisks then a slash matches zero or more directories...

Other consecutive asterisks are considered invalid.

Quoted 4 lines

git itself happily accepts these patterns, however, apparently treating the ** like a single * without fnmatch(3)'s FNM_PATHNAME flag set (in other words, it matches / as a regular character, thus crossing directory boundaries).

Quoted 8 lines

ripgrep's developer is loathe to reverse-engineer this undocumented behaviour, and so the reports keep coming, both for ripgrep itself and for down-stream consumers of it and its ignore crate (including most notably Microsoft's VS Code editor).

My request: Assuming that git's actual handling of these patterns is intended, would it be possible to make it 'official' and explicitly add it to the documentation?

Yeah those docs seem wrong. In general the docs for the matching function are quite bad. I have on my TODO list to factor this out into some gitwildmatch manpage, but right now the bit in gitignore is all we have.

Our matching function comes from rsync originally, whose manpage says:

use ’**’ to match anything, including slashes.

I believe this is accurate as far as the implementation goes.

Quoted 5 lines

Our matching function comes from rsync originally, whose manpage says:

use ’**’ to match anything, including slashes.

I believe this is accurate as far as the implementation goes.

No. "**" semantics is not the same as from rsync. The three cases "**/", "/**/" and "/**" were requested by Junio if I remember correctly. You can search the mail archive for more information.

Quoted 4 lines · Duy Nguyen

Quoted 4 lines

Our matching function comes from rsync originally, whose manpage says:

use ’**’ to match anything, including slashes.

Quoted 5 lines

Quoted 1 line

I believe this is accurate as far as the implementation goes.

No. "**" semantics is not the same as from rsync. The three cases "**/", "/**/" and "/**" were requested by Junio if I remember correctly. You can search the mail archive for more information.

Perhaps spelling the rules out would be more benefitial for the purpose of this thread? I do not recall what I requested, but let

me throw out my guesses (i.e. what I would have wished if I were making a request to implement something) to keep the thread alive, you can correct me, and people can take it from there to update the docs ;-)

A double-asterisk, both of whose ends are adjacent to a directory boundary (i.e. the beginning of the pattern, the end of the pattern or a slash) macthes 0 or more levels of directories. e.g. **/a/b would match a/b, x/a/b, x/y/a/b, but not z-a/b. a/**/b would match a/b, a/x/b, but not a/z-b or a-z-b.

What a double-asterisk that does not sit on a directory boundary, e.g. "a**b", "a**/b", "a/**b", or "**a/b", matches, as far as I am concerned, is undefined, meaning that (1) I do not care all that much what the code actually do when a pattern like that is given as long as it does not segfault, and (2) I do not think I would mind changing the behaviour as a "bugfix", if their current behaviour does not make sense and we can come up with a saner alternative.

Hello,

I'm a contributor to ripgrep, which is a grep-like tool that supports using gitignore files to control which files are searched in a repo (or any other directory tree). ripgrep's support for the patterns in these files is based on git's official documentation, as seen here:

https://git-scm.com/docs/gitignore

One of the most common reports on the ripgrep bug tracker is that it does not allow patterns like the following real-world examples, where a ** is used along with other text within the same path component:

**/**$$*.java
**.orig
**local.properties
!**.sha1

The reason it doesn't allow them is that the gitignore documentation explicitly states that they're invalid:

...

Which way should we go? I'm leaning towards the second one...

Quoted 1 line · Duy Nguyen

Which way should we go? I'm leaning towards the second one...

Not sure how much my opinion is worth, but the second option does feel more friendly (from a usage perspective) as well as more straight-forward (from a re-implementation perspective).

There's a third option too, though, right? The 'rsync' behaviour mentioned earlier? It wouldn't matter either way in any of the examples i listed, but is there ever a conceivable use case for something like foo**bar, where the ** matches across slashes? (I can't think of any reason i'd need that personally, but then again i don't understand why these people are using ** the way they are in the first place.)

foo**bar would match foobar as well as foo/bar, foo/x/bar and foo/x/y/bar... Its behavior is error prone in my opinion. There's also some concerns in early iterations of this "**" support that we would need to revisit if we want 'rsync' behavior. I'm not very excited about doing that.

In WM_PATHNAME mode (or FNM_PATHNAME), '*' does not match '/' and '**' can but only in three patterns:

- '**/' matches zero or more leading directories
- '/**/' matches zero or more directories in between
- '/**' matches zero or more trailing directories/files

When '**' is present but not in one of these patterns, the current behavior is consider the pattern invalid and stop matching. In other words, 'foo**bar' never matches anything, whatever you throw at it.

This behavior is arguably a bit confusing partly because we can't really tell the user their pattern is invalid so that they can fix it. So instead, tolerate it and make '**' act like two regular '*'s (which is essentially the same as a single asterisk). This behavior seems more predictable.

Quoted 11 lines · Nguyễn Thái Ngọc Duy

In WM_PATHNAME mode (or FNM_PATHNAME), '*' does not match '/' and '**' can but only in three patterns:

- '**/' matches zero or more leading directories
- '/**/' matches zero or more directories in between
- '/**' matches zero or more trailing directories/files

When '**' is present but not in one of these patterns, the current behavior is consider the pattern invalid and stop matching. In other words, 'foo**bar' never matches anything, whatever you throw at it.

Quoted 5 lines

This behavior is arguably a bit confusing partly because we can't really tell the user their pattern is invalid so that they can fix it. So instead, tolerate it and make '**' act like two regular '*'s (which is essentially the same as a single asterisk). This behavior seems more predictable.

Nice analyzes.
I have one question here:
If the user specifies '**' and nothing is found,
would it be better to die() with a useful message
instead of silently correcting it ?

See the the patch below:

wildmatch.c static int dowild(const uchar *p, const uchar *text, unsigned int flags) · line 108

Quoted 2 lines

wildmatch.c +0 -2

-                             } else
-                                     return WM_ABORT_MALFORMED;

Would it be possible to put in the die() here?
As it is outlined so nicely above, a '**' must have either a '/' before, or behind, or both, to make sense.
When there is no '/' then the user specified something wrong. Either a '/' has been forgotten, or the '*' key may be bouncing. I don't think that Git should assume anything here.
(but I didn't follow the previous discussions, so I may have missed some arguments.)

[]

Quoted 5 lines

Nice analyzes.
I have one question here:
If the user specifies '**' and nothing is found,
would it be better to die() with a useful message
instead of silently correcting it ?

Consider the main use case of wildmatch, .gitignore patterns, dying would be really bad because it can affect a lot of commands....

In WM_PATHNAME mode (or FNM_PATHNAME), '*' does not match '/' and '**' can but only in three patterns:

- '**/' matches zero or more leading directories
- '/**/' matches zero or more directories in between
- '/**' matches zero or more trailing directories/files

When '**' is present but not in one of these patterns, the current behavior is consider the pattern invalid and stop matching. In other words, 'foo**bar' never matches anything, whatever you throw at it.

This behavior is arguably a bit confusing partly because we can't really tell the user their pattern is invalid so that they can fix it. So instead, tolerate it and make '**' act like two regular '*'s (which is essentially the same as a single asterisk). This behavior seems more predictable.

Noticed-by dana <[email protected]>

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 Documentation/gitignore.txt | 3 ++-
 t/t3070-wildmatch.sh        | 4 ++--
 wildmatch.c                 | 4 ++--
 wildmatch.h                 | 1 -
 4 files changed, 6 insertions(+), 6 deletions(-)

Documentation/gitignore.txt +2 -1

index d107daaffd..1c94f08ff4 100644
@@ -129,7 +129,8 @@ full pathname may have special meaning:
    matches zero or more directories. For example, "`a/**/b`"
    matches "`a/b`", "`a/x/b`", "`a/x/y/b`" and so on.
 
- - Other consecutive asterisks are considered invalid.
+ - Other consecutive asterisks are considered regular asterisks and
+   will match according to the previous rules.
 
 NOTES
 -----

t/t3070-wildmatch.sh +2 -2

index 46aca0af10..891d4d7cb9 100755
@@ -237,7 +237,7 @@ match 0 0 0 0 foobar 'foo\*bar'
 match 1 1 1 1 'f\oo' 'f\\oo'
 match 1 1 1 1 ball '*[al]?'
 match 0 0 0 0 ten '[ten]'
-match 0 0 1 1 ten '**[!te]'
+match 1 1 1 1 ten '**[!te]'
 match 0 0 0 0 ten '**[!ten]'
 match 1 1 1 1 ten 't[a-g]n'
 match 0 0 0 0 ten 't[!a-g]n'

@@ -253,7 +253,7 @@ match 1 1 1 1 ']' ']'
 # Extended slash-matching features
 match 0 0 1 1 'foo/baz/bar' 'foo*bar'
 match 0 0 1 1 'foo/baz/bar' 'foo**bar'
-match 0 0 1 1 'foobazbar' 'foo**bar'
+match 1 1 1 1 'foobazbar' 'foo**bar'
 match 1 1 1 1 'foo/baz/bar' 'foo/**/bar'
 match 1 1 0 0 'foo/baz/bar' 'foo/**/**/bar'
 match 1 1 1 1 'foo/b/a/z/bar' 'foo/**/bar'

wildmatch.c +2 -2

index d074c1be10..9e9e2a2f95 100644
@@ -104,8 +104,8 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)
 					    dowild(p + 1, text, flags) == WM_MATCH)
 						return WM_MATCH;
 					match_slash = 1;
-				} else
-					return WM_ABORT_MALFORMED;
+				} else /* WM_PATHNAME is set */
+					match_slash = 0;
 			} else
 				/* without WM_PATHNAME, '*' == '**' */
 				match_slash = flags & WM_PATHNAME ? 0 : 1;

wildmatch.h +0 -0

index b8c826aa68..5993696298 100644

This patch looks good to me, but I think it's a bad state of affairs to keep changing these semantics and not having something like a "gitwildmatch" doc were we document this matching syntax.

Do you have any thoughts on how to proceed with getting this documented / into some stable state where we can specify it?