From a24d56faa8bdd40ab56f6491e103151c12d90f30 Mon Sep 17 00:00:00 2001 From: Oscar Busk Date: Fri, 25 Sep 2026 00:01:49 +0200 Subject: [PATCH] fix(diff): compile file filter globs once per diff Previously every tarball entry called `minimatch()` once per filter pattern, and minimatch does not cache compiled patterns, so each glob was re-parsed and turned into a regex (files x patterns x 2 tarballs) times. Compile each filter into a `Minimatch` instance once in `readTarballs` and hand `untar` a single `isIncluded(path)` predicate instead. Matching behavior is unchanged. Co-Authored-By: Claude Opus 5.5 --- workspaces/libnpmdiff/lib/untar.js | 56 +++++++++++++------------ workspaces/libnpmdiff/test/untar.js | 64 +++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 26 deletions(-) diff --git a/workspaces/libnpmdiff/lib/untar.js b/workspaces/libnpmdiff/lib/untar.js index 6bbecd8a59ce0..fed5df8f2d020 100644 --- a/workspaces/libnpmdiff/lib/untar.js +++ b/workspaces/libnpmdiff/lib/untar.js @@ -1,36 +1,40 @@ const tar = require('tar') -const { minimatch } = require('minimatch') +const { Minimatch } = require('minimatch') const normalizeMatch = str => str .replace(/\\+/g, '/') .replace(/^\.\/|^\./, '') -// files and refs are mutating params -// filterFiles, item, prefix and opts are read-only options -const untar = ({ files, refs }, { filterFiles, item, prefix }) => { - tar.list({ - filter: (path, entry) => { - const fileMatch = () => - (!filterFiles.length || - filterFiles.some(f => { - const pattern = normalizeMatch(f) - return minimatch( - normalizeMatch(path), - `{package/,}${pattern}`, - { matchBase: pattern.startsWith('*') } - ) - })) +// returns a predicate telling whether a tarball path is included by the +// user-provided filters, compiling each glob only once +const buildFilter = (diffFiles) => { + if (!diffFiles.length) { + return () => true + } + + const filters = diffFiles.map(normalizeMatch) + const matchers = filters.map(pattern => + new Minimatch( + `{package/,}${pattern}`, + { matchBase: pattern.startsWith('*') } + )) + return path => { + const normalizedPath = normalizeMatch(path) + return matchers.some(m => m.match(normalizedPath)) || // expands usage of simple path filters, e.g: lib or src/ - const folderMatch = () => - filterFiles.some(f => - normalizeMatch(path).startsWith(normalizeMatch(f)) || - normalizeMatch(path).startsWith(`package/${normalizeMatch(f)}`)) + filters.some(f => + normalizedPath.startsWith(f) || + normalizedPath.startsWith(`package/${f}`)) + } +} - if ( - entry.type === 'File' && - (fileMatch() || folderMatch()) - ) { +// files and refs are mutating params +// isIncluded, item and prefix are read-only options +const untar = ({ files, refs }, { isIncluded, item, prefix }) => { + tar.list({ + filter: (path, entry) => { + if (entry.type === 'File' && isIncluded(path)) { const key = path.replace(/^[^/]+\/?/, '') files.add(key) @@ -65,7 +69,7 @@ const readTarballs = async (tarballs, opts = {}) => { const refs = new Map() const arr = [].concat(tarballs) - const filterFiles = opts.diffFiles || [] + const isIncluded = buildFilter(opts.diffFiles || []) for (const i of arr) { untar({ @@ -74,7 +78,7 @@ const readTarballs = async (tarballs, opts = {}) => { }, { item: i.item, prefix: i.prefix, - filterFiles, + isIncluded, }) } diff --git a/workspaces/libnpmdiff/test/untar.js b/workspaces/libnpmdiff/test/untar.js index 19e55b7d006fb..a7a5248370ba4 100644 --- a/workspaces/libnpmdiff/test/untar.js +++ b/workspaces/libnpmdiff/test/untar.js @@ -229,3 +229,67 @@ t.test('filter out all files', async t => { t.equal(files.size, 0, 'should have no files') t.equal(refs.size, 0, 'should have no refs') }) + +t.test('compiles each glob pattern only once', async t => { + const realMinimatch = require('minimatch') + const mockUntar = () => { + const compiled = [] + const untarWithSpy = t.mock('../lib/untar.js', { + minimatch: { + ...realMinimatch, + Minimatch: function (pattern, opts) { + compiled.push(pattern) + return new realMinimatch.Minimatch(pattern, opts) + }, + }, + }) + return { compiled, untar: untarWithSpy } + } + const tarballs = async () => [ + { item: await pacote.tarball(resolve('./test/fixtures/archive.tgz')), prefix: 'a/' }, + { item: await pacote.tarball(resolve('./test/fixtures/archive.tgz')), prefix: 'b/' }, + ] + + t.test('with filters', async t => { + const { compiled, untar: untarWithSpy } = mockUntar() + const { files, refs } = await untarWithSpy(await tarballs(), { + diffFiles: ['*.json', 'lib/**/*.js'], + }) + + t.same(compiled, [ + '{package/,}*.json', + '{package/,}lib/**/*.js', + ], 'should compile each pattern once across all tarballs and files') + t.same([...files], [ + 'lib/index.js', + 'lib/utils/b.js', + 'package-lock.json', + 'package.json', + ], 'should return list of matched filenames') + t.same([...refs.keys()], [ + 'a/lib/index.js', + 'a/lib/utils/b.js', + 'a/package-lock.json', + 'a/package.json', + 'b/lib/index.js', + 'b/lib/utils/b.js', + 'b/package-lock.json', + 'b/package.json', + ], 'should read matched files from both tarballs') + }) + + t.test('without filters', async t => { + const { compiled, untar: untarWithSpy } = mockUntar() + const { files } = await untarWithSpy(await tarballs()) + + t.same(compiled, [], 'should not compile any pattern') + t.same([...files], [ + 'lib/index.js', + 'lib/utils/b.js', + 'package-lock.json', + 'package.json', + 'test/index.js', + 'test/utils/b.js', + ], 'should return all filenames') + }) +})