From 52038c0d9e4a326e6b69df2ebc1300a2fa179087 Mon Sep 17 00:00:00 2001
From: Tetsuharu Ohzeki <tetsuharu.ohzeki@gmail.com>
Date: Tue, 14 Mar 2023 14:42:02 +0900
Subject: [PATCH 1/2] fs: add support for mode flag to specify the copy
 behavior

`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 whether the destination is expected
- 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                 |  20 +++++++
 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  | 101 ++++++++++++++++++++++++++++++++++
 5 files changed, 124 insertions(+), 2 deletions(-)

diff --git a/doc/api/fs.md b/doc/api/fs.md
index 44bcd28a2ce638..a0fad8254b6580 100644
--- a/doc/api/fs.md
+++ b/doc/api/fs.md
@@ -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
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..9434f64cf1f40d 100644
--- a/test/parallel/test-fs-cp.mjs
+++ b/test/parallel/test-fs-cp.mjs
@@ -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) {
+    // 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 () => {
+  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);
+})();
+
 // 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);

From 6a2b4bc7a566568ca65d5bcfc255a5761e424289 Mon Sep 17 00:00:00 2001
From: Tetsuharu Ohzeki <tetsuharu.ohzeki@gmail.com>
Date: Sat, 15 Apr 2023 01:17:05 +0900
Subject: [PATCH 2/2] fixup! fs: add support for mode flag to specify the copy
 behavior

---
 doc/api/fs.md                |  6 +++---
 test/parallel/test-fs-cp.mjs | 17 ++++++++++-------
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/doc/api/fs.md b/doc/api/fs.md
index a0fad8254b6580..c3a4a857e86a29 100644
--- a/doc/api/fs.md
+++ b/doc/api/fs.md
@@ -997,7 +997,7 @@ changes:
     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()`][]
+    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`
@@ -2328,7 +2328,7 @@ changes:
     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()`][]
+    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`
@@ -5233,7 +5233,7 @@ changes:
     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()`][]
+    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/test/parallel/test-fs-cp.mjs b/test/parallel/test-fs-cp.mjs
index 9434f64cf1f40d..8aedad334d2b24 100644
--- a/test/parallel/test-fs-cp.mjs
+++ b/test/parallel/test-fs-cp.mjs
@@ -877,29 +877,32 @@ if (!isWindows) {
 
 // It copies a nested folder structure with mode flags.
 // This test is based on fs.promises.copyFile() with `COPYFILE_FICLONE_FORCE`.
-await (async () => {
+{
   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');
-    return;
   }
 
-  // If the platform support `COPYFILE_FICLONE_FORCE` operation,
-  // it should reach to here.
-  assert.strictEqual(p, undefined);
-  assertDirEquivalent(src, dest);
-})();
+  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.
 {