From 7bae578797cf65a71a5f4fa65506d6c5c21db091 Mon Sep 17 00:00:00 2001 From: Tetsuharu Ohzeki Date: Tue, 14 Mar 2023 14:42:02 +0900 Subject: [PATCH] fs: `fs.cp()` should accept `mode` flag to specify the behavior of copying `fs.copyFile()` supports copy-on-write operation if the underlying platform supports it by passing a mode flag. This behavior was added in https://github.com/nodejs/node/commit/a16d88d9e9a6581e463082549823189aba25ca76. This patch adds `mode` flag to `fs.cp()`, `fs.cpSync()`, and `fsPromises.cp()` to allow to change their behaviors to copy files. This test case is based on the test case that was introduced when we add `fs.constants.COPYFILE_FICLONE`. https://github.com/nodejs/node/commit/a16d88d9e9a6581e463082549823189aba25ca76. This test strategy is: - If the platform supports copy-on-write operation, check the success case. - Otherwise, the operation will fail and check whether the failure error information is expected. Fixes: https://github.com/nodejs/node/issues/47080 --- doc/api/fs.md | 3 ++ lib/internal/fs/cp/cp-sync.js | 2 +- lib/internal/fs/cp/cp.js | 2 +- lib/internal/fs/utils.js | 1 + test/parallel/test-fs-cp.mjs | 96 +++++++++++++++++++++++++++++++++++ 5 files changed, 102 insertions(+), 2 deletions(-) diff --git a/doc/api/fs.md b/doc/api/fs.md index 1a48fc072ac450..a30dbdb40c29e9 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -984,6 +984,7 @@ changes: operation will ignore errors if you set this to false and the destination exists. Use the `errorOnExist` option to change this behavior. **Default:** `true`. + * `mode` {integer} modifiers for copy operation. **Default:** `0`. See `mode` flag of [`fsPromises.copyFile()`][] * `preserveTimestamps` {boolean} When `true` timestamps from `src` will be preserved. **Default:** `false`. * `recursive` {boolean} copy directories recursively **Default:** `false` @@ -2309,6 +2310,7 @@ changes: operation will ignore errors if you set this to false and the destination exists. Use the `errorOnExist` option to change this behavior. **Default:** `true`. + * `mode` {integer} modifiers for copy operation. **Default:** `0`. See `mode` flag of [`fs.copyFile()`][] * `preserveTimestamps` {boolean} When `true` timestamps from `src` will be preserved. **Default:** `false`. * `recursive` {boolean} copy directories recursively **Default:** `false` @@ -5200,6 +5202,7 @@ changes: operation will ignore errors if you set this to false and the destination exists. Use the `errorOnExist` option to change this behavior. **Default:** `true`. + * `mode` {integer} modifiers for copy operation. **Default:** `0`. See `mode` flag of [`fs.copyFileSync()`][] * `preserveTimestamps` {boolean} When `true` timestamps from `src` will be preserved. **Default:** `false`. * `recursive` {boolean} copy directories recursively **Default:** `false` diff --git a/lib/internal/fs/cp/cp-sync.js b/lib/internal/fs/cp/cp-sync.js index cfd54a4ff0e23f..348d45adcd7c4e 100644 --- a/lib/internal/fs/cp/cp-sync.js +++ b/lib/internal/fs/cp/cp-sync.js @@ -226,7 +226,7 @@ function mayCopyFile(srcStat, src, dest, opts) { } function copyFile(srcStat, src, dest, opts) { - copyFileSync(src, dest); + copyFileSync(src, dest, opts.mode); if (opts.preserveTimestamps) handleTimestamps(srcStat.mode, src, dest); return setDestMode(dest, srcStat.mode); } diff --git a/lib/internal/fs/cp/cp.js b/lib/internal/fs/cp/cp.js index cba405223a4fc7..130bf98ae7be4c 100644 --- a/lib/internal/fs/cp/cp.js +++ b/lib/internal/fs/cp/cp.js @@ -257,7 +257,7 @@ async function mayCopyFile(srcStat, src, dest, opts) { } async function _copyFile(srcStat, src, dest, opts) { - await copyFile(src, dest); + await copyFile(src, dest, opts.mode); if (opts.preserveTimestamps) { return handleTimestampsAndMode(srcStat.mode, src, dest); } diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index b32527f9cdf779..c4f4785b339c39 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -787,6 +787,7 @@ const validateCpOptions = hideStackFrames((options) => { validateBoolean(options.preserveTimestamps, 'options.preserveTimestamps'); validateBoolean(options.recursive, 'options.recursive'); validateBoolean(options.verbatimSymlinks, 'options.verbatimSymlinks'); + options.mode = getValidMode(options.mode, 'copyFile'); if (options.dereference === true && options.verbatimSymlinks === true) { throw new ERR_INCOMPATIBLE_OPTION_PAIR('dereference', 'verbatimSymlinks'); } diff --git a/test/parallel/test-fs-cp.mjs b/test/parallel/test-fs-cp.mjs index c4d6c4b737e371..af1a021d3a24c7 100644 --- a/test/parallel/test-fs-cp.mjs +++ b/test/parallel/test-fs-cp.mjs @@ -38,6 +38,28 @@ function nextdir() { assertDirEquivalent(src, dest); } +// It copies a nested folder structure with mode flags. +// This test is based on fs.promises.copyFile() with `COPYFILE_FICLONE_FORCE`. +{ + const src = './test/fixtures/copy/kitchen-sink'; + const dest = nextdir(); + try { + cpSync(src, dest, mustNotMutateObjectDeep({ + recursive: true, + mode: fs.constants.COPYFILE_FICLONE_FORCE, + })); + assertDirEquivalent(src, dest); + // If the platform support `COPYFILE_FICLONE_FORCE` operation, + // it should reach to here. + } catch (err) { + // If the platform does not support `COPYFILE_FICLONE_FORCE` operation, + // it should enter this path. + assert.strictEqual(err.syscall, 'copyfile'); + assert(err.code === 'ENOTSUP' || err.code === 'ENOTTY' || + err.code === 'ENOSYS' || err.code === 'EXDEV'); + } +} + // It does not throw errors when directory is copied over and force is false. { const src = nextdir(); @@ -107,6 +129,14 @@ function nextdir() { }); } +// It rejects if options.mode is invalid. +{ + assert.throws( + () => cpSync('a', 'b', { mode: -1 }), + { code: 'ERR_OUT_OF_RANGE' } + ); +} + // It throws an error when both dereference and verbatimSymlinks are enabled. { @@ -425,6 +455,31 @@ if (!isWindows) { })); } +// It copies a nested folder structure with mode flags. +// This test is based on fs.promises.copyFile() with `COPYFILE_FICLONE_FORCE`. +{ + const src = './test/fixtures/copy/kitchen-sink'; + const dest = nextdir(); + cp(src, dest, mustNotMutateObjectDeep({ + recursive: true, + mode: fs.constants.COPYFILE_FICLONE_FORCE, + }), mustCall((err) => { + if (!err) { + // If the platform support `COPYFILE_FICLONE_FORCE` operation, + // it should reach to here. + assert.strictEqual(err, null); + assertDirEquivalent(src, dest); + return; + } + + // If the platform does not support `COPYFILE_FICLONE_FORCE` operation, + // it should enter this path. + assert.strictEqual(err.syscall, 'copyfile'); + assert(err.code === 'ENOTSUP' || err.code === 'ENOTTY' || + err.code === 'ENOSYS' || err.code === 'EXDEV'); + })); +} + // It does not throw errors when directory is copied over and force is false. { const src = nextdir(); @@ -799,6 +854,14 @@ if (!isWindows) { ); } +// It throws if options is not object. +{ + assert.throws( + () => cp('a', 'b', { mode: -1 }, () => {}), + { code: 'ERR_OUT_OF_RANGE' } + ); +} + // Promises implementation of copy. // It copies a nested folder structure with files and folders. @@ -810,6 +873,29 @@ if (!isWindows) { assertDirEquivalent(src, dest); } +// It copies a nested folder structure with mode flags. +// This test is based on fs.promises.copyFile() with `COPYFILE_FICLONE_FORCE`. +{ + const src = './test/fixtures/copy/kitchen-sink'; + const dest = nextdir(); + try { + const p = await fs.promises.cp(src, dest, mustNotMutateObjectDeep({ + recursive: true, + mode: fs.constants.COPYFILE_FICLONE_FORCE, + })); + assert.strictEqual(p, undefined); + assertDirEquivalent(src, dest); + // If the platform support `COPYFILE_FICLONE_FORCE` operation, + // it should reach to here. + } catch (err) { + // If the platform does not support `COPYFILE_FICLONE_FORCE` operation, + // it should enter this path. + assert.strictEqual(err.syscall, 'copyfile'); + assert(err.code === 'ENOTSUP' || err.code === 'ENOTTY' || + err.code === 'ENOSYS' || err.code === 'EXDEV'); + } +} + // It accepts file URL as src and dest. { const src = './test/fixtures/copy/kitchen-sink'; @@ -847,6 +933,16 @@ if (!isWindows) { ); } +// It rejects if options.mode is invalid. +{ + await assert.rejects( + fs.promises.cp('a', 'b', { + mode: -1, + }), + { code: 'ERR_OUT_OF_RANGE' } + ); +} + function assertDirEquivalent(dir1, dir2) { const dir1Entries = []; collectEntries(dir1, dir1Entries);