{"thread":{"id":"66233","subject":"[PATCH] dir: fix negative pathspecs in 'git ls-files' and 'git add'","startedAt":"2026-08-28T20:35:50Z","lastAt":"2026-10-02T21:59:39Z","messageCount":7,"participants":["Diogo Castro via GitGitGadget","Junio C Hamano","Diogo Castro"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"551432","messageId":"pull.2391.git.git.1787949348110.gitgitgadget@gmail.com","threadId":"66233","inReplyTo":null,"subject":"[PATCH] dir: fix negative pathspecs in 'git ls-files' and 'git add'","fromName":"Diogo Castro via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-08-28T20:35:48Z","receivedAt":"2026-08-28T20:35:50Z","isPatch":true,"body":"From: Diogo Castro \n\n`git ls-files` calls `common_prefix()` / `get_common_prefix_len()` which\ncalculate the length of the common prefix of all *positive* pathspecs,\n`max_prefix_len`.\n\n`max_prefix_len` is then passed to `match_pathspec()` ->\n`match_pathspec_with_flags()` -> `do_match_pathspec()`, which strips\n`max_prefix_len` bytes off of *all* paths and `match_pathspec_item()`\nstrips *all* pathspecs (positive or negative).\n\nThis causes the bug previously reported in [1].\n\nAs a result, when we run `git ls-files -- sub/sub/sub/file\n':(exclude)nonexistent'`:\n* The common prefix of the positive pathspecs is `sub/sub/sub`, 11 bytes\n* 11 bytes get stripped off both pathspecs:\n * \"sub/sub/sub/file\" becomes \"/file\"\n * \"nonexistent\" becomes \"\"\n* Since the negative pathspec degenerated into \"\", it matches every\n file, and thus no results are returned.\n\nWhen the common prefix is longer than the negative pathspec, we read out\nof bounds.\n\n`git add` suffers from the same issue. It uses `fill_directory()`, which\nreturns the common prefix length, but doesn't strip the trailing slash.\nUsing the same pathspecs as in the example above, the common prefix\nwould be `sub/sub/sub/`, 12 bytes.\n\nOnly `git ls-files` and `git add` are impacted. Other callers pass in\n`0` as the prefix.\n\nBug introduced in: ef79b1f870 (Support pathspec magic :(exclude) and its\nshort form :!, 2013-12-06).\n\nSolution: in `do_match_pathspec()`, only strip the prefix when handling\npositive pathspecs, not when handling negative pathspecs.\n\n[1]: https://lore.kernel.org/git/e2dbe996f6a7285fe0487e34d65eccf712867547.camel@redhat.com\n\nReported-by: Thomas Haller \nSigned-off-by: Diogo Castro \n---\n dir: fix negative pathspecs in git ls-files and git add\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2391%2Fdcastro%2Fdiogo.castro%2Ffix-pathspecs-common-prefix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2391/dcastro/diogo.castro/fix-pathspecs-common-prefix-v1\nPull-Request: https://github.com/git/git/pull/2391\n\n dir.c | 11 ++++++++\n t/t6132-pathspec-exclude.sh | 52 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 63 insertions(+)\n\ndiff --git a/dir.c b/dir.c\nindex 32430090dc..3fb2764efe 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -539,6 +539,17 @@ static int do_match_pathspec(struct index_state *istate,\n \t\t\treturn 0;\n \t}\n \n+\t/*\n+\t * The `prefix`, calculated by `common_prefix_len()`, only takes\n+\t * positive pathspecs into account. Negative pathspecs are not\n+\t * considered.\n+\t *\n+\t * Therefore, the prefix can only be stripped from positive\n+\t * pathspecs, not from negative pathspecs.\n+\t */\n+\tif (exclude)\n+\t\tprefix = 0;\n+\n \tname += prefix;\n \tnamelen -= prefix;\n \ndiff --git a/t/t6132-pathspec-exclude.sh b/t/t6132-pathspec-exclude.sh\nindex 9fdafeb1e9..dd54378019 100755\n--- a/t/t6132-pathspec-exclude.sh\n+++ b/t/t6132-pathspec-exclude.sh\n@@ -425,4 +425,56 @@ test_expect_success 'stash with all negative' '\n \ttest_cmp expect actual\n '\n \n+# `ls-files` finds the length of the common prefix of the *positive* pathspecs.\n+# In this example, there's only one positive pathspec, so the common prefix is `aaa/bbb`, with length 7.\n+#\n+# Before the bug described in https://lore.kernel.org/git/e2dbe996f6a7285fe0487e34d65eccf712867547.camel@redhat.com\n+# was patched, as an optimization, we would then strip the first 7 characters from the path,\n+# the positive pathspec, and (incorrectly) the negative pathspec.\n+#\n+# But stripping the negative pathspec would mean that `xxx/yyy/file` becomes `file`\n+# and we'd wrongly end up excluding `aaa/bbb/file`.\n+#\n+# After this bug fix, `aaa/bbb/file` should no longer be excluded by `:(exclude)xxx/yyy/file`.\n+test_expect_success 'exclude is not matched against the tail of the path' '\n+\ttest_when_finished \"git rm -q --cached -r aaa xxx && rm -rf aaa xxx\" &&\n+\tmkdir -p aaa/bbb xxx/yyy &&\n+\t>aaa/bbb/file &&\n+\t>xxx/yyy/other &&\n+\tgit add aaa xxx &&\n+\techo aaa/bbb/file >expect &&\n+\tgit ls-files -- aaa/bbb/file \":(exclude)xxx/yyy/file\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+# Before the bug described in https://lore.kernel.org/git/e2dbe996f6a7285fe0487e34d65eccf712867547.camel@redhat.com\n+# was patched, when the negative pathspec had the same length or was\n+# shorter than the common prefix of the positive pathspecs,\n+# then stripping the common prefix from the negative pathspec would result in an empty string,\n+# which would match everything, and thus exclude all files.\n+#\n+# In this test, the prefix for \"sub/sub/sub/file\" is \"sub/sub/sub\" (11 bytes).\n+test_expect_success 'ls-files keeps entries when an exclude matches the common prefix length' '\n+\techo sub/sub/sub/file >expect &&\n+\tgit ls-files -- sub/sub/sub/file \":(exclude)nonexistent\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+# This test is similar to the above, but tests `git add` instead of `git ls-files`.\n+#\n+# `git add` does not exclude the trailing slash, so the common prefix is \"sub/sub/sub/\" (12 bytes).\n+test_expect_success 'add keeps entries when an exclude matches the common prefix length' '\n+\ttest_when_finished \"git reset -q && rm -f sub/sub/sub/untracked\" &&\n+\t>sub/sub/sub/untracked &&\n+\tgit add -- sub/sub/sub/ \":(exclude)no/such/path\" &&\n+\techo sub/sub/sub/untracked >expect &&\n+\tgit diff --cached --name-only HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'an exclude shorter than the common prefix still excludes' '\n+\tgit ls-files -- sub/sub/sub/file \":(exclude)sub\" >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_done\n\nbase-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc\n-- \ngitgitgadget\n"},{"id":"551434","messageId":"xmqqwlta2agt.fsf@gitster.g","threadId":"66233","inReplyTo":"pull.2391.git.git.1787949348110.gitgitgadget@gmail.com","subject":"Re: [PATCH] dir: fix negative pathspecs in 'git ls-files' and 'git add'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-28T21:37:06Z","receivedAt":"2026-08-28T21:37:08Z","isPatch":true,"body":"\"Diogo Castro via GitGitGadget\" writes:\n\n> From: Diogo Castro \n>\n> `git ls-files` calls `common_prefix()` / `get_common_prefix_len()` which\n> calculate the length of the common prefix of all *positive* pathspecs,\n> `max_prefix_len`.\n> ...\n> Solution: in `do_match_pathspec()`, only strip the prefix when handling\n> positive pathspecs, not when handling negative pathspecs.\n\nHmph, if the command line were\n\n\tgit ls-files -- a/b/c a/b/d !a/b/\n\nshouldn't we strip a/b/ from all three? Would it make sense to\nleave the negative one relative to the full tree? I am wondering\nif the solution is to compute common prefix across both positive and\nnegative ones instead.\n"},{"id":"551473","messageId":"CAJw8QBPbxangB90DceDXxaDmyz8fn5jbEUihhe2faJrZ3o7BeQ@mail.gmail.com","threadId":"66233","inReplyTo":"xmqqwlta2agt.fsf@gitster.g","subject":"Re: [PATCH] dir: fix negative pathspecs in 'git ls-files' and 'git add'","fromName":"Diogo Castro","fromEmail":"dc@diogocastro.com","sentAt":"2026-08-30T14:57:27Z","receivedAt":"2026-08-30T14:57:45Z","isPatch":true,"body":"I don't think so.\n\nAs far as I can tell, the \"strip the common prefix\" feature is a\nperformance optimization aimed at avoiding walking the working\ndirectory needlessly.\nSo for `git add -- a/b/c a/b/d`, there's no need to look anywhere\nother than in `a/b/`.\n\nBut extending the \"strip the common prefix\" to negative pathspecs\ncould end up negating the benefits we get from this perf optimization.\nE.g. in `git add -- a/b/c a/b/d ':!*.md'`, there is no prefix common\nto *all* pathspecs, so we'd revert to walking the entire working\ndirectory, even though `a/b/` would still suffice.\n\n\nOn Sun, 30 Aug 2026 at 15:25, Junio C Hamano wrote:\n>\n> \"Diogo Castro via GitGitGadget\" writes:\n>\n> > From: Diogo Castro \n> >\n> > `git ls-files` calls `common_prefix()` / `get_common_prefix_len()` which\n> > calculate the length of the common prefix of all *positive* pathspecs,\n> > `max_prefix_len`.\n> > ...\n> > Solution: in `do_match_pathspec()`, only strip the prefix when handling\n> > positive pathspecs, not when handling negative pathspecs.\n>\n> Hmph, if the command line were\n>\n> git ls-files -- a/b/c a/b/d !a/b/\n>\n> shouldn't we strip a/b/ from all three? Would it make sense to\n> leave the negative one relative to the full tree? I am wondering\n> if the solution is to compute common prefix across both positive and\n> negative ones instead.\n>\n"},{"id":"551490","messageId":"xmqqmru3z051.fsf@gitster.g","threadId":"66233","inReplyTo":"CAJw8QBPbxangB90DceDXxaDmyz8fn5jbEUihhe2faJrZ3o7BeQ@mail.gmail.com","subject":"Re: [PATCH] dir: fix negative pathspecs in 'git ls-files' and 'git add'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-30T22:58:18Z","receivedAt":"2026-08-30T22:58:21Z","isPatch":true,"body":"Diogo Castro writes:\n\n> I don't think so.\n>\n> As far as I can tell, the \"strip the common prefix\" feature is a\n> performance optimization aimed at avoiding walking the working\n> directory needlessly.\n> So for `git add -- a/b/c a/b/d`, there's no need to look anywhere\n> other than in `a/b/`.\n>\n> But extending the \"strip the common prefix\" to negative pathspecs\n> could end up negating the benefits we get from this perf optimization.\n> E.g. in `git add -- a/b/c a/b/d ':!*.md'`, there is no prefix common\n> to *all* pathspecs, so we'd revert to walking the entire working\n> directory, even though `a/b/` would still suffice.\n\nI was wondering more about case like this:\n\n $ git add -- a/b/c a/b/d ':!a/b/x\n\nI agree that it is nonsense to compute the common prefix over only\npositive ones, and then to strip the common prefix from both\npositive and negative ones, and it needs to be corrected.\n"},{"id":"551559","messageId":"CAJw8QBMmv=zLN6sd_W9uQMF3H6Baatyq=TogLyZSFXK2gN4V8w@mail.gmail.com","threadId":"66233","inReplyTo":"a8955129fcb7478f9739c8586c6975e1@CWXP265MB5784.GBRP265.PROD.OUTLOOK.COM","subject":"Re: [PATCH] dir: fix negative pathspecs in 'git ls-files' and 'git add'","fromName":"Diogo Castro","fromEmail":"diogo.filipe.acastro@gmail.com","sentAt":"2026-08-31T14:30:22Z","receivedAt":"2026-08-31T14:30:34Z","isPatch":true,"body":"I think there's some misunderstanding, please allow me to take a step\nback and attempt to clarify.\nMy previous message was a reply to this:\n\n> I am wondering if the solution is to compute common prefix across both positive and negative ones instead.\n\nAs far as I can tell, this \"common prefix\" feature does not affect the\nsemantics of \"ls-files\" or \"add\", it doesn't affect which files are\nreported.\nIt only affects the performance.\n\nYour first example of \"git ls-files -- a/b/c a/b/d :!a/b/\" already\nworks correctly, the pattern \":!a/b/\" excludes everything from the\nfirst 2 pathspecs.\n\n\nSo the discussion to be had is purely about performance.\nMy point was that computing the common prefix across both positive\n*and* negative pathspecs would not improve performance, and might\nactually make it worse.\n\nThe \"common prefix\" is mainly used to avoid walking the entire working\ndirectory.\nA couple of examples to illustrate:\n\n* \"git add -- a/b/c a/b/d ':!a/b/x'\"\n * Under the current implementation, the common prefix is \"a/b/\",\nso as a performance optimization, we can look only into the \"a/b/\"\ndirectory and ignore the others.\n * Under your proposal of computing the \"common prefix across both\npositive and negative ones\", the common prefix would still be \"a/b/\",\nso performance wouldn't be affected.\n* \"git add -- a/b/c a/b/d ':!a/**/x'\"\n * Under the current implementation, the common prefix is \"a/b/\",\nlike in the example above.\n * Under your proposal, the common prefix would be \"a/\", so we'd\nhave to walk _more_ directories, which would hurt performance.\n\nDoes that answer your question? Or perhaps I misunderstood your point?\n"},{"id":"551590","messageId":"xmqqv78qw3hc.fsf@gitster.g","threadId":"66233","inReplyTo":"CAJw8QBMmv=zLN6sd_W9uQMF3H6Baatyq=TogLyZSFXK2gN4V8w@mail.gmail.com","subject":"Re: [PATCH] dir: fix negative pathspecs in 'git ls-files' and 'git add'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-31T18:26:39Z","receivedAt":"2026-08-31T18:26:42Z","isPatch":true,"body":"Diogo Castro writes:\n\n> My point was that computing the common prefix across both positive\n> *and* negative pathspecs would not improve performance, and might\n> actually make it worse.\n\nOK. Then that points at the right solution. Ignore negative ones\nwhen finding what the common prefix is, strip it only from positive\nones to reduce the width of the traversal to come up with the list\nof possible match candidates, and match them as full paths against\nthe negative ones to cull \"within the positive set but is excluded\"\npaths, and the posted patch looks good.\n\nI still wonder if we need different implementation when we have many\nmore negative patterns than the positive ones. In such a case, the\nstage to filter paths that matched one positive pattern by finding\nmatches with a negative pattern among many of them, which may\nbenefit from having a similar common prefix (among negative\npatterns) optimization, but that is a separate topic.\n\nThanks.\n\n"},{"id":"554015","messageId":"CAJw8QBOnpkXAG3i6BGL6nKeyPgssr9-qJ9vdgH_MhYJi9VhrMQ@mail.gmail.com","threadId":"66233","inReplyTo":"xmqqv78qw3hc.fsf@gitster.g","subject":"Re: [PATCH] dir: fix negative pathspecs in 'git ls-files' and 'git add'","fromName":"Diogo Castro","fromEmail":"dc@diogocastro.com","sentAt":"2026-10-02T21:52:53Z","receivedAt":"2026-10-02T21:59:39Z","isPatch":true,"body":"I see this work has been incorporated into [1]\n\nI'm closing the PR on git/git.\n\n[1]: https://lore.kernel.org/git/81EC0E28-13E7-4D10-BD07-3601124CBD77@ytausch.de/T/#t\n\nOn Mon, 31 Aug 2026 at 19:26, Junio C Hamano wrote:\n>\n> Diogo Castro writes:\n>\n> > My point was that computing the common prefix across both positive\n> > *and* negative pathspecs would not improve performance, and might\n> > actually make it worse.\n>\n> OK. Then that points at the right solution. Ignore negative ones\n> when finding what the common prefix is, strip it only from positive\n> ones to reduce the width of the traversal to come up with the list\n> of possible match candidates, and match them as full paths against\n> the negative ones to cull \"within the positive set but is excluded\"\n> paths, and the posted patch looks good.\n>\n> I still wonder if we need different implementation when we have many\n> more negative patterns than the positive ones. In such a case, the\n> stage to filter paths that matched one positive pattern by finding\n> matches with a negative pattern among many of them, which may\n> benefit from having a similar common prefix (among negative\n> patterns) optimization, but that is a separate topic.\n>\n> Thanks.\n>\n"}]}