Skip to content

Keep trailing globstars from matching their parent directory - #138

Merged
cpburnz merged 3 commits into
cpburnz:masterfrom
jackwalkerlabs:fix/trailing-globstar-directory
Sep 10, 2026
Merged

Keep trailing globstars from matching their parent directory#138
cpburnz merged 3 commits into
cpburnz:masterfrom
jackwalkerlabs:fix/trailing-globstar-directory

Conversation

@jackwalkerlabs

Copy link
Copy Markdown
Contributor

Addresses part A of #137, reported and reduced by @KaizenShogun. d/** currently matches the bare directory query d/, so a caller pruning a walk can skip children that a later negation would re-include. Require a nonempty child segment after the trailing globstar's slash. Keep explicit directory patterns such as d/ unchanged, and cover child names beginning with a newline.

The regression failed on the base. Validation: 213 tests and 591 subtests pass with simple, RE2, and Hyperscan backends on CPython 3.12.14. A real Git repository canary confirms d/** plus !d/canary permits staging and traversal, while d/ plus the same negation excludes it. Updated affected regex expectations and added a changelog entry.

This does not address part B of #137. Implemented and locally validated with OpenAI Codex.

@KaizenShogun

Copy link
Copy Markdown
Contributor

Ran this through the bench from #137, since that was the offer I made there. Everything below is base 5f14318 against 5e0786e, the two trees side by side, GitIgnoreSpec on the simple, RE2 and Hyperscan backends, CPython 3.14.7, git 2.55.0 as the oracle.

Corpus — 9,852 (.gitignore, path) cases from 43 real repositories:

backend base this PR
simple 6 1
re2 6 1
hyperscan 6 1

Compared as sets rather than counts: five fixed, zero new, on all three. The survivor is nodejs/node :: vendor/deps/npm/node_modules/.bin/x.txt — part B, which you say you don't touch. So the split in #137 holds up from the other direction: the five really were one bug.

Narrowing a regex is the classic way to turn a false positive into a false negative, so I went looking for that specifically. 20 pattern shapes with a trailing globstar × 18 query paths, each verdict taken from a real repository two independent ways — check-ignore -v on the bare path, plus the re-inclusion canary probe — and dropped where the two disagreed (56 of 360). That leaves 304 cases × 3 backends:

  • base: 26 (pattern, path) pairs disagree with git
  • this PR: 8
  • fixed 18, new 0

I couldn't produce a false negative.

The 8 survivors are three older shapes. All reproduce identically on 93e0179 — before #132 — so none of them is part B either:

d/**/                d/top.txt      git stages it; pathspec says ignored
d/** + !d/k          d/k/
d/** + !d/sub/k      d/sub/k

The first is next door to your change: d/**/ takes the is_dir_pattern arm three lines up and still collapses to ^d(?P<ps_d>/), which swallows d itself and every file directly inside it. Same shape of bug, other branch. I'd keep it out of this PR — 0 of the 2,691 patterns across those 43 real .gitignore files end in /**/, so it's a footnote, not a blocker.

The tree API is untouched. I materialised all 43 corpus repos as real git trees and diffed match_tree_files(negate=True) before and after: identical in every repo. That's the expected answer, and worth stating, because it's the API most callers actually walk with — the fix lands on match_file, which was the one that was wrong.

Two smaller confirmations. The suite runs 213 tests OK here, and 14 failures against base, so the regression does fail on the base as you describe. And [^/] is the better choice over the /. I floated in #137: . won't match a child whose name begins with a newline, and yours does, on all three backends. My probe was the weaker one.

One correction to my own #137 while I'm here. I suggested part A "may be a documentation matter rather than a code one; your call." On this evidence I withdraw that. The code fix costs zero measurable regressions and moves the bench from 6 to 1; sending callers to a different API would have cost them more.

— Midas

@cpburnz
cpburnz merged commit 06bb47c into cpburnz:master Sep 10, 2026
6 of 64 checks passed
@cpburnz

cpburnz commented Sep 10, 2026

Copy link
Copy Markdown
Owner

@jackwalkerlabs Thanks for fixing #137 part A.

@cpburnz

cpburnz commented Sep 10, 2026

Copy link
Copy Markdown
Owner

@KaizenShogun Thanks for confirming the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants