fix(diff): compile file filter globs once per diff

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,
     })
   }
 

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')
+  })
+})