Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: fs.cp() should accept mode flag to specify the copy behavior #47084

Merged
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions doc/api/fs.md
Original file line number Diff line number Diff line change
@@ -966,6 +966,10 @@ try {
<!-- YAML
added: v16.7.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/47084
description: Accept an additional `mode` option to specify
the copy behavior as the `mode` argument of `fs.copyFile()`.
- version:
- v17.6.0
- v16.15.0
@@ -992,6 +996,8 @@ 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`
@@ -2286,6 +2292,10 @@ copyFile('source.txt', 'destination.txt', constants.COPYFILE_EXCL, callback);
<!-- YAML
added: v16.7.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/47084
description: Accept an additional `mode` option to specify
the copy behavior as the `mode` argument of `fs.copyFile()`.
- version: v18.0.0
pr-url: https://github.com/nodejs/node/pull/41678
description: Passing an invalid callback to the `callback` argument
@@ -2317,6 +2327,8 @@ 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`
@@ -5191,6 +5203,10 @@ copyFileSync('source.txt', 'destination.txt', constants.COPYFILE_EXCL);
<!-- YAML
added: v16.7.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/47084
description: Accept an additional `mode` option to specify
the copy behavior as the `mode` argument of `fs.copyFile()`.
- version:
- v17.6.0
- v16.15.0
@@ -5216,6 +5232,8 @@ 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`
@@ -8004,6 +8022,7 @@ the file contents.
[`fs.chmod()`]: #fschmodpath-mode-callback
[`fs.chown()`]: #fschownpath-uid-gid-callback
[`fs.copyFile()`]: #fscopyfilesrc-dest-mode-callback
[`fs.copyFileSync()`]: #fscopyfilesyncsrc-dest-mode
[`fs.createReadStream()`]: #fscreatereadstreampath-options
[`fs.createWriteStream()`]: #fscreatewritestreampath-options
[`fs.exists()`]: #fsexistspath-callback
@@ -8037,6 +8056,7 @@ the file contents.
[`fs.writeFile()`]: #fswritefilefile-data-options-callback
[`fs.writev()`]: #fswritevfd-buffers-position-callback
[`fsPromises.access()`]: #fspromisesaccesspath-mode
[`fsPromises.copyFile()`]: #fspromisescopyfilesrc-dest-mode
[`fsPromises.open()`]: #fspromisesopenpath-flags-mode
[`fsPromises.opendir()`]: #fspromisesopendirpath-options
[`fsPromises.rm()`]: #fspromisesrmpath-options
2 changes: 1 addition & 1 deletion lib/internal/fs/cp/cp-sync.js
Original file line number Diff line number Diff line change
@@ -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);
}
2 changes: 1 addition & 1 deletion lib/internal/fs/cp/cp.js
Original file line number Diff line number Diff line change
@@ -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);
}
1 change: 1 addition & 0 deletions lib/internal/fs/utils.js
Original file line number Diff line number Diff line change
@@ -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');
}
104 changes: 104 additions & 0 deletions test/parallel/test-fs-cp.mjs
Original file line number Diff line number Diff line change
@@ -38,6 +38,30 @@ 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,
}));
} catch (err) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This catch block also catches assertDirEquivalent failures, doesn't it? Better to move that outside the block.

(Also applies to code further below.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This catch block also catches assertDirEquivalent failures, doesn't it?

Yes, it is. I addressed them in 3451bbe.

// 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');
return;
}

// If the platform support `COPYFILE_FICLONE_FORCE` operation,
// it should reach to here.
assertDirEquivalent(src, dest);
})();

// It does not throw errors when directory is copied over and force is false.
{
const src = nextdir();
@@ -107,6 +131,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 +457,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 +856,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 +875,35 @@ 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();
let p = null;
let successFiClone = false;
try {
p = await fs.promises.cp(src, dest, mustNotMutateObjectDeep({
recursive: true,
mode: fs.constants.COPYFILE_FICLONE_FORCE,
}));
successFiClone = true;
} 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');
}

if (successFiClone) {
// If the platform support `COPYFILE_FICLONE_FORCE` operation,
// it should reach to here.
assert.strictEqual(p, undefined);
assertDirEquivalent(src, dest);
}
}

// It accepts file URL as src and dest.
{
const src = './test/fixtures/copy/kitchen-sink';
@@ -847,6 +941,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);