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
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 20 additions & 0 deletions doc/api/fs.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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()`][]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
See `mode` flag of [`fsPromises.copyFile()`][]
See `mode` flag of [`fsPromises.copyFile()`][].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 6a2b4bc

* `preserveTimestamps` {boolean} When `true` timestamps from `src` will
be preserved. **Default:** `false`.
* `recursive` {boolean} copy directories recursively **Default:** `false`
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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()`][]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
See `mode` flag of [`fs.copyFile()`][]
See `mode` flag of [`fs.copyFile()`][].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 6a2b4bc

* `preserveTimestamps` {boolean} When `true` timestamps from `src` will
be preserved. **Default:** `false`.
* `recursive` {boolean} copy directories recursively **Default:** `false`
Expand Down Expand Up @@ -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
Expand All @@ -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()`][]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
See `mode` flag of [`fs.copyFileSync()`][]
See `mode` flag of [`fs.copyFileSync()`][].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 6a2b4bc

* `preserveTimestamps` {boolean} When `true` timestamps from `src` will
be preserved. **Default:** `false`.
* `recursive` {boolean} copy directories recursively **Default:** `false`
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/fs/cp/cp-sync.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/fs/cp/cp.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
1 change: 1 addition & 0 deletions lib/internal/fs/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
}
Expand Down
101 changes: 101 additions & 0 deletions test/parallel/test-fs-cp.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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.
{
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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.
Expand All @@ -810,6 +875,32 @@ 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`.
await (async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
await (async () => {
{

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 6a2b4bc

const src = './test/fixtures/copy/kitchen-sink';
const dest = nextdir();
let p = null;
try {
p = await fs.promises.cp(src, dest, mustNotMutateObjectDeep({
recursive: true,
mode: fs.constants.COPYFILE_FICLONE_FORCE,
}));
} 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');
return;
}

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

Choose a reason for hiding this comment

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

Suggested change
})();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 6a2b4bc


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