{"thread":{"id":"52383","subject":"[BUG?] ls-files -o now traverses nested repo when given multiple pathspecs","startedAt":"2019-12-03T22:08:08Z","lastAt":"2019-12-08T23:00:05Z","messageCount":8,"participants":["Kyle Meyer","Elijah Newren","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"387466","messageId":"87fti15agv.fsf@kyleam.com","threadId":"52383","inReplyTo":null,"subject":"[BUG?] ls-files -o now traverses nested repo when given multiple pathspecs","fromName":"Kyle Meyer","fromEmail":"kyle@kyleam.com","sentAt":"2019-12-03T22:08:00Z","receivedAt":"2019-12-03T22:08:08Z","isPatch":false,"sender":{"key":"kyle@kyleam.com","avatar":"https://avatars.githubusercontent.com/u/1297788?v=4"},"body":"89a1f4aaf7 (dir: if our pathspec might match files under a dir, recurse\ninto it, 2019-09-17) introduced a change in behavior in terms of\ntraversing untracked nested repositories.  Say we have a repository that\ncontains a single untracked repository with untracked content:\n\n    $ git init && git init a && touch a/x\n\nCalling ls-files with the nested repository as the sole pathspec does\nnot recurse into that repository:\n\n    $ git ls-files --other a\n    a/\n\nHowever, as of 89a1f4aaf7, adding an additional pathspec results in the\nnested repository being traversed:\n\n    $ git ls-files --other a foo\n    a/\n    a/x\n\nReading 89a1f4aaf7 and skimming the patch series and related thread [*],\nI haven't found anything that makes me think this change in behavior was\nintentional.\n\n[*]: https://lore.kernel.org/git/20190905154735.29784-1-newren@gmail.com/\n"},{"id":"387496","messageId":"CABPp-BFG3FkTkC=L1v97LUksndkOmCN8ZhNJh5eoNdquE7v9DA@mail.gmail.com","threadId":"52383","inReplyTo":"87fti15agv.fsf@kyleam.com","subject":"Re: [BUG?] ls-files -o now traverses nested repo when given multiple pathspecs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-12-04T17:30:41Z","receivedAt":"2019-12-04T17:30:55Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Kyle,\n\nThanks for the clear report and pointing out relevant commit(s) and\ndiscussion.  Apologies in advance if I come off a bit ranty; it's\ndirected to dir.c and its API and not you.  It seems to be quite the\nmess...\n\nOn Tue, Dec 3, 2019 at 2:08 PM Kyle Meyer <kyle@kyleam.com> wrote:\n>\n> 89a1f4aaf7 (dir: if our pathspec might match files under a dir, recurse\n> into it, 2019-09-17) introduced a change in behavior in terms of\n> traversing untracked nested repositories.  Say we have a repository that\n> contains a single untracked repository with untracked content:\n>\n>     $ git init && git init a && touch a/x\n>\n> Calling ls-files with the nested repository as the sole pathspec does\n> not recurse into that repository:\n>\n>     $ git ls-files --other a\n>     a/\n>\n> However, as of 89a1f4aaf7, adding an additional pathspec results in the\n> nested repository being traversed:\n>\n>     $ git ls-files --other a foo\n>     a/\n>     a/x\n>\n> Reading 89a1f4aaf7 and skimming the patch series and related thread [*],\n> I haven't found anything that makes me think this change in behavior was\n> intentional.\n\nOh man, I guess I shouldn't be surprised by another area of the code\nthat depends on dir.c but lacks any meaningful tests of its behavior,\nviolated the existing contract[1], and depends on the side effects of\nother bugs -- bugs which don't cover all cases and thus causes it to\nget different behavior depending on things that otherwise shouldn't\nmatter.  Behold, *before* my changes to dir.c:\n\n$ git --version\n2.23.0\n$ find . -type f | grep -v .git\n./empty\n./untracked_repo/empty\n./untracked_dir/empty\n./world\n$ git ls-files\nworld\n$ git ls-files -o\nempty\nuntracked_dir/empty\nuntracked_repo/\n\nSo, as you say, it wouldn't traverse into untracked_repo/.  Now we\nstart adding pathspecs:\n\n$ git ls-files -o untracked_repo\nuntracked_repo/\n\nAs you mentioned, it won't traverse into it even when specified...\n\n$ git ls-files -o untracked_repo/\nuntracked_repo/empty\n\n...except that it does traverse into this directory if the user tab\ncompletes the name or otherwise manually adds a trailing slash.\nWeird, let's try multiple pathspecs:\n\n$ git ls-files -o untracked_dir untracked_repo\nuntracked_dir/empty\nuntracked_repo/\n\n$ git ls-files -o untracked_dir untracked_repo/\nuntracked_dir/empty\nuntracked_repo/\n\nSo it will traverse into the untracked_repo when specified as\n'untracked_repo/' but not if there are more than one pathspec given?!?\n And it traverses into an untracked directory regardless of the\ntrailing slash?  <sarcasm>What a paragon of consistency...</sarcasm>\n\n\nAt least my changes in git-2.24.0 made the behavior consistent; it'll\nalways traverse into a directory that matches a given pathspec.  As\nfor whether that's desirable or not when the pathspec is a submodule,\nI'm not certain.  My fixes to dir.c stalled out for over a year and a\nhalf despite a few reports that they had fixed issues people were\ncontinuing to report in the wild because the whole traversal logic was\nsuch a mess and there were so many dependencies on existing behavior\nbuilt up with that mess that it was really hard to determine what\n\"correct\" behavior was and others seemed to be unwilling to wade\nthrough the muck to figure out where sanity might lie so they could\nhelp give me pointers or opinions on what \"correct\" was[2].\n\nBut here are some possibilities that at least sound sane:\n\nA) ls-files -o should traverse into untracked submodules.  This case\nis easy; the code already does that.\n\nB) ls-files -o should NOT traverse into untracked submodules AND\nshould not even report them.  If so, fix looks like this:\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex f069a028ce..f144d44d8b 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -301,6 +301,9 @@ static void show_files(struct repository *repo,\nstruct dir_struct *dir)\n        int i;\n        struct strbuf fullname = STRBUF_INIT;\n\n+       if (!recurse_submodules)\n+               dir->flags |= DIR_SKIP_NESTED_GIT;\n+\n        /* For cached/deleted files we don't need to even do the readdir */\n        if (show_others || show_killed) {\n                if (!show_others)\n\nC) ls-files -o should NOT traverse into untracked submodules, but\nshould at least report their directory name.  If so, the fix is\nprobably to move the DIR_NO_GITLINKS if-block within\ndir.c:treat_directory() to put it right next to the\nDIR_SKIP_NESTED_GIT if-block (and maybe even partially combine the\ntwo) so that it comes before some of the other code in that function.\nMight have to be careful about checking for the presence of trailing\nslashes.\n\n\nIt seems like it should be clear which one of these is \"correct\", but\nI seem to be short a few brain cycles right now...either that, or\nmaybe my very rare use of submodules and nested repositories means I\njust don't have enough context to answer.  Maybe it's obvious to\nsomeone else...\n\n\nElijah\n\n[1] https://lore.kernel.org/git/xmqqefjp6sko.fsf@gitster-ct.c.googlers.com/\n[2] https://lore.kernel.org/git/20190905154735.29784-1-newren@gmail.com/\n"},{"id":"387506","messageId":"xmqqblsn514l.fsf@gitster-ct.c.googlers.com","threadId":"52383","inReplyTo":"CABPp-BFG3FkTkC=L1v97LUksndkOmCN8ZhNJh5eoNdquE7v9DA@mail.gmail.com","subject":"Re: [BUG?] ls-files -o now traverses nested repo when given multiple pathspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-04T19:42:02Z","receivedAt":"2019-12-04T19:42:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> C) ls-files -o should NOT traverse into untracked submodules, but\n> should at least report their directory name.\n\nI think this probably is the most sensible.  \n\nThe top-level directory of a working tree of a repository other than\nthe current one may exist in the working tree.  It is very tempting\nto declare that, unless we know it is a submodule that has the\ncurrent repository as its superproject, we should just treat it as a\nnormal subdirectory without *any* files tracked by the current\nrepository, which would mean that we pretend that the \".git/\" in\nthat subdirectory is not any special---but that would obviously make\nthings quite messy (e.g. our \"ls-files -o\" would descend into the\nother project's working tree and even in its .git/ directory), so we\nneed to special case a directory that has \".git/\" in it, whether it\nis a submodule for our current repository or not.\n\nThanks for working on this.  I agree that dir.c traversal has become\nmessier and messier, especially with its interaction with\nsubmodules.\n"},{"id":"387508","messageId":"874kyf6en3.fsf@kyleam.com","threadId":"52383","inReplyTo":"CABPp-BFG3FkTkC=L1v97LUksndkOmCN8ZhNJh5eoNdquE7v9DA@mail.gmail.com","subject":"Re: [BUG?] ls-files -o now traverses nested repo when given multiple pathspecs","fromName":"Kyle Meyer","fromEmail":"kyle@kyleam.com","sentAt":"2019-12-04T20:04:48Z","receivedAt":"2019-12-04T20:04:54Z","isPatch":false,"sender":{"key":"kyle@kyleam.com","avatar":"https://avatars.githubusercontent.com/u/1297788?v=4"},"body":"Hi Elijah,\n\nThanks for the detailed and helpful reply.\n\nElijah Newren <newren@gmail.com> writes:\n\n[...]\n\n> As you mentioned, it won't traverse into it even when specified...\n>\n> $ git ls-files -o untracked_repo/\n> untracked_repo/empty\n>\n> ...except that it does traverse into this directory if the user tab\n> completes the name or otherwise manually adds a trailing slash.\n\nAh yes, I recall encountering what I think is the same underlying issue\nwhen working on a previous series [0,1].  In the context of 'git add\nuntracked_repo/', there's been some discussion related to this trailing\nslash discrepancy at\n\n  https://lore.kernel.org/git/20180618111919.GA10085@book.hvoigt.net/T/#u\n\n> Weird, let's try multiple pathspecs:\n>\n> $ git ls-files -o untracked_dir untracked_repo\n> untracked_dir/empty\n> untracked_repo/\n>\n> $ git ls-files -o untracked_dir untracked_repo/\n> untracked_dir/empty\n> untracked_repo/\n>\n> So it will traverse into the untracked_repo when specified as\n> 'untracked_repo/' but not if there are more than one pathspec given?!?\n\nEh, indeed.\n\n>  And it traverses into an untracked directory regardless of the\n> trailing slash?  <sarcasm>What a paragon of consistency...</sarcasm>\n>\n>\n> At least my changes in git-2.24.0 made the behavior consistent; it'll\n> always traverse into a directory that matches a given pathspec.\n\nI might be getting mixed up, but the changes in 2.24.0 did introduce\nsome inconsistent behavior (in the no trailing slash case) with respect\nto giving a single pathspec and giving multiple pathspecs, no?  Using\nyour example:\n\n    $ git --version\n    git version 2.24.0\n    $ git ls-files -o untracked_repo\n    untracked_repo/\n    $ git ls-files -o untracked_repo empty\n    empty\n    untracked_repo/\n    untracked_repo/empty\n\n> As for whether that's desirable or not when the pathspec is a submodule,\n> I'm not certain. [...]\n>\n> But here are some possibilities that at least sound sane:\n>\n> A) ls-files -o should traverse into untracked submodules.  This case\n> is easy; the code already does that.\n\nHmm, but as shown in the last example, ls-files -o doesn't traverse into\nuntracked submodules for the single pathspec case.\n\n> B) ls-files -o should NOT traverse into untracked submodules AND\n> should not even report them.\n>\n> C) ls-files -o should NOT traverse into untracked submodules, but\n> should at least report their directory name.  If so, the fix is\n> [...]\n\nThis behavior---which matches the no-slash behavior when no patchspec or\na single pathspec is given (on both v2.24.0 and previous version) as\nwell as when multiple pathspecs are given (before v2.24.0)---is the one\nI prefer.  My biased reason for this preference is that in the DataLad\nproject we identify untracked nested repositories based on `ls-files -o\n<untracked directory>...` reporting only the directory name for\nrepositories.  (Looking into one of our tests that fails with Git\nv2.24.0 is how I ran into the reported change in behavior [2].)\n\nThat some external project relies on unintended ls-files output of\ncourse doesn't mean that Git should keep reporting things that way, but\nit does mean that I _hope_ that not traversing into untracked\nrepositories is the intended behavior and that traversing (either\nbecause a slash is appended or as of 89a1f4aaf7 because multiple\npathspecs are given) is not intended :>\n\n\n[0]: https://lore.kernel.org/git/20190409230737.26809-1-kyle@kyleam.com\n[1]: https://lore.kernel.org/git/87bm1mbua4.fsf@kyleam.com/\n[2]: https://github.com/datalad/datalad/issues/3890#issuecomment-561722194\n"},{"id":"387704","messageId":"871rtfv0wn.fsf@kyleam.com","threadId":"52383","inReplyTo":"874kyf6en3.fsf@kyleam.com","subject":"Re: [BUG?] ls-files -o now traverses nested repo when given multiple pathspecs","fromName":"Kyle Meyer","fromEmail":"kyle@kyleam.com","sentAt":"2019-12-08T05:31:20Z","receivedAt":"2019-12-08T05:33:18Z","isPatch":false,"sender":{"key":"kyle@kyleam.com","avatar":"https://avatars.githubusercontent.com/u/1297788?v=4"},"body":"Kyle Meyer <kyle@kyleam.com> writes:\n\n> Elijah Newren <newren@gmail.com> writes:\n>> [...]\n>> At least my changes in git-2.24.0 made the behavior consistent; it'll\n>> always traverse into a directory that matches a given pathspec.\n>\n> I might be getting mixed up, but the changes in 2.24.0 did introduce\n> some inconsistent behavior (in the no trailing slash case) with respect\n> to giving a single pathspec and giving multiple pathspecs, no?  Using\n> your example:\n>\n>     $ git --version\n>     git version 2.24.0\n>     $ git ls-files -o untracked_repo\n>     untracked_repo/\n>     $ git ls-files -o untracked_repo empty\n>     empty\n>     untracked_repo/\n>     untracked_repo/empty\n\nIt looks like the \"multiple pathspecs trigger traversal\" change isn't\nlimited to nested repositories.  It can also be observed with\n--directory and plain untracked directories.  Assume the tree layout\nfrom your example again.  With a single pathspec (and no slash),\n'ls-files -o --directory' will not expand the untracked directory's\ncontents:\n\n    $ git ls-files -o --directory untracked_dir\n    untracked_dir/\n\nBut, as of 89a1f4aaf7, tacking on an additional pathspec will cause\nls-files to traverse into the untracked directory:\n\n    $ git ls-files -o --directory untracked_dir empty\n    empty\n    untracked_dir/\n    untracked_dir/empty\n\nIn contrast, on 89a1f4aaf7^ the same command shows\n\n    $ git ls-files -o --directory untracked_dir empty\n    empty\n    untracked_dir/\n"},{"id":"387705","messageId":"CABPp-BEvr+wB_yqOAG9oOaONtckYzn-zghyAtx2fWJweg55ovA@mail.gmail.com","threadId":"52383","inReplyTo":"871rtfv0wn.fsf@kyleam.com","subject":"Re: [BUG?] ls-files -o now traverses nested repo when given multiple pathspecs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-12-08T05:42:00Z","receivedAt":"2019-12-08T05:42:13Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Kyle,\n\nOn Sat, Dec 7, 2019 at 9:31 PM Kyle Meyer <kyle@kyleam.com> wrote:\n>\n> Kyle Meyer <kyle@kyleam.com> writes:\n>\n> > Elijah Newren <newren@gmail.com> writes:\n> >> [...]\n> >> At least my changes in git-2.24.0 made the behavior consistent; it'll\n> >> always traverse into a directory that matches a given pathspec.\n> >\n> > I might be getting mixed up, but the changes in 2.24.0 did introduce\n> > some inconsistent behavior (in the no trailing slash case) with respect\n> > to giving a single pathspec and giving multiple pathspecs, no?  Using\n> > your example:\n> >\n> >     $ git --version\n> >     git version 2.24.0\n> >     $ git ls-files -o untracked_repo\n> >     untracked_repo/\n> >     $ git ls-files -o untracked_repo empty\n> >     empty\n> >     untracked_repo/\n> >     untracked_repo/empty\n>\n> It looks like the \"multiple pathspecs trigger traversal\" change isn't\n> limited to nested repositories.  It can also be observed with\n> --directory and plain untracked directories.  Assume the tree layout\n> from your example again.  With a single pathspec (and no slash),\n> 'ls-files -o --directory' will not expand the untracked directory's\n> contents:\n>\n>     $ git ls-files -o --directory untracked_dir\n>     untracked_dir/\n>\n> But, as of 89a1f4aaf7, tacking on an additional pathspec will cause\n> ls-files to traverse into the untracked directory:\n>\n>     $ git ls-files -o --directory untracked_dir empty\n>     empty\n>     untracked_dir/\n>     untracked_dir/empty\n>\n> In contrast, on 89a1f4aaf7^ the same command shows\n>\n>     $ git ls-files -o --directory untracked_dir empty\n>     empty\n>     untracked_dir/\n\nYeah, I spotted that too.  You left out a case, a single pathspec with\nthe trailing slash:\n\n   git ls-files -o --directory untracked_dir/\n\nThat will traverse into the directory before or after my changes.  I\nalso spotted a few other bugs, e.g. try out 'git ls-files -o .git/'\n(with either git-2.23 or git-2.24).  Whoops.  We do correctly avoid\ntraversing into the .git directory if multiple pathspecs are provided.\nAnyway, this whole area seems to be a bug factory.  Every time I think\nI'm close to having some patches to send to the list to fix up the\nissues I've found, I find the fix isn't where I thought it was and/or\nfind yet another bug.  Quite aggravating.\n\nI'm thinking of just sending the patches I have, since they fix up all\nthe issues we've discussed so far (including the .git/ case I just\nmentioned), and ignoring the 2-3 other bugs I found that are still\nbroken other than providing testcases documenting their breakage.\n"},{"id":"387706","messageId":"CABPp-BGpfATCdiat0A6OHx3aG22BzOcC37HG8wBMRrrbLG_Mfw@mail.gmail.com","threadId":"52383","inReplyTo":"CABPp-BEvr+wB_yqOAG9oOaONtckYzn-zghyAtx2fWJweg55ovA@mail.gmail.com","subject":"Re: [BUG?] ls-files -o now traverses nested repo when given multiple pathspecs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-12-08T07:46:03Z","receivedAt":"2019-12-08T07:47:29Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Dec 7, 2019 at 9:42 PM Elijah Newren <newren@gmail.com> wrote:\n>\n> Hi Kyle,\n>\n> On Sat, Dec 7, 2019 at 9:31 PM Kyle Meyer <kyle@kyleam.com> wrote:\n> >\n> > Kyle Meyer <kyle@kyleam.com> writes:\n> >\n> > > Elijah Newren <newren@gmail.com> writes:\n> > >> [...]\n> > >> At least my changes in git-2.24.0 made the behavior consistent; it'll\n> > >> always traverse into a directory that matches a given pathspec.\n> > >\n> > > I might be getting mixed up, but the changes in 2.24.0 did introduce\n> > > some inconsistent behavior (in the no trailing slash case) with respect\n> > > to giving a single pathspec and giving multiple pathspecs, no?  Using\n> > > your example:\n> > >\n> > >     $ git --version\n> > >     git version 2.24.0\n> > >     $ git ls-files -o untracked_repo\n> > >     untracked_repo/\n> > >     $ git ls-files -o untracked_repo empty\n> > >     empty\n> > >     untracked_repo/\n> > >     untracked_repo/empty\n> >\n> > It looks like the \"multiple pathspecs trigger traversal\" change isn't\n> > limited to nested repositories.  It can also be observed with\n> > --directory and plain untracked directories.  Assume the tree layout\n> > from your example again.  With a single pathspec (and no slash),\n> > 'ls-files -o --directory' will not expand the untracked directory's\n> > contents:\n> >\n> >     $ git ls-files -o --directory untracked_dir\n> >     untracked_dir/\n> >\n> > But, as of 89a1f4aaf7, tacking on an additional pathspec will cause\n> > ls-files to traverse into the untracked directory:\n> >\n> >     $ git ls-files -o --directory untracked_dir empty\n> >     empty\n> >     untracked_dir/\n> >     untracked_dir/empty\n> >\n> > In contrast, on 89a1f4aaf7^ the same command shows\n> >\n> >     $ git ls-files -o --directory untracked_dir empty\n> >     empty\n> >     untracked_dir/\n>\n> Yeah, I spotted that too.  You left out a case, a single pathspec with\n> the trailing slash:\n>\n>    git ls-files -o --directory untracked_dir/\n>\n> That will traverse into the directory before or after my changes.  I\n> also spotted a few other bugs, e.g. try out 'git ls-files -o .git/'\n> (with either git-2.23 or git-2.24).  Whoops.  We do correctly avoid\n> traversing into the .git directory if multiple pathspecs are provided.\n> Anyway, this whole area seems to be a bug factory.  Every time I think\n> I'm close to having some patches to send to the list to fix up the\n> issues I've found, I find the fix isn't where I thought it was and/or\n> find yet another bug.  Quite aggravating.\n>\n> I'm thinking of just sending the patches I have, since they fix up all\n> the issues we've discussed so far (including the .git/ case I just\n> mentioned), and ignoring the 2-3 other bugs I found that are still\n> broken other than providing testcases documenting their breakage.\n\nIf you want to take an early look, I've got some patches up at\nhttps://github.com/git/git/pull/676.  I plan to write a proper cover\nletter and submit to the list on Monday.\n"},{"id":"387723","messageId":"87v9qqtocz.fsf@kyleam.com","threadId":"52383","inReplyTo":"CABPp-BGpfATCdiat0A6OHx3aG22BzOcC37HG8wBMRrrbLG_Mfw@mail.gmail.com","subject":"Re: [BUG?] ls-files -o now traverses nested repo when given multiple pathspecs","fromName":"Kyle Meyer","fromEmail":"kyle@kyleam.com","sentAt":"2019-12-08T22:59:56Z","receivedAt":"2019-12-08T23:00:05Z","isPatch":false,"sender":{"key":"kyle@kyleam.com","avatar":"https://avatars.githubusercontent.com/u/1297788?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> If you want to take an early look, I've got some patches up at\n> https://github.com/git/git/pull/676.  I plan to write a proper cover\n> letter and submit to the list on Monday.\n\nI can confirm that your patches resolve the cases reported here.  Thank\nyou for working on this!\n"}]}