fs: fix opts.filter issue in cpSync

PR-URL: #45143 Fixes: #44720 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
nodejs · Jan 3, 2023 · 1db20c8 · 1db20c8
1 parent 345b847
commit 1db20c8
Show file tree

Hide file tree

Showing 2 changed files with 27 additions and 28 deletions.
diff --git a/lib/internal/fs/cp/cp-sync.js b/lib/internal/fs/cp/cp-sync.js
@@ -55,12 +55,20 @@ function cpSyncFn(src, dest, opts) {
       'node is not recommended';
     process.emitWarning(warning, 'TimestampPrecisionWarning');
   }
-  const { srcStat, destStat } = checkPathsSync(src, dest, opts);
+  const { srcStat, destStat, skipped } = checkPathsSync(src, dest, opts);
+  if (skipped) return;
   checkParentPathsSync(src, srcStat, dest);
-  return handleFilterAndCopy(destStat, src, dest, opts);
+  return checkParentDir(destStat, src, dest, opts);
 }
 
 function checkPathsSync(src, dest, opts) {
+  if (opts.filter) {
+    const shouldCopy = opts.filter(src, dest);
+    if (isPromise(shouldCopy)) {
+      throw new ERR_INVALID_RETURN_VALUE('boolean', 'filter', shouldCopy);
+    }
+    if (!shouldCopy) return { __proto__: null, skipped: true };
+  }
   const { srcStat, destStat } = getStatsSync(src, dest, opts);
 
   if (destStat) {
@@ -104,7 +112,7 @@ function checkPathsSync(src, dest, opts) {
       code: 'EINVAL',
     });
   }
-  return { srcStat, destStat };
+  return { __proto__: null, srcStat, destStat, skipped: false };
 }
 
 function getStatsSync(src, dest, opts) {
@@ -145,24 +153,12 @@ function checkParentPathsSync(src, srcStat, dest) {
   return checkParentPathsSync(src, srcStat, destParent);
 }
 
-function handleFilterAndCopy(destStat, src, dest, opts) {
-  if (opts.filter) {
-    const shouldCopy = opts.filter(src, dest);
-    if (isPromise(shouldCopy)) {
-      throw new ERR_INVALID_RETURN_VALUE('boolean', 'filter', shouldCopy);
-    }
-    if (!shouldCopy) return;
-  }
+function checkParentDir(destStat, src, dest, opts) {
   const destParent = dirname(dest);
   if (!existsSync(destParent)) mkdirSync(destParent, { recursive: true });
   return getStats(destStat, src, dest, opts);
 }
 
-function startCopy(destStat, src, dest, opts) {
-  if (opts.filter && !opts.filter(src, dest)) return;
-  return getStats(destStat, src, dest, opts);
-}
-
 function getStats(destStat, src, dest, opts) {
   const statSyncFn = opts.dereference ? statSync : lstatSync;
   const srcStat = statSyncFn(src);
@@ -284,9 +280,8 @@ function copyDir(src, dest, opts) {
       const { name } = dirent;
       const srcItem = join(src, name);
       const destItem = join(dest, name);
-      const { destStat } = checkPathsSync(srcItem, destItem, opts);
-
-      startCopy(destStat, srcItem, destItem, opts);
+      const { destStat, skipped } = checkPathsSync(srcItem, destItem, opts);
+      if (!skipped) getStats(destStat, srcItem, destItem, opts);
     }
   } finally {
     dir.closeSync();

diff --git a/test/parallel/test-fs-cp.mjs b/test/parallel/test-fs-cp.mjs
@@ -746,24 +746,26 @@ if (!isWindows) {
      }));
 }
 
-// Copy async should not throw exception if child folder is filtered out.
+// Copy should not throw exception if child folder is filtered out.
 {
   const src = nextdir();
-  mkdirSync(join(src, 'test-cp-async'), mustNotMutateObjectDeep({ recursive: true }));
+  mkdirSync(join(src, 'test-cp'), mustNotMutateObjectDeep({ recursive: true }));
 
   const dest = nextdir();
   mkdirSync(dest, mustNotMutateObjectDeep({ recursive: true }));
-  writeFileSync(join(dest, 'test-cp-async'), 'test-content', mustNotMutateObjectDeep({ mode: 0o444 }));
+  writeFileSync(join(dest, 'test-cp'), 'test-content', mustNotMutateObjectDeep({ mode: 0o444 }));
 
-  cp(src, dest, {
-    filter: (path) => !path.includes('test-cp-async'),
+  const opts = {
+    filter: (path) => !path.includes('test-cp'),
     recursive: true,
-  }, mustCall((err) => {
+  };
+  cp(src, dest, opts, mustCall((err) => {
     assert.strictEqual(err, null);
   }));
+  cpSync(src, dest, opts);
 }
 
-// Copy async should not throw exception if dest is invalid but filtered out.
+// Copy should not throw exception if dest is invalid but filtered out.
 {
   // Create dest as a file.
   // Expect: cp skips the copy logic entirely and won't throw any exception in path validation process.
@@ -775,12 +777,14 @@ if (!isWindows) {
   mkdirSync(destParent, mustNotMutateObjectDeep({ recursive: true }));
   writeFileSync(dest, 'test-content', mustNotMutateObjectDeep({ mode: 0o444 }));
 
-  cp(src, dest, {
+  const opts = {
     filter: (path) => !path.includes('bar'),
     recursive: true,
-  }, mustCall((err) => {
+  };
+  cp(src, dest, opts, mustCall((err) => {
     assert.strictEqual(err, null);
   }));
+  cpSync(src, dest, opts);
 }
 
 // It throws if options is not object.