{"thread":{"id":"54984","subject":"[bug] git-ls-files sometimes does not list files with pathspec magic \":(exclude)\"","startedAt":"2021-01-13T17:43:03Z","lastAt":"2021-01-14T20:21:57Z","messageCount":3,"participants":["Thomas Haller","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"414274","messageId":"e2dbe996f6a7285fe0487e34d65eccf712867547.camel@redhat.com","threadId":"54984","inReplyTo":null,"subject":"[bug] git-ls-files sometimes does not list files with pathspec magic \":(exclude)\"","fromName":"Thomas Haller","fromEmail":"thaller@redhat.com","sentAt":"2021-01-13T17:41:28Z","receivedAt":"2021-01-13T17:43:03Z","isPatch":false,"sender":{"key":"thaller@redhat.com","avatar":null},"body":"Hi,\n\n\nIt seems git-ls-files has an issue together with certain \":(exclude)\"\ntags.\n\nFor example, on NetworkManager's git tree (top-level directory) I run\n\n  git ls-files -- src/platform/ \":(exclude)shared/n-acd\"\n\nwhich wrongly lists not files. It seems to be some\nrelation between the exclude and the search path because a different\npath/exclude does not exhibit the problem.\n\nI also tested current git-master (72c4083ddf91b489b7b7b812df67ee8842177d98)\nwhich has the same issue.\n\nHere is a reproducer (in a container run on Fedora 33/x86_64):\n\n  podman run -ti alpine:latest \\\n    sh -c '\n      apk add git &&\n      git clone https://gitlab.freedesktop.org/NetworkManager/NetworkManager.git &&\n      cd NetworkManager &&\n      git checkout -B tmp cd754680a6a0e35b286d4157269053ccc3996a32 &&\n      echo \">>>>>>now BAD1...\" &&\n      git ls-files -- src/platform/ \":(exclude)shared/n-acd\"\n      echo \">>>>>>now GOOD1...\" &&\n      git ls-files -- src/platform/ \":(exclude)shared/c-list\"\n      echo \">>>>>>now GOOD2...\" &&\n      git ls-files -- src/platform \":(exclude)shared/n-acd\"\n      echo \">>>>>>now GOOD3...\" &&\n      git ls-files -- src/vpn/ \":(exclude)shared/n-acd\"\n   '\n\nNote that only the first call in the reproducer has the unexpected\nresult.\n\n\nbest,\nThomas\n\n"},{"id":"414312","messageId":"xmqqa6tc1dab.fsf@gitster.c.googlers.com","threadId":"54984","inReplyTo":"e2dbe996f6a7285fe0487e34d65eccf712867547.camel@redhat.com","subject":"Re: [bug] git-ls-files sometimes does not list files with pathspec magic \":(exclude)\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-14T02:07:56Z","receivedAt":"2021-01-14T02:09:03Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Haller <thaller@redhat.com> writes:\n\n>       git ls-files -- src/platform/ \":(exclude)shared/n-acd\"\n\nThis does look interesting.\n\n$ git ls-files -- src/platform/ \":(exclude)shared/n-acd\"\n$ git ls-files -- src/platform/ \":(exclude)??????/?????\"\n$ git ls-files -- src/platform/ \":(exclude)??????/????\"\n$ git ls-files -- src/platform/ \":(exclude)??????/???\"\n$ git ls-files -- src/platform/ \":(exclude)??????/??\"\n$ git ls-files -- src/platform/ \":(exclude)??????/?\"\n\nNone of the above gives any output.  And the '/' seems to be a red\nherring.  None of the below (where the '/' in the exclude pathspec\nis replaced with a single '?') gives any output, either.\n\n$ git ls-files -- src/platform/ \":(exclude)????????????\"\n$ git ls-files -- src/platform/ \":(exclude)???????????\"\n$ git ls-files -- src/platform/ \":(exclude)??????????\"\n$ git ls-files -- src/platform/ \":(exclude)?????????\"\n$ git ls-files -- src/platform/ \":(exclude)????????\"\n\nBut if we add one more \"?\" to the longuest ones, i.e.\n\n$ git ls-files -- src/platform/ \":(exclude)?????????????\"\n$ git ls-files -- src/platform/ \":(exclude)shared/n-acd?\"\n\nwe start seeing output.\n\nWhat is curious is that the longest problematic negative pathspec,\n\"shared/n-acd\" or \"????????????\", have the same length as\n\"src/platform\" without the trailing slash.\n\n    \"shared/n-acd\"\n    \"src/platform/\"\n    \"????????????\"\n\nThe rule IIUC is that a path must match one of the positive pathspec\nand none of the negative pathspec, but it looks as if there is some\nbogus optimization based on string length.\n\nAn experiment.  These ought to do the same as the first exacmple:\n\n$ git ls-files -- src/platform/ \":(exclude)shared/n-ac[d]\"\n$ git ls-files -- src/platform/ \":(exclude)shared/n-[acd][acd][acd]\"\n\nbut probably because of the character class [d], it seems to\ndefeat/bypass the broken \"optimization\" and gives what we expect\nback.\n\nI'll have to go back to the desk where I have a development\nenvironment (not an end user enviornment) to dig deeper, but this is\nintriguing.  I'll look more later unless somebody else beats me to\nit.\n\nThanks for a report.\n"},{"id":"414386","messageId":"YACnuuQ9ezo6Bb9H@coredump.intra.peff.net","threadId":"54984","inReplyTo":"xmqqa6tc1dab.fsf@gitster.c.googlers.com","subject":"Re: [bug] git-ls-files sometimes does not list files with pathspec magic \":(exclude)\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-14T20:21:14Z","receivedAt":"2021-01-14T20:21:57Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 13, 2021 at 06:07:56PM -0800, Junio C Hamano wrote:\n\n> The rule IIUC is that a path must match one of the positive pathspec\n> and none of the negative pathspec, but it looks as if there is some\n> bogus optimization based on string length.\n\nI dug a little on this earlier this morning, but didn't get far enough\nto have any confidence that I wasn't barking totally up the wrong tree.\nBut I found the way the \"prefix\" is passed match_pathspec to be\nconfusing. This seems to make the problem go away:\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex c8eae899b8..93796404bd 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -232,7 +232,7 @@ static void show_ce(struct repository *repo, struct dir_struct *dir,\n \t    is_submodule_active(repo, ce->name)) {\n \t\tshow_submodule(repo, dir, ce->name);\n \t} else if (match_pathspec(repo->index, &pathspec, fullname, strlen(fullname),\n-\t\t\t\t  max_prefix_len, ps_matched,\n+\t\t\t\t  0, ps_matched,\n \t\t\t\t  S_ISDIR(ce->ce_mode) ||\n \t\t\t\t  S_ISGITLINK(ce->ce_mode))) {\n \t\ttag = get_tag(ce, tag);\n\nwhich I guess is likewise disabling the same optimization you're talking\nabout. But that probably means the bug is in match_pathspec().\n\n(I probably won't look further at this for a while, so please continue\nyour prodding; I just wanted to drop what little knowledge I came up\nwith).\n\n-Peff\n"}]}