Skip to content

fs,path: fix glob globstar section bounds - #66738

Open
gurgunday wants to merge 1 commit into
nodejs:mainfrom
gurgunday:fix-glob-globstar-bound
Open

gurgunday wants to merge 1 commit into
nodejs:mainfrom
gurgunday:fix-glob-globstar-bound

Conversation

@gurgunday

Copy link
Copy Markdown
Member

The last start index of each globstar-delimited body section was computed from the non-globstar prefix counts in reverse order. When sections have different lengths, the bound is too tight, so paths such as a/b/c/d/e failed to match **/a/b/c/**/d/**. This affected path.matchesGlob() and exclude patterns in fs.glob()

Compute the bound from the number of non-globstar parts in the section and every section after it

FYI minimatch has the bug too

The last start index of each globstar-delimited body section was
computed from the non-globstar prefix counts in reverse order. When
sections have different lengths, the bound is too tight, so paths
such as `a/b/c/d/e` failed to match `**/a/b/c/**/d/**`. This affected
`path.matchesGlob()` and `exclude` patterns in `fs.glob()`.

Compute the bound from the number of non-globstar parts in the
section and every section after it.

Signed-off-by: Gürgün Dayıoğlu <hey@gurgun.day>
@gurgunday
gurgunday requested a review from avivkeller October 14, 2026 18:54
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/path

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Oct 14, 2026
@avivkeller

Copy link
Copy Markdown
Member

FYI minimatch has the bug too

Is this a breaking change, since the previous implementation suffered the same issue? Or just a bug fix

@gurgunday

gurgunday commented Oct 14, 2026 •

Copy link
Copy Markdown
Member Author

I don't have a strong opinion, but this can't be backported anyway right? I think no active release has the C++ port? And previous releases (24, 26) have the bug from minimatch directly

@avivkeller avivkeller left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@codecov

codecov Bot commented Oct 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.49%. Comparing base (dd7235c) to head (0a97eb6).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/glob/glob_matcher.cc 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #66738   +/-   ##
=======================================
  Coverage   90.48%   90.49%           
=======================================
  Files         791      791           
  Lines      277204   277225   +21     
  Branches    53311    53315    +4     
=======================================
+ Hits       250841   250861   +20     
+ Misses      16760    16753    -7     
- Partials     9603     9611    +8     
Files with missing lines Coverage Δ
src/glob/glob_matcher.cc 66.80% <66.66%> (ø)

... and 28 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants