From 88d3721c79ce8a6fdfa47f5ae02699b988870993 Mon Sep 17 00:00:00 2001 From: Yuesong Jake Li Date: Wed, 28 May 2025 09:27:34 +0930 Subject: [PATCH 1/3] Revert "benchmark: fix broken fs.cpSync benchmark" This reverts commit 52430b9839c31f846a788125efda82887d115e21. --- benchmark/fs/bench-cpSync.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/benchmark/fs/bench-cpSync.js b/benchmark/fs/bench-cpSync.js index 822036726d198d..fa6350384c69dd 100644 --- a/benchmark/fs/bench-cpSync.js +++ b/benchmark/fs/bench-cpSync.js @@ -8,10 +8,7 @@ const tmpdir = require('../../test/common/tmpdir'); const bench = common.createBenchmark(main, { n: [1, 100, 10_000], dereference: ['true', 'false'], - // When `force` is `true` the `cpSync` function is called twice the second - // time however an `ERR_FS_CP_EINVAL` is thrown, so skip `true` for the time being - // TODO: allow `force` to also be `true` once https://github.com/nodejs/node/issues/58468 is addressed - force: ['false'], + force: ['true', 'false'], }); function prepareTestDirectory() { From ea7989143a0561a076cc819e7ed765eabbd05d6b Mon Sep 17 00:00:00 2001 From: Yuesong Jake Li Date: Tue, 27 May 2025 12:16:11 +0930 Subject: [PATCH 2/3] fs: fix cpSync handle existing symlinks --- lib/internal/fs/cp/cp-sync.js | 4 +++- test/parallel/test-fs-cp.mjs | 13 ++++++++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/lib/internal/fs/cp/cp-sync.js b/lib/internal/fs/cp/cp-sync.js index 9e67ae6335ec46..1b562b7d239079 100644 --- a/lib/internal/fs/cp/cp-sync.js +++ b/lib/internal/fs/cp/cp-sync.js @@ -196,7 +196,9 @@ function onLink(destStat, src, dest, verbatimSymlinks) { if (!isAbsolute(resolvedDest)) { resolvedDest = resolve(dirname(dest), resolvedDest); } - if (isSrcSubdir(resolvedSrc, resolvedDest)) { + const srcIsDir = fsBinding.internalModuleStat(src) === 1; + + if (srcIsDir && isSrcSubdir(resolvedSrc, resolvedDest)) { throw new ERR_FS_CP_EINVAL({ message: `cannot copy ${resolvedSrc} to a subdirectory of self ` + `${resolvedDest}`, diff --git a/test/parallel/test-fs-cp.mjs b/test/parallel/test-fs-cp.mjs index 260a1449d1a953..dbdb0b7b19e54d 100644 --- a/test/parallel/test-fs-cp.mjs +++ b/test/parallel/test-fs-cp.mjs @@ -15,7 +15,7 @@ const { writeFileSync, } = fs; import net from 'net'; -import { join } from 'path'; +import { join, resolve } from 'path'; import { pathToFileURL } from 'url'; import { setTimeout } from 'timers/promises'; @@ -248,6 +248,17 @@ function nextdir(dirname) { ); } +// It allows copying when is not a directory +{ + const src = nextdir(); + const dest = nextdir(); + mkdirSync(src, mustNotMutateObjectDeep({ recursive: true })); + writeFileSync(`${src}/test.txt`, 'test'); + symlinkSync(resolve(`${src}/test.txt`), join(src, 'link.txt')); + cpSync(src, dest, mustNotMutateObjectDeep({ recursive: true })); + cpSync(src, dest, mustNotMutateObjectDeep({ recursive: true })); +} + // It throws error if symlink in dest points to location in src. { const src = nextdir(); From cde63b281640f8a436ea9023883d48a1d8010b4b Mon Sep 17 00:00:00 2001 From: Yuesong Jake Li Date: Wed, 28 May 2025 09:26:50 +0930 Subject: [PATCH 3/3] fs: fix cp handle existing symlinks --- lib/internal/fs/cp/cp.js | 6 +++++- test/parallel/test-fs-cp.mjs | 19 ++++++++++++++++++- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/lib/internal/fs/cp/cp.js b/lib/internal/fs/cp/cp.js index 0724eb730db058..b632c650681e77 100644 --- a/lib/internal/fs/cp/cp.js +++ b/lib/internal/fs/cp/cp.js @@ -54,6 +54,7 @@ const { resolve, sep, } = require('path'); +const fsBinding = internalBinding('fs'); async function cpFn(src, dest, opts) { // Warn about using preserveTimestamps on 32-bit node @@ -344,7 +345,10 @@ async function onLink(destStat, src, dest, opts) { if (!isAbsolute(resolvedDest)) { resolvedDest = resolve(dirname(dest), resolvedDest); } - if (isSrcSubdir(resolvedSrc, resolvedDest)) { + + const srcIsDir = fsBinding.internalModuleStat(src) === 1; + + if (srcIsDir && isSrcSubdir(resolvedSrc, resolvedDest)) { throw new ERR_FS_CP_EINVAL({ message: `cannot copy ${resolvedSrc} to a subdirectory of self ` + `${resolvedDest}`, diff --git a/test/parallel/test-fs-cp.mjs b/test/parallel/test-fs-cp.mjs index dbdb0b7b19e54d..7a89d0d859bc6c 100644 --- a/test/parallel/test-fs-cp.mjs +++ b/test/parallel/test-fs-cp.mjs @@ -248,7 +248,7 @@ function nextdir(dirname) { ); } -// It allows copying when is not a directory +// It allows cpSync copying symlinks in src to locations in dest with existing synlinks not pointing to a directory. { const src = nextdir(); const dest = nextdir(); @@ -259,6 +259,23 @@ function nextdir(dirname) { cpSync(src, dest, mustNotMutateObjectDeep({ recursive: true })); } +// It allows cp copying symlinks in src to locations in dest with existing synlinks not pointing to a directory. +{ + const src = nextdir(); + const dest = nextdir(); + mkdirSync(src, mustNotMutateObjectDeep({ recursive: true })); + writeFileSync(`${src}/test.txt`, 'test'); + symlinkSync(resolve(`${src}/test.txt`), join(src, 'link.txt')); + cp(src, dest, { recursive: true }, + mustCall((err) => { + assert.strictEqual(err, null); + + cp(src, dest, { recursive: true }, mustCall((err) => { + assert.strictEqual(err, null); + })); + })); +} + // It throws error if symlink in dest points to location in src. { const src = nextdir();