fs: `fs.cp()` should accept mode flag to specify the copy behavior by tetsuharuohzeki · Pull Request #47084 · nodejs/node

tetsuharuohzeki · 2023-03-14T06:52:45Z

fs.copyFile() supports copy-on-write operation if the underlying platform supports it by passing a mode flag. This behavior was added in a16d88d.

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.
a16d88d.

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: #47080

debadree25 · 2023-03-14T06:59:13Z

cc @nodejs/fs

bnoordhuis · 2023-03-14T11:06:24Z

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


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

(Also applies to code further below.)

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

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

tetsuharuohzeki · 2023-03-20T17:49:02Z

@bnoordhuis How about this? Could you review this again?

tetsuharuohzeki · 2023-03-21T17:44:06Z

@nodejs/fs

Could you review this?

bnoordhuis · 2023-03-21T20:50:49Z

@tetsuharuohzeki my comment was a drive-by thing, just something I noticed. I'll leave signing off on the main change to @nodejs/fs.

tetsuharuohzeki · 2023-03-29T11:39:05Z

I rebased this on the latest main.

aduh95 · 2023-04-14T10:24:56Z

Can you amend the commit message so it matches our commit message guidelines?

node/doc/contributing/pull-requests.md

Lines 168 to 169 in 5af2021

    
           * be prefixed with the name of the changed [subsystem](#appendix-subsystems) 
        
             and start with an imperative verb. Check the output of `git log --oneline

E.g. fs: add support for mode flag to specify the copy behavior would work.

aduh95

Can you add an entry in the YAML history section for each modified fs method?

node/doc/api/fs.md

Lines 5193 to 5199 in a9c3d92

    
           changes: 
        
             - version: 
        
               - v17.6.0 
        
               - v16.15.0 
        
               pr-url: https://github.com/nodejs/node/pull/41819 
        
               description: Accepts an additional `verbatimSymlinks` option to specify 
        
                            whether to perform path resolution for symlinks.

tetsuharuohzeki · 2023-04-14T13:17:20Z

@aduh95

Thank you for your review! I addressed my patch.
How about this?

`fs.copyFile()` supports copy-on-write operation if the underlying platform supports it by passing a mode flag. This behavior was added in nodejs@a16d88d. 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`. nodejs@a16d88d. 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: nodejs#47080

aduh95 · 2023-04-14T16:01:48Z

    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()`][]


Suggested change

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

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

fixed in 6a2b4bc

aduh95 · 2023-04-14T16:01:56Z

    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()`][]


Suggested change

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

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

fixed in 6a2b4bc

aduh95 · 2023-04-14T16:01:58Z

    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()`][]


Suggested change

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

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

fixed in 6a2b4bc

aduh95 · 2023-04-14T16:03:00Z


+// It copies a nested folder structure with mode flags.
+// This test is based on fs.promises.copyFile() with `COPYFILE_FICLONE_FORCE`.
+await (async () => {


Suggested change

await (async () => {

{

fixed in 6a2b4bc

aduh95 · 2023-04-14T16:03:05Z

+  // it should reach to here.
+  assert.strictEqual(p, undefined);
+  assertDirEquivalent(src, dest);
+})();


Suggested change

})();

}

fixed in 6a2b4bc

nodejs-github-bot · 2023-04-14T19:16:01Z

CI: https://ci.nodejs.org/job/node-test-pull-request/51266/

tetsuharuohzeki · 2023-04-20T11:42:50Z

@aduh95 Thank you for your review!

nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Mar 14, 2023

tetsuharuohzeki force-pushed the allow-to-pass-mode-flag-for-cp branch from 3579490 to 1b4e164 Compare March 14, 2023 07:38

bnoordhuis reviewed Mar 14, 2023

View reviewed changes

tetsuharuohzeki requested a review from bnoordhuis March 16, 2023 16:26

tetsuharuohzeki force-pushed the allow-to-pass-mode-flag-for-cp branch from 3451bbe to 34dc434 Compare March 29, 2023 11:38

aduh95 reviewed Apr 14, 2023

View reviewed changes

aduh95 added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 14, 2023

tetsuharuohzeki force-pushed the allow-to-pass-mode-flag-for-cp branch from 34dc434 to fc8eafe Compare April 14, 2023 13:16

debadree25 added the review wanted PRs that need reviews. label Apr 14, 2023

tetsuharuohzeki force-pushed the allow-to-pass-mode-flag-for-cp branch from fc8eafe to 1f6dfc4 Compare April 14, 2023 13:27

tetsuharuohzeki force-pushed the allow-to-pass-mode-flag-for-cp branch from 1f6dfc4 to 52038c0 Compare April 14, 2023 15:55

aduh95 reviewed Apr 14, 2023

View reviewed changes

fixup! fs: add support for mode flag to specify the copy behavior

6a2b4bc

tetsuharuohzeki force-pushed the allow-to-pass-mode-flag-for-cp branch from 4e1e069 to 6a2b4bc Compare April 14, 2023 17:08

tetsuharuohzeki requested a review from aduh95 April 14, 2023 17:09

aduh95 approved these changes Apr 14, 2023

View reviewed changes

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Apr 14, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 14, 2023

github-actions Bot mentioned this pull request Apr 15, 2023

CI Reliability 2023-04-15 nodejs/reliability#543

Open

33 tasks

targos mentioned this pull request May 2, 2023

v20.1.0 release proposal #47820

Merged

tetsuharuohzeki mentioned this pull request May 4, 2023

feat: Support mode flag by cp() polyfill npm/fs#73

Open

1 task

danielleadams mentioned this pull request Jul 10, 2023

v18.17.0 release proposal #48694

Merged

	changes:
	- version:
	- v17.6.0
	- v16.15.0
	pr-url: https://github.com/nodejs/node/pull/41819
	description: Accepts an additional `verbatimSymlinks` option to specify
	whether to perform path resolution for symlinks.

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

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

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

Uh oh!

Uh oh!

Conversation

tetsuharuohzeki commented Mar 14, 2023

Uh oh!

debadree25 commented Mar 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tetsuharuohzeki commented Mar 20, 2023

Uh oh!

tetsuharuohzeki commented Mar 21, 2023

Uh oh!

bnoordhuis commented Mar 21, 2023

Uh oh!

tetsuharuohzeki commented Mar 29, 2023

Uh oh!

aduh95 commented Apr 14, 2023

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

tetsuharuohzeki commented Apr 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Apr 14, 2023

Uh oh!

tetsuharuohzeki commented Apr 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants