{"thread":{"id":"49539","subject":"[BUG] gitignore documentation inconsistent with actual behaviour","startedAt":"2018-10-11T10:19:09Z","lastAt":"2018-10-29T15:54:23Z","messageCount":17,"participants":["dana","Ævar Arnfjörð Bjarmason","Duy Nguyen","Junio C Hamano","Nguyễn Thái Ngọc Duy","Torsten Bögershausen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"360166","messageId":"C16A9F17-0375-42F9-90A9-A92C9F3D8BBA@dana.is","threadId":"49539","inReplyTo":null,"subject":"[BUG] gitignore documentation inconsistent with actual behaviour","fromName":"dana","fromEmail":"dana@dana.is","sentAt":"2018-10-11T10:19:06Z","receivedAt":"2018-10-11T10:19:09Z","isPatch":false,"sender":{"key":"dana@dana.is","avatar":null},"body":"Hello,\n\nI'm a contributor to ripgrep, which is a grep-like tool that supports using\ngitignore files to control which files are searched in a repo (or any other\ndirectory tree). ripgrep's support for the patterns in these files is based on\ngit's official documentation, as seen here:\n\n https://git-scm.com/docs/gitignore\n\nOne of the most common reports on the ripgrep bug tracker is that it does not\nallow patterns like the following real-world examples, where a ** is used along\nwith other text within the same path component:\n\n **/**$$*.java\n **.orig\n **local.properties\n !**.sha1\n\nThe reason it doesn't allow them is that the gitignore documentation explicitly\nstates that they're invalid:\n\n A leading \"**\" followed by a slash means match in all directories...\n\n A trailing \"/**\" matches everything inside...\n\n A slash followed by two consecutive asterisks then a slash matches zero or\n more directories...\n\n Other consecutive asterisks are considered invalid.\n\ngit itself happily accepts these patterns, however, apparently treating the **\nlike a single * without fnmatch(3)'s FNM_PATHNAME flag set (in other words, it\nmatches / as a regular character, thus crossing directory boundaries).\n\nripgrep's developer is loathe to reverse-engineer this undocumented behaviour,\nand so the reports keep coming, both for ripgrep itself and for down-stream\nconsumers of it and its ignore crate (including most notably Microsoft's VS Code\neditor).\n\nMy request: Assuming that git's actual handling of these patterns is intended,\nwould it be possible to make it 'official' and explicitly add it to the\ndocumentation?\n\nReferences (the first one is the main bug):\n\nhttps://github.com/BurntSushi/ripgrep/issues/373\nhttps://github.com/BurntSushi/ripgrep/issues/507\nhttps://github.com/BurntSushi/ripgrep/issues/859\nhttps://github.com/BurntSushi/ripgrep/issues/945\nhttps://github.com/BurntSushi/ripgrep/issues/1080\nhttps://github.com/BurntSushi/ripgrep/issues/1082\nhttps://github.com/Microsoft/vscode/issues/24050\n\ndana\n\n"},{"id":"360170","messageId":"EB612DF5-EA44-4BB9-95D4-540E3F09FD0F@dana.is","threadId":"49539","inReplyTo":"C16A9F17-0375-42F9-90A9-A92C9F3D8BBA@dana.is","subject":"Re: [BUG] gitignore documentation inconsistent with actual behaviour","fromName":"dana","fromEmail":"dana@dana.is","sentAt":"2018-10-11T10:37:10Z","receivedAt":"2018-10-11T10:37:13Z","isPatch":false,"sender":{"key":"dana@dana.is","avatar":null},"body":"On 11 Oct 2018, at 05:19, dana wrote:\n>git itself happily accepts these patterns, however, apparently treating the **\n>like a single * without fnmatch(3)'s FNM_PATHNAME flag set (in other words, it\n>matches / as a regular character, thus crossing directory boundaries).\n\nSorry for replying to myself so quickly, but i think this bit is wrong. It seems\nlike it actually just treats ** in this sort of pattern like a regular * — but\ni haven't studied the source very closely, so maybe that's not the full story\neither, i'm not sure.\n\ndana\n\n"},{"id":"360172","messageId":"87bm80eo6h.fsf@evledraar.gmail.com","threadId":"49539","inReplyTo":"C16A9F17-0375-42F9-90A9-A92C9F3D8BBA@dana.is","subject":"Re: [BUG] gitignore documentation inconsistent with actual behaviour","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-10-11T11:08:06Z","receivedAt":"2018-10-11T11:08:12Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Oct 11 2018, dana wrote:\n\n> Hello,\n>\n> I'm a contributor to ripgrep, which is a grep-like tool that supports using\n> gitignore files to control which files are searched in a repo (or any other\n> directory tree). ripgrep's support for the patterns in these files is based on\n> git's official documentation, as seen here:\n>\n> https://git-scm.com/docs/gitignore\n>\n> One of the most common reports on the ripgrep bug tracker is that it does not\n> allow patterns like the following real-world examples, where a ** is used along\n> with other text within the same path component:\n>\n> **/**$$*.java\n> **.orig\n> **local.properties\n> !**.sha1\n>\n> The reason it doesn't allow them is that the gitignore documentation explicitly\n> states that they're invalid:\n>\n> A leading \"**\" followed by a slash means match in all directories...\n>\n> A trailing \"/**\" matches everything inside...\n>\n> A slash followed by two consecutive asterisks then a slash matches zero or\n> more directories...\n>\n> Other consecutive asterisks are considered invalid.\n>\n> git itself happily accepts these patterns, however, apparently treating the **\n> like a single * without fnmatch(3)'s FNM_PATHNAME flag set (in other words, it\n> matches / as a regular character, thus crossing directory boundaries).\n>\n> ripgrep's developer is loathe to reverse-engineer this undocumented behaviour,\n> and so the reports keep coming, both for ripgrep itself and for down-stream\n> consumers of it and its ignore crate (including most notably Microsoft's VS Code\n> editor).\n>\n> My request: Assuming that git's actual handling of these patterns is intended,\n> would it be possible to make it 'official' and explicitly add it to the\n> documentation?\n>\n> References (the first one is the main bug):\n>\n> https://github.com/BurntSushi/ripgrep/issues/373\n> https://github.com/BurntSushi/ripgrep/issues/507\n> https://github.com/BurntSushi/ripgrep/issues/859\n> https://github.com/BurntSushi/ripgrep/issues/945\n> https://github.com/BurntSushi/ripgrep/issues/1080\n> https://github.com/BurntSushi/ripgrep/issues/1082\n> https://github.com/Microsoft/vscode/issues/24050\n\nYeah those docs seem wrong. In general the docs for the matching\nfunction are quite bad. I have on my TODO list to factor this out into\nsome gitwildmatch manpage, but right now the bit in gitignore is all we\nhave.\n\nOur matching function comes from rsync originally, whose manpage says:\n\n use ’**’ to match anything, including slashes.\n\nI believe this is accurate as far as the implementation goes. You can\nalso see the rather exhaustive tests here:\nhttps://github.com/git/git/blob/master/t/t3070-wildmatch.sh\n\nNote the different behavior with e.g. --glob-pathspecs v.s. the\ndefault. There's also stuff like:\n\n $ grep diff=perl .gitattributes\n *.perl eol=lf diff=perl\n *.pl eof=lf diff=perl\n *.pm eol=lf diff=perl\n $ git ls-files \":(attr:diff=perl)\" | wc -l\n 65\n\nAnd then the exclude syntax. This is not in .gitignore:\n\n $ git ls-files \":(exclude)*.pm\" \":(attr:diff=perl)\" | wc -l\n 41\n $ git ls-files \":^*.pm\" \":(attr:diff=perl)\" | wc -l\n 41\n\nI.e. we have wildmatch() on one hand and then the pathspec matching.\n\nFor an unrelated thing I have been thinking of adding a new plumbing\ncommand who'd get input like this on stdin:\n\n 1 text t/t0202/test.pl\\0\\0\n 2 text perl/Git.pm\\0\\0\n 3 text *.pm\\0\\0\n 4 text :^*.pm\"\\0:(attr:diff=perl)\\0\\0\n 5 match glob,icase\\04\\03\\0\\0\n 6 match icase\\04\\02\\0\\0\n 7 match \\04\\01\\0\\0\n\nWhich would return (in any order):\n\n 1 OK\n 2 OK\n 3 OK\n 4 OK\n 5 NO\n 6 NO\n 7 YES\n\nOr whatever. I.e. something where you can as a batch feed various\nstrings into a program in a batch, and then ask how some of them would\nmatch against each other.\n\nThe reason for this is to extend something like git-multimail[1] with a\nconfig where users can subscribe to changes to paths as declared by git\npathspecs, and be informed about which of the glob rules they defined\nmatched their config.\n\nRight now you'd need to e.g. for a \"git push\" run each match rule for\neach user with such config against each changed path in each commit\nthat's pushed, but plumbing like this would allow for feeding arbitrary\ncombinations of those in and ask what matches against what.\n\nThe reason I'm on this tangent is to ask whether if such a thing\nexisted, if it's something you can see something like ripgrep\nusing. I.e. ask git given its .gitignore and .gitattributes what thing\nmatches one of its pathspecs instead of carrying your own bug-compatible\nimplementation.\n\n1. https://github.com/git-multimail/git-multimail/\n"},{"id":"360397","messageId":"E7F1D716-D8DE-40DA-AE3E-740BD28AB677@dana.is","threadId":"49539","inReplyTo":"87bm80eo6h.fsf@evledraar.gmail.com","subject":"Re: [BUG] gitignore documentation inconsistent with actual behaviour","fromName":"dana","fromEmail":"dana@dana.is","sentAt":"2018-10-14T02:14:19Z","receivedAt":"2018-10-14T02:40:56Z","isPatch":false,"sender":{"key":"dana@dana.is","avatar":null},"body":"On 11 Oct 2018, at 06:08, Ævar Arnfjörð Bjarmason wrote:\n>Our matching function comes from rsync originally, whose manpage says:\n>\n> use ’**’ to match anything, including slashes.\n>\n>I believe this is accurate as far as the implementation goes. You can\n>also see the rather exhaustive tests here:\n>https://github.com/git/git/blob/master/t/t3070-wildmatch.sh\n\nThanks. The tests are a little hard to follow at first glance, so i guess i'd\nhave to study them more to see what the semantics actually are. When i tested it\nbriefly it didn't seem like it was behaving as you described, but maybe i wasn't\npaying close enough attention. In any case, i'm sure these will be useful.\n\nOn 11 Oct 2018, at 06:08, Ævar Arnfjörð Bjarmason wrote:\n>The reason I'm on this tangent is to ask whether if such a thing\n>existed, if it's something you can see something like ripgrep\n>using. I.e. ask git given its .gitignore and .gitattributes what thing\n>matches one of its pathspecs instead of carrying your own bug-compatible\n>implementation.\n\nMy impression is that it probably isn't practical to use something like that as\nripgrep's main pattern-matching facility — for one thing there are performance\nconcerns, and for another it makes it dependent on git for some of its core\nfunctionality (and it's not a git-specific tool). But i was going to say that\nit'd be useful for testing, and when i e-mailed Andrew (the main ripgrep dev)\nabout this he said the same. I've CCed him on this message in case he has\nanything to add.\n\ndana\n\n"},{"id":"360404","messageId":"CACsJy8DVSu2Sh34UG4P9aqqEMsY5zOGc61tEizo=0sG4BoSgkw@mail.gmail.com","threadId":"49539","inReplyTo":"87bm80eo6h.fsf@evledraar.gmail.com","subject":"Re: [BUG] gitignore documentation inconsistent with actual behaviour","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-10-14T12:15:31Z","receivedAt":"2018-10-14T12:20:14Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Oct 11, 2018 at 2:41 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n>\n> On Thu, Oct 11 2018, dana wrote:\n>\n> > Hello,\n> >\n> > I'm a contributor to ripgrep, which is a grep-like tool that supports using\n> > gitignore files to control which files are searched in a repo (or any other\n> > directory tree). ripgrep's support for the patterns in these files is based on\n> > git's official documentation, as seen here:\n> >\n> > https://git-scm.com/docs/gitignore\n> >\n> > One of the most common reports on the ripgrep bug tracker is that it does not\n> > allow patterns like the following real-world examples, where a ** is used along\n> > with other text within the same path component:\n> >\n> > **/**$$*.java\n> > **.orig\n> > **local.properties\n> > !**.sha1\n> >\n> > The reason it doesn't allow them is that the gitignore documentation explicitly\n> > states that they're invalid:\n> >\n> > A leading \"**\" followed by a slash means match in all directories...\n> >\n> > A trailing \"/**\" matches everything inside...\n> >\n> > A slash followed by two consecutive asterisks then a slash matches zero or\n> > more directories...\n> >\n> > Other consecutive asterisks are considered invalid.\n\nPerhaps \"undefined\" is a better word than \"invalid\".\n\n> > git itself happily accepts these patterns, however, apparently treating the **\n> > like a single * without fnmatch(3)'s FNM_PATHNAME flag set (in other words, it\n> > matches / as a regular character, thus crossing directory boundaries).\n> >\n> > ripgrep's developer is loathe to reverse-engineer this undocumented behaviour,\n> > and so the reports keep coming, both for ripgrep itself and for down-stream\n> > consumers of it and its ignore crate (including most notably Microsoft's VS Code\n> > editor).\n> >\n> > My request: Assuming that git's actual handling of these patterns is intended,\n> > would it be possible to make it 'official' and explicitly add it to the\n> > documentation?\n\nYou mean \"**\" in the fourth case is interpreted as \"*\"? Yes I guess we\ncould rephrase it as \"Other consecutive asterisks are considered\nnormal wildcard asterisks\"\n\n> Yeah those docs seem wrong. In general the docs for the matching\n> function are quite bad. I have on my TODO list to factor this out into\n> some gitwildmatch manpage, but right now the bit in gitignore is all we\n> have.\n>\n> Our matching function comes from rsync originally, whose manpage says:\n>\n> use ’**’ to match anything, including slashes.\n>\n> I believe this is accurate as far as the implementation goes.\n\nNo. \"**\" semantics is not the same as from rsync. The three cases\n\"**/\", \"/**/\" and \"/**\" were requested by Junio if I remember\ncorrectly. You can search the mail archive for more information.\n-- \nDuy\n"},{"id":"360434","messageId":"xmqqk1mk87d0.fsf@gitster-ct.c.googlers.com","threadId":"49539","inReplyTo":"CACsJy8DVSu2Sh34UG4P9aqqEMsY5zOGc61tEizo=0sG4BoSgkw@mail.gmail.com","subject":"Re: [BUG] gitignore documentation inconsistent with actual behaviour","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-14T22:56:59Z","receivedAt":"2018-10-14T23:04:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n>> Our matching function comes from rsync originally, whose manpage says:\n>>\n>> use ’**’ to match anything, including slashes.\n>>\n>> I believe this is accurate as far as the implementation goes.\n>\n> No. \"**\" semantics is not the same as from rsync. The three cases\n> \"**/\", \"/**/\" and \"/**\" were requested by Junio if I remember\n> correctly. You can search the mail archive for more information.\n\nPerhaps spelling the rules out would be more benefitial for the\npurpose of this thread? I do not recall what I requested, but let\nme throw out my guesses (i.e. what I would have wished if I were\nmaking a request to implement something) to keep the thread alive,\nyou can correct me, and people can take it from there to update the\ndocs ;-)\n\n A double-asterisk, both of whose ends are adjacent to a\n directory boundary (i.e. the beginning of the pattern, the end\n of the pattern or a slash) macthes 0 or more levels of\n directories. e.g. **/a/b would match a/b, x/a/b, x/y/a/b, but\n not z-a/b. a/**/b would match a/b, a/x/b, but not a/z-b or\n a-z-b.\n\nWhat a double-asterisk that does not sit on a directory boundary,\ne.g. \"a**b\", \"a**/b\", \"a/**b\", or \"**a/b\", matches, as far as I am\nconcerned, is undefined, meaning that (1) I do not care all that\nmuch what the code actually do when a pattern like that is given as\nlong as it does not segfault, and (2) I do not think I would mind\nchanging the behaviour as a \"bugfix\", if their current behaviour\ndoes not make sense and we can come up with a saner alternative.\n\nBut the documentation probably should describe what these currently\nmatch as the starting point.\n\nThanks.\n"},{"id":"360536","messageId":"CACsJy8CeD_PtmYoQL-qccHoTg-GaGk0ATMoBUqyi=ER7By+2Rw@mail.gmail.com","threadId":"49539","inReplyTo":"xmqqk1mk87d0.fsf@gitster-ct.c.googlers.com","subject":"Re: [BUG] gitignore documentation inconsistent with actual behaviour","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-10-15T15:27:15Z","receivedAt":"2018-10-15T15:27:44Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Oct 15, 2018 at 12:57 AM Junio C Hamano wrote:\n>\n> Duy Nguyen writes:\n>\n> >> Our matching function comes from rsync originally, whose manpage says:\n> >>\n> >> use ’**’ to match anything, including slashes.\n> >>\n> >> I believe this is accurate as far as the implementation goes.\n> >\n> > No. \"**\" semantics is not the same as from rsync. The three cases\n> > \"**/\", \"/**/\" and \"/**\" were requested by Junio if I remember\n> > correctly. You can search the mail archive for more information.\n>\n> Perhaps spelling the rules out would be more benefitial for the\n> purpose of this thread? I do not recall what I requested, but let\n\nOK I gave you too much credit. You pointed out semantics problem with\nrsync \"**\" [1] and gently pushed me to implement a safer subset ;-)\n\n[1] https://public-inbox.org/git/7vbogj5sji.fsf@alter.siamese.dyndns.org/\n\n> me throw out my guesses (i.e. what I would have wished if I were\n> making a request to implement something) to keep the thread alive,\n> you can correct me, and people can take it from there to update the\n> docs ;-)\n>\n> A double-asterisk, both of whose ends are adjacent to a\n> directory boundary (i.e. the beginning of the pattern, the end\n> of the pattern or a slash) macthes 0 or more levels of\n> directories. e.g. **/a/b would match a/b, x/a/b, x/y/a/b, but\n> not z-a/b. a/**/b would match a/b, a/x/b, but not a/z-b or\n> a-z-b.\n>\n> What a double-asterisk that does not sit on a directory boundary,\n> e.g. \"a**b\", \"a**/b\", \"a/**b\", or \"**a/b\", matches, as far as I am\n> concerned, is undefined, meaning that (1) I do not care all that\n> much what the code actually do when a pattern like that is given as\n> long as it does not segfault, and (2) I do not think I would mind\n> changing the behaviour as a \"bugfix\", if their current behaviour\n> does not make sense and we can come up with a saner alternative.\n\nI think the document describes more or less what you wrote above in\nthe indented paragraph (but maybe less clear, patches are of course\nwelcome). It's the last paragraph that is problematic. It right now\nsays \"invalid\" which could be interpreted as \"bad pattern, rejected\"\nbut we in fact accept these \"*\" are regular ones.\n\nThere are not many alternatives we could do though. Erroring out could\nreally flood the stderr because we match these patterns zillions of\ntime and traditionally fnmatch gracefully accepts bad patterns, trying\nto make the best of of them. So keeping undefined \"**\" as two \"*\"\nsounds good enough.\n\nBut of course I'm open for saner alternatives if people find any.\n-- \nDuy\n"},{"id":"360994","messageId":"20181020052624.GA31433@duynguyen.home","threadId":"49539","inReplyTo":"C16A9F17-0375-42F9-90A9-A92C9F3D8BBA@dana.is","subject":"Re: [BUG] gitignore documentation inconsistent with actual behaviour","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-10-20T05:26:24Z","receivedAt":"2018-10-20T05:26:34Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Oct 11, 2018 at 05:19:06AM -0500, dana wrote:\n> Hello,\n> \n> I'm a contributor to ripgrep, which is a grep-like tool that supports using\n> gitignore files to control which files are searched in a repo (or any other\n> directory tree). ripgrep's support for the patterns in these files is based on\n> git's official documentation, as seen here:\n> \n> https://git-scm.com/docs/gitignore\n> \n> One of the most common reports on the ripgrep bug tracker is that it does not\n> allow patterns like the following real-world examples, where a ** is used along\n> with other text within the same path component:\n> \n> **/**$$*.java\n> **.orig\n> **local.properties\n> !**.sha1\n> \n> The reason it doesn't allow them is that the gitignore documentation explicitly\n> states that they're invalid:\n>\n> ...\n\nI've checked the code and run some tests. There is a twist here. \"**\"\nis only special when matched in \"pathname\" mode. That is when the\npattern contains at least one slash. In your patterns above, that only\napplies to the first pattern.\n\nWhen '**' is special, if it's neither '**/', '/**/' or '/**', it _is_\nconsidered invalid (i.e. bad pattern) and the pattern will not match\nanything.\n\nThe confusion comes from when '**' is not special for the remaining\nthree patterns, it's considered as regular '*' and still matches\nstuff.\n\nSo, I think we have two options. The document could be clarified with\nsomething like this\n\n-- 8< --\ndiff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt\nindex d107daaffd..500cd43939 100644\n--- a/Documentation/gitignore.txt\n+++ b/Documentation/gitignore.txt\n@@ -100,7 +100,8 @@ PATTERN FORMAT\n a shell glob pattern and checks for a match against the\n pathname relative to the location of the `.gitignore` file\n (relative to the toplevel of the work tree if not from a\n- `.gitignore` file).\n+ `.gitignore` file). Note that the \"two consecutive asterisks\" rule\n+ below does not apply.\n \n - Otherwise, Git treats the pattern as a shell glob: \"`*`\" matches\n anything except \"`/`\", \"`?`\" matches any one character except \"`/`\"\n@@ -129,7 +130,8 @@ full pathname may have special meaning:\n matches zero or more directories. For example, \"`a/**/b`\"\n matches \"`a/b`\", \"`a/x/b`\", \"`a/x/y/b`\" and so on.\n \n- - Other consecutive asterisks are considered invalid.\n+ - Other consecutive asterisks are considered invalid and the pattern\n+ is ignored.\n \n NOTES\n -----\n-- 8< --\n\nOr we could make the behavior consistent. If '**' is invalid, just\nconsider it two separate regular '*'. Then all four of your patterns\nwill behave the same way. The change for that is quite simple\n\n-- 8< --\ndiff --git a/wildmatch.c b/wildmatch.c\nindex d074c1be10..64087bf02c 100644\n--- a/wildmatch.c\n+++ b/wildmatch.c\n@@ -104,8 +104,10 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n \t\t\t\t\t dowild(p + 1, text, flags) == WM_MATCH)\n \t\t\t\t\t\treturn WM_MATCH;\n \t\t\t\t\tmatch_slash = 1;\n-\t\t\t\t} else\n-\t\t\t\t\treturn WM_ABORT_MALFORMED;\n+\t\t\t\t} else {\n+\t\t\t\t\t/* without WM_PATHNAME, '*' == '**' */\n+\t\t\t\t\tmatch_slash = flags & WM_PATHNAME ? 0 : 1;\n+\t\t\t\t}\n \t\t\t} else\n \t\t\t\t/* without WM_PATHNAME, '*' == '**' */\n \t\t\t\tmatch_slash = flags & WM_PATHNAME ? 0 : 1;\n-- 8< --\n\nWhich way should we go? I'm leaning towards the second one...\n--\nDuy\n"},{"id":"360995","messageId":"AFFFEB92-0CFE-45BA-8BE3-105E8963A121@dana.is","threadId":"49539","inReplyTo":"20181020052624.GA31433@duynguyen.home","subject":"Re: [BUG] gitignore documentation inconsistent with actual behaviour","fromName":"dana","fromEmail":"dana@dana.is","sentAt":"2018-10-20T05:53:16Z","receivedAt":"2018-10-20T05:53:20Z","isPatch":false,"sender":{"key":"dana@dana.is","avatar":null},"body":"On 20 Oct 2018, at 00:26, Duy Nguyen wrote:\n>Which way should we go? I'm leaning towards the second one...\n\nNot sure how much my opinion is worth, but the second option does feel more\nfriendly (from a usage perspective) as well as more straight-forward (from a\nre-implementation perspective).\n\nThere's a third option too, though, right? The 'rsync' behaviour mentioned\nearlier? It wouldn't matter either way in any of the examples i listed, but is\nthere ever a conceivable use case for something like `foo**bar`, where the `**`\nmatches across slashes? (I can't think of any reason i'd need that personally,\nbut then again i don't understand why these people are using `**` the way they\nare in the first place.)\n\ndana\n\n"},{"id":"360996","messageId":"CACsJy8CMSQZ7guGAyDqU_xdJrt7BiEDwdtePdfmgagnvSb2p=Q@mail.gmail.com","threadId":"49539","inReplyTo":"AFFFEB92-0CFE-45BA-8BE3-105E8963A121@dana.is","subject":"Re: [BUG] gitignore documentation inconsistent with actual behaviour","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-10-20T06:03:13Z","receivedAt":"2018-10-20T06:06:44Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Oct 20, 2018 at 7:53 AM dana wrote:\n>\n> On 20 Oct 2018, at 00:26, Duy Nguyen wrote:\n> >Which way should we go? I'm leaning towards the second one...\n>\n> Not sure how much my opinion is worth, but the second option does feel more\n> friendly (from a usage perspective) as well as more straight-forward (from a\n> re-implementation perspective).\n\nYeah. And not having to describe all the corner cases is a plus. Too\nmany corner cases are a sign of bad implementation anyway. I'll wait\nsome more time for the others to speak up before I cook a proper\npatch.\n\n> There's a third option too, though, right? The 'rsync' behaviour mentioned\n> earlier? It wouldn't matter either way in any of the examples i listed, but is\n> there ever a conceivable use case for something like `foo**bar`, where the `**`\n> matches across slashes? (I can't think of any reason i'd need that personally,\n> but then again i don't understand why these people are using `**` the way they\n> are in the first place.)\n\nfoo**bar would match foobar as well as foo/bar, foo/x/bar and\nfoo/x/y/bar... Its behavior is error prone in my opinion. There's also\nsome concerns in early iterations of this \"**\" support that we would\nneed to revisit if we want 'rsync' behavior. I'm not very excited\nabout doing that.\n-- \nDuy\n"},{"id":"360997","messageId":"7C1E9A1B-9FA6-40C6-95E0-6C90271C37C1@dana.is","threadId":"49539","inReplyTo":"CACsJy8CMSQZ7guGAyDqU_xdJrt7BiEDwdtePdfmgagnvSb2p=Q@mail.gmail.com","subject":"Re: [BUG] gitignore documentation inconsistent with actual behaviour","fromName":"dana","fromEmail":"dana@dana.is","sentAt":"2018-10-20T06:26:59Z","receivedAt":"2018-10-20T06:30:56Z","isPatch":false,"sender":{"key":"dana@dana.is","avatar":null},"body":"On 20 Oct 2018, at 01:03, Duy Nguyen wrote:\n>foo**bar would match foobar as well as foo/bar, foo/x/bar and\n>foo/x/y/bar... Its behavior is error prone in my opinion. There's also\n>some concerns in early iterations of this \"**\" support that we would\n>need to revisit if we want 'rsync' behavior. I'm not very excited\n>about doing that.\n\nThat's fair.\n\nI guess another point in favour of your second option is that it's essentially\nthe same behaviour used by bash (with the `globstar` option) and zsh (with the\ndefault options); they also give `**` special recursion powers only when used in\na path component by itself, otherwise it acts like `*`. So there's precedent\nthere.\n\ndana\n\n"},{"id":"361778","messageId":"20181027084823.23382-1-pclouds@gmail.com","threadId":"49539","inReplyTo":"C16A9F17-0375-42F9-90A9-A92C9F3D8BBA@dana.is","subject":"[PATCH] wildmatch: change behavior of \"foo**bar\" in WM_PATHNAME mode","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-10-27T08:48:23Z","receivedAt":"2018-10-27T08:48:38Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"In WM_PATHNAME mode (or FNM_PATHNAME), '*' does not match '/' and '**'\ncan but only in three patterns:\n\n- '**/' matches zero or more leading directories\n- '/**/' matches zero or more directories in between\n- '/**' matches zero or more trailing directories/files\n\nWhen '**' is present but not in one of these patterns, the current\nbehavior is consider the pattern invalid and stop matching. In other\nwords, 'foo**bar' never matches anything, whatever you throw at it.\n\nThis behavior is arguably a bit confusing partly because we can't\nreally tell the user their pattern is invalid so that they can fix\nit. So instead, tolerate it and make '**' act like two regular '*'s\n(which is essentially the same as a single asterisk). This behavior\nseems more predictable.\n\nNoticed-by: dana \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Documentation/gitignore.txt | 3 ++-\n t/t3070-wildmatch.sh | 4 ++--\n wildmatch.c | 4 ++--\n wildmatch.h | 1 -\n 4 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt\nindex d107daaffd..1c94f08ff4 100644\n--- a/Documentation/gitignore.txt\n+++ b/Documentation/gitignore.txt\n@@ -129,7 +129,8 @@ full pathname may have special meaning:\n matches zero or more directories. For example, \"`a/**/b`\"\n matches \"`a/b`\", \"`a/x/b`\", \"`a/x/y/b`\" and so on.\n \n- - Other consecutive asterisks are considered invalid.\n+ - Other consecutive asterisks are considered regular asterisks and\n+ will match according to the previous rules.\n \n NOTES\n -----\ndiff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh\nindex 46aca0af10..891d4d7cb9 100755\n--- a/t/t3070-wildmatch.sh\n+++ b/t/t3070-wildmatch.sh\n@@ -237,7 +237,7 @@ match 0 0 0 0 foobar 'foo\\*bar'\n match 1 1 1 1 'f\\oo' 'f\\\\oo'\n match 1 1 1 1 ball '*[al]?'\n match 0 0 0 0 ten '[ten]'\n-match 0 0 1 1 ten '**[!te]'\n+match 1 1 1 1 ten '**[!te]'\n match 0 0 0 0 ten '**[!ten]'\n match 1 1 1 1 ten 't[a-g]n'\n match 0 0 0 0 ten 't[!a-g]n'\n@@ -253,7 +253,7 @@ match 1 1 1 1 ']' ']'\n # Extended slash-matching features\n match 0 0 1 1 'foo/baz/bar' 'foo*bar'\n match 0 0 1 1 'foo/baz/bar' 'foo**bar'\n-match 0 0 1 1 'foobazbar' 'foo**bar'\n+match 1 1 1 1 'foobazbar' 'foo**bar'\n match 1 1 1 1 'foo/baz/bar' 'foo/**/bar'\n match 1 1 0 0 'foo/baz/bar' 'foo/**/**/bar'\n match 1 1 1 1 'foo/b/a/z/bar' 'foo/**/bar'\ndiff --git a/wildmatch.c b/wildmatch.c\nindex d074c1be10..9e9e2a2f95 100644\n--- a/wildmatch.c\n+++ b/wildmatch.c\n@@ -104,8 +104,8 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n \t\t\t\t\t dowild(p + 1, text, flags) == WM_MATCH)\n \t\t\t\t\t\treturn WM_MATCH;\n \t\t\t\t\tmatch_slash = 1;\n-\t\t\t\t} else\n-\t\t\t\t\treturn WM_ABORT_MALFORMED;\n+\t\t\t\t} else /* WM_PATHNAME is set */\n+\t\t\t\t\tmatch_slash = 0;\n \t\t\t} else\n \t\t\t\t/* without WM_PATHNAME, '*' == '**' */\n \t\t\t\tmatch_slash = flags & WM_PATHNAME ? 0 : 1;\ndiff --git a/wildmatch.h b/wildmatch.h\nindex b8c826aa68..5993696298 100644\n--- a/wildmatch.h\n+++ b/wildmatch.h\n@@ -4,7 +4,6 @@\n #define WM_CASEFOLD 1\n #define WM_PATHNAME 2\n \n-#define WM_ABORT_MALFORMED 2\n #define WM_NOMATCH 1\n #define WM_MATCH 0\n #define WM_ABORT_ALL -1\n-- \n2.19.1.647.g708186aaf9\n\n"},{"id":"361823","messageId":"20181028062533.GA15061@tor.lan","threadId":"49539","inReplyTo":"20181027084823.23382-1-pclouds@gmail.com","subject":"Re: [PATCH] wildmatch: change behavior of \"foo**bar\" in WM_PATHNAME mode","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2018-10-28T06:25:33Z","receivedAt":"2018-10-28T06:29:35Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Sat, Oct 27, 2018 at 10:48:23AM +0200, Nguyễn Thái Ngọc Duy wrote:\n> In WM_PATHNAME mode (or FNM_PATHNAME), '*' does not match '/' and '**'\n> can but only in three patterns:\n> \n> - '**/' matches zero or more leading directories\n> - '/**/' matches zero or more directories in between\n> - '/**' matches zero or more trailing directories/files\n> \n> When '**' is present but not in one of these patterns, the current\n> behavior is consider the pattern invalid and stop matching. In other\n> words, 'foo**bar' never matches anything, whatever you throw at it.\n> \n> This behavior is arguably a bit confusing partly because we can't\n> really tell the user their pattern is invalid so that they can fix\n> it. So instead, tolerate it and make '**' act like two regular '*'s\n> (which is essentially the same as a single asterisk). This behavior\n> seems more predictable.\n\nNice analyzes.\nI have one question here:\nIf the user specifies '**' and nothhing is found,\nwould it be better to die() with a useful message\ninstead of silently correcting it ?\n\nSee the the patch below:\n> -\t\t\t\t} else\n> -\t\t\t\t\treturn WM_ABORT_MALFORMED;\n\nWould it be possible to put in the die() here?\nAs it is outlined so nicely above, a '**' must have either a '/'\nbefore, or behind, or both, to make sense.\nWhen there is no '/' then the user specified something wrong.\nEither a '/' has been forgotten, or the '*' key may be bouncing.\nI don't think that Git should assume anything here.\n(but I didn't follow the previous discussions, so I may have missed\nsome arguments.)\n\n[]\n"},{"id":"361824","messageId":"CACsJy8BoWmWaVpXnSL8Stm_2XDnKfXSaj2i0UwZg7aEgYF9i4w@mail.gmail.com","threadId":"49539","inReplyTo":"20181028062533.GA15061@tor.lan","subject":"Re: [PATCH] wildmatch: change behavior of \"foo**bar\" in WM_PATHNAME mode","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-10-28T06:35:38Z","receivedAt":"2018-10-28T06:36:08Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Oct 28, 2018 at 7:25 AM Torsten Bögershausen wrote:\n>\n> On Sat, Oct 27, 2018 at 10:48:23AM +0200, Nguyễn Thái Ngọc Duy wrote:\n> > In WM_PATHNAME mode (or FNM_PATHNAME), '*' does not match '/' and '**'\n> > can but only in three patterns:\n> >\n> > - '**/' matches zero or more leading directories\n> > - '/**/' matches zero or more directories in between\n> > - '/**' matches zero or more trailing directories/files\n> >\n> > When '**' is present but not in one of these patterns, the current\n> > behavior is consider the pattern invalid and stop matching. In other\n> > words, 'foo**bar' never matches anything, whatever you throw at it.\n> >\n> > This behavior is arguably a bit confusing partly because we can't\n> > really tell the user their pattern is invalid so that they can fix\n> > it. So instead, tolerate it and make '**' act like two regular '*'s\n> > (which is essentially the same as a single asterisk). This behavior\n> > seems more predictable.\n>\n> Nice analyzes.\n> I have one question here:\n> If the user specifies '**' and nothing is found,\n> would it be better to die() with a useful message\n> instead of silently correcting it ?\n\nConsider the main use case of wildmatch, .gitignore patterns, dying\nwould be really bad because it can affect a lot of commands. It would\nbe much better if wildmatch api allows the caller to handle the error\nso they can either die() or propagate the error upwards. But even then\nthe current API is not suited for that, ideally we should have a\ncompile phase where you can validate the pattern first. Without it,\nyou encounter the error every time you try to match something and\nhandling errors becomes much uglier.\n\nAnd it goes against the way pattern errors are handled by\nfnmatch/wildmatch anyway. If you write '[ab' instead of '[ab]', '['\nwill just be considered literal '[' instead of erroring out.\n\n> See the the patch below:\n> > - } else\n> > - return WM_ABORT_MALFORMED;\n>\n> Would it be possible to put in the die() here?\n> As it is outlined so nicely above, a '**' must have either a '/'\n> before, or behind, or both, to make sense.\n> When there is no '/' then the user specified something wrong.\n> Either a '/' has been forgotten, or the '*' key may be bouncing.\n> I don't think that Git should assume anything here.\n> (but I didn't follow the previous discussions, so I may have missed\n> some arguments.)\n>\n> []\n\n\n\n-- \nDuy\n"},{"id":"361861","messageId":"xmqqin1l8p1i.fsf@gitster-ct.c.googlers.com","threadId":"49539","inReplyTo":"CACsJy8BoWmWaVpXnSL8Stm_2XDnKfXSaj2i0UwZg7aEgYF9i4w@mail.gmail.com","subject":"Re: [PATCH] wildmatch: change behavior of \"foo**bar\" in WM_PATHNAME mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-29T02:28:09Z","receivedAt":"2018-10-29T02:28:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n>> Nice analyzes.\n>> I have one question here:\n>> If the user specifies '**' and nothing is found,\n>> would it be better to die() with a useful message\n>> instead of silently correcting it ?\n>\n> Consider the main use case of wildmatch, .gitignore patterns, dying\n> would be really bad because it can affect a lot of commands....\n\nIf the user gives 'foo*' and nothing is found, we may say \"no match\"\nand some codepaths that uses wildmatch API may die. And in such place,\nwhen the user gives '**' and nothing is found, we should do the same\nin the same codepath. In either case, the implementation of wildmatch\nAPI is not the place to call a die(), I think.\n\nAnd yes, treating an unanchored \"**\" as if there is just a \"*\" followed\nby another '*\" makes good sense.\n\nThanks, both.\n"},{"id":"361895","messageId":"87h8h4lwcv.fsf@evledraar.gmail.com","threadId":"49539","inReplyTo":"20181027084823.23382-1-pclouds@gmail.com","subject":"Re: [PATCH] wildmatch: change behavior of \"foo**bar\" in WM_PATHNAME mode","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-10-29T13:24:00Z","receivedAt":"2018-10-29T13:24:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Oct 27 2018, Nguyễn Thái Ngọc Duy wrote:\n\n> In WM_PATHNAME mode (or FNM_PATHNAME), '*' does not match '/' and '**'\n> can but only in three patterns:\n>\n> - '**/' matches zero or more leading directories\n> - '/**/' matches zero or more directories in between\n> - '/**' matches zero or more trailing directories/files\n>\n> When '**' is present but not in one of these patterns, the current\n> behavior is consider the pattern invalid and stop matching. In other\n> words, 'foo**bar' never matches anything, whatever you throw at it.\n>\n> This behavior is arguably a bit confusing partly because we can't\n> really tell the user their pattern is invalid so that they can fix\n> it. So instead, tolerate it and make '**' act like two regular '*'s\n> (which is essentially the same as a single asterisk). This behavior\n> seems more predictable.\n>\n> Noticed-by: dana \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> Documentation/gitignore.txt | 3 ++-\n> t/t3070-wildmatch.sh | 4 ++--\n> wildmatch.c | 4 ++--\n> wildmatch.h | 1 -\n> 4 files changed, 6 insertions(+), 6 deletions(-)\n>\n> diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt\n> index d107daaffd..1c94f08ff4 100644\n> --- a/Documentation/gitignore.txt\n> +++ b/Documentation/gitignore.txt\n> @@ -129,7 +129,8 @@ full pathname may have special meaning:\n> matches zero or more directories. For example, \"`a/**/b`\"\n> matches \"`a/b`\", \"`a/x/b`\", \"`a/x/y/b`\" and so on.\n>\n> - - Other consecutive asterisks are considered invalid.\n> + - Other consecutive asterisks are considered regular asterisks and\n> + will match according to the previous rules.\n>\n> NOTES\n> -----\n> diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh\n> index 46aca0af10..891d4d7cb9 100755\n> --- a/t/t3070-wildmatch.sh\n> +++ b/t/t3070-wildmatch.sh\n> @@ -237,7 +237,7 @@ match 0 0 0 0 foobar 'foo\\*bar'\n> match 1 1 1 1 'f\\oo' 'f\\\\oo'\n> match 1 1 1 1 ball '*[al]?'\n> match 0 0 0 0 ten '[ten]'\n> -match 0 0 1 1 ten '**[!te]'\n> +match 1 1 1 1 ten '**[!te]'\n> match 0 0 0 0 ten '**[!ten]'\n> match 1 1 1 1 ten 't[a-g]n'\n> match 0 0 0 0 ten 't[!a-g]n'\n> @@ -253,7 +253,7 @@ match 1 1 1 1 ']' ']'\n> # Extended slash-matching features\n> match 0 0 1 1 'foo/baz/bar' 'foo*bar'\n> match 0 0 1 1 'foo/baz/bar' 'foo**bar'\n> -match 0 0 1 1 'foobazbar' 'foo**bar'\n> +match 1 1 1 1 'foobazbar' 'foo**bar'\n> match 1 1 1 1 'foo/baz/bar' 'foo/**/bar'\n> match 1 1 0 0 'foo/baz/bar' 'foo/**/**/bar'\n> match 1 1 1 1 'foo/b/a/z/bar' 'foo/**/bar'\n> diff --git a/wildmatch.c b/wildmatch.c\n> index d074c1be10..9e9e2a2f95 100644\n> --- a/wildmatch.c\n> +++ b/wildmatch.c\n> @@ -104,8 +104,8 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n> \t\t\t\t\t dowild(p + 1, text, flags) == WM_MATCH)\n> \t\t\t\t\t\treturn WM_MATCH;\n> \t\t\t\t\tmatch_slash = 1;\n> -\t\t\t\t} else\n> -\t\t\t\t\treturn WM_ABORT_MALFORMED;\n> +\t\t\t\t} else /* WM_PATHNAME is set */\n> +\t\t\t\t\tmatch_slash = 0;\n> \t\t\t} else\n> \t\t\t\t/* without WM_PATHNAME, '*' == '**' */\n> \t\t\t\tmatch_slash = flags & WM_PATHNAME ? 0 : 1;\n> diff --git a/wildmatch.h b/wildmatch.h\n> index b8c826aa68..5993696298 100644\n> --- a/wildmatch.h\n> +++ b/wildmatch.h\n> @@ -4,7 +4,6 @@\n> #define WM_CASEFOLD 1\n> #define WM_PATHNAME 2\n>\n> -#define WM_ABORT_MALFORMED 2\n> #define WM_NOMATCH 1\n> #define WM_MATCH 0\n> #define WM_ABORT_ALL -1\n\nThis patch looks good to me, but I think it's a bad state of affairs to\nkeep changing these semantics and not having something like a\n\"gitwildmatch\" doc were we document this matching syntax.\n\nAlso I still need to dig up the work for using PCRE as an alternate\nmatching engine, the PCRE devs produced a bug-for-bug compatible version\nof our wildmatch function (all the more reason to document it), so I\nthink they'll need to change it now that this is in, but I haven't\nrebased those ancient patches yet.\n\nDo you have any thoughts on how to proceed with getting this documented\n/ into some stable state where we can specify it? Even if we don't end\nup using PCRE as a matching engine (sometimes it was faster, sometimes\nslower) I think it would be very useful if we can spew out \"here's your\npattern as a regex\" for self-documentation purposes.\n\nThen that can be piped into e.g. \"perl -Mre=debug\" to see a step-by-step\nguide for how the pattern compiles, and why it does or doesn't match a\ngiven thing.\n"},{"id":"361912","messageId":"CACsJy8CLHQ0mKhKXvTDAqy9TLwEFBSvHEu5UbPxHX4is2mK+Cg@mail.gmail.com","threadId":"49539","inReplyTo":"87h8h4lwcv.fsf@evledraar.gmail.com","subject":"Re: [PATCH] wildmatch: change behavior of \"foo**bar\" in WM_PATHNAME mode","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-10-29T15:53:52Z","receivedAt":"2018-10-29T15:54:23Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Oct 29, 2018 at 2:24 PM Ævar Arnfjörð Bjarmason\n wrote:\n> This patch looks good to me, but I think it's a bad state of affairs to\n> keep changing these semantics and not having something like a\n> \"gitwildmatch\" doc were we document this matching syntax.\n\nWhile we don't have a separate document for it, the behavior _is_\ndocumented even if perhaps it wasn't as clear. There were even tests\nfor this corner case.\n\n> Do you have any thoughts on how to proceed with getting this documented\n> / into some stable state where we can specify it?\n\nwildmatch has been used in git for a few years if I remember correctly\nso to me it is stable. Granted there are corner cases like this, but I\ncan't prevent all bugs (especially this one which is more like design\nmistake than bug per se). You are welcome to refactor gitignore.txt\nand add gitwildmatch.txt.\n-- \nDuy\n"}]}