From 7a3b027cec7947a9f961976a97871e1fce7e2554 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Tue, 11 Mar 2025 20:13:28 -0700 Subject: [PATCH 1/4] fs: apply exclude function to root path In at least some situations, the root path was being added to the results of globbing even if it matched the optional exclude function. In the relevant test file, a variable was renamed for consistency. (There was a patterns and pattern2, rather than patterns2.) The test case is added to patterns2. Fixes: https://github.com/nodejs/node/issues/56260 --- lib/internal/fs/glob.js | 12 ++++++++++++ test/parallel/test-fs-glob.mjs | 9 +++++---- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/lib/internal/fs/glob.js b/lib/internal/fs/glob.js index ba6066d80d8558..aa88ec3dcc491a 100644 --- a/lib/internal/fs/glob.js +++ b/lib/internal/fs/glob.js @@ -326,6 +326,18 @@ class Glob { if (this.#isExcluded(path)) { return; } + if (this.#exclude) { + if (this.#withFileTypes) { + const stat = this.#cache.statSync(path); + if (stat !== null) { + if (this.#exclude(stat)) { + return; + } + } + } else if (this.#exclude(path)) { + return; + } + } if (!this.#subpatterns.has(path)) { this.#subpatterns.set(path, [pattern]); } else { diff --git a/test/parallel/test-fs-glob.mjs b/test/parallel/test-fs-glob.mjs index 09cc8791bcc34f..b000d1bd359e28 100644 --- a/test/parallel/test-fs-glob.mjs +++ b/test/parallel/test-fs-glob.mjs @@ -388,7 +388,7 @@ describe('fsPromises glob - withFileTypes', function() { }); // [pattern, exclude option, expected result] -const pattern2 = [ +const patterns2 = [ ['a/{b,c}*', ['a/*c'], ['a/b', 'a/cb']], ['a/{a,b,c}*', ['a/*bc*', 'a/cb'], ['a/b', 'a/c']], ['a/**/[cg]', ['**/c'], ['a/abcdef/g', 'a/abcfed/g']], @@ -427,6 +427,7 @@ const pattern2 = [ [`${absDir}/*{a,q}*`, './a/*{c,b}*/*'], [`${absDir}/foo`, 'a/c', ...(common.isWindows ? [] : ['a/symlink/a/b/c'])], ], + [ 'a/**', () => true, [] ], ]; describe('globSync - exclude', function() { @@ -436,7 +437,7 @@ describe('globSync - exclude', function() { assert.strictEqual(actual.length, 0); }); } - for (const [pattern, exclude, expected] of pattern2) { + for (const [pattern, exclude, expected] of patterns2) { test(`${pattern} - exclude: ${exclude}`, () => { const actual = globSync(pattern, { cwd: fixtureDir, exclude }).sort(); const normalized = expected.filter(Boolean).map((item) => item.replaceAll('/', sep)).sort(); @@ -453,7 +454,7 @@ describe('glob - exclude', function() { assert.strictEqual(actual.length, 0); }); } - for (const [pattern, exclude, expected] of pattern2) { + for (const [pattern, exclude, expected] of patterns2) { test(`${pattern} - exclude: ${exclude}`, async () => { const actual = (await promisified(pattern, { cwd: fixtureDir, exclude })).sort(); const normalized = expected.filter(Boolean).map((item) => item.replaceAll('/', sep)).sort(); @@ -471,7 +472,7 @@ describe('fsPromises glob - exclude', function() { assert.strictEqual(actual.length, 0); }); } - for (const [pattern, exclude, expected] of pattern2) { + for (const [pattern, exclude, expected] of patterns2) { test(`${pattern} - exclude: ${exclude}`, async () => { const actual = []; for await (const item of asyncGlob(pattern, { cwd: fixtureDir, exclude })) actual.push(item); From 91d2d004d1fbafc9e14bb3285b1b9685944ad688 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Thu, 13 Mar 2025 12:13:21 -0700 Subject: [PATCH 2/4] fixup! fs: apply exclude function to root path --- lib/internal/fs/glob.js | 13 ++++++++++++- test/parallel/test-fs-glob.mjs | 3 +++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/lib/internal/fs/glob.js b/lib/internal/fs/glob.js index aa88ec3dcc491a..054177894f6106 100644 --- a/lib/internal/fs/glob.js +++ b/lib/internal/fs/glob.js @@ -125,7 +125,8 @@ class Cache { } statSync(path) { const cached = this.#statsCache.get(path); - if (cached) { + // Do not return a promise from a sync function. + if (cached && !(cached instanceof Promise)) { return cached; } const val = getDirentSync(path); @@ -326,6 +327,16 @@ class Glob { if (this.#isExcluded(path)) { return; } + const fullpath = resolve(this.#root, path); + + // If path is a directory, add trailing slash and test patterns again. + // TODO: Would running #isExcluded() first and checking isDirectory() only + // if it matches be more performant in the typical use case? #isExcluded() + // is often ()=>false which is about as optimizable as a function gets. + if (this.#cache.statSync(fullpath).isDirectory() && this.#isExcluded(`${fullpath}/`)) { + return; + } + if (this.#exclude) { if (this.#withFileTypes) { const stat = this.#cache.statSync(path); diff --git a/test/parallel/test-fs-glob.mjs b/test/parallel/test-fs-glob.mjs index b000d1bd359e28..077c05279915b5 100644 --- a/test/parallel/test-fs-glob.mjs +++ b/test/parallel/test-fs-glob.mjs @@ -428,6 +428,9 @@ const patterns2 = [ [`${absDir}/foo`, 'a/c', ...(common.isWindows ? [] : ['a/symlink/a/b/c'])], ], [ 'a/**', () => true, [] ], + [ 'a/**', [ '*' ], [] ], + [ 'a/**', [ '**' ], [] ], + [ 'a/**', [ 'a/**' ], [] ], ]; describe('globSync - exclude', function() { From 7bbd7e4ec95419649222138c873037cf82f3c067 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Thu, 13 Mar 2025 12:17:42 -0700 Subject: [PATCH 3/4] fixup! fixup! fs: apply exclude function to root path --- lib/internal/fs/glob.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/internal/fs/glob.js b/lib/internal/fs/glob.js index 054177894f6106..b2fc5bdc039ed9 100644 --- a/lib/internal/fs/glob.js +++ b/lib/internal/fs/glob.js @@ -9,6 +9,7 @@ const { ArrayPrototypePop, ArrayPrototypePush, ArrayPrototypeSome, + Promise, PromisePrototypeThen, SafeMap, SafeSet, From 53d6b3111d838de7316fecc6514905aa2a0d3b5e Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Fri, 14 Mar 2025 13:23:56 -0700 Subject: [PATCH 4/4] Update lib/internal/fs/glob.js Co-authored-by: Livia Medeiros --- lib/internal/fs/glob.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/fs/glob.js b/lib/internal/fs/glob.js index b2fc5bdc039ed9..a20ca9d2b26fb5 100644 --- a/lib/internal/fs/glob.js +++ b/lib/internal/fs/glob.js @@ -331,7 +331,7 @@ class Glob { const fullpath = resolve(this.#root, path); // If path is a directory, add trailing slash and test patterns again. - // TODO: Would running #isExcluded() first and checking isDirectory() only + // TODO(Trott): Would running #isExcluded() first and checking isDirectory() only // if it matches be more performant in the typical use case? #isExcluded() // is often ()=>false which is about as optimizable as a function gets. if (this.#cache.statSync(fullpath).isDirectory() && this.#isExcluded(`${fullpath}/`)) {