-
Notifications
You must be signed in to change notification settings - Fork 30.5k
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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()`][] | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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` | ||||||
|
@@ -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()`][] | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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` | ||||||
|
@@ -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 | ||||||
|
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) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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,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 () => { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||||||
})(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'; | ||||||
|
@@ -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); | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 6a2b4bc