[WIP] Rewrite lib/copy/ncp.js #374

manidlou · 2017-02-26T07:49:30Z

Refs: #292.

This is the first draft of reconsidering/rewriting lib/copy/ncp.js.

As @RyanZim mentioned before:

Once rewritten, it may be small enough to inline in lib/copy/copy.js;

It make sense to me too. So, it is wrapped in one file now lib/copy/copy.js.

A few notes:

In checkDest(), the reason that I directly use fs.readlink() as apposed to getting the fs.stat() then decide accordingly, is to avoid possibility of race since we essentially need to check src against a possible resolved dest path. See handle copying between identical files #198.
I added more tests for symlinks, copying identical, and copying into itself as well. These changes should resolve handle copying between identical files #198 and Prevent copying a directory into itself #83.
I activated the move() skipped tests: https://github.com/jprichardson/node-fs-extra/blob/master/lib/move/__tests__/move.test.js#L310. It passes now.
Currently all tests pass including old and new tests.
The reason that I didn't create the branch here is I figured that'd be better to keep the upstream repo as clean as possible since this one probably take some time to discuss and decide on and whenever we want we can migrate it here.

Please feel free to point out any issues that you can think of with these changes.

coveralls · 2017-02-26T07:51:18Z

Coverage decreased (-1.1%) to 84.337% when pulling 54ebe52 on manidlou:rewrite-copy into 42656df on jprichardson:master.

manidlou · 2017-02-26T11:13:37Z

Hmm! coverage decreased! I will look at it and try to add tests for the parts that are not covered yet.

coveralls · 2017-02-26T13:45:59Z

Coverage increased (+0.2%) to 85.663% when pulling cac1696 on manidlou:rewrite-copy into 42656df on jprichardson:master.

RyanZim

Overall, LGTM. Couple of nits below. Also a couple of questions for @jprichardson on aspects I'm not sure about.

@manidlou This needs to be rebased on top of the latest master to avoid undoing work done on move's tests.

RyanZim · 2017-03-03T14:23:40Z

lib/copy/__tests__/copy-prevent-copying-into-itself.test.js

+    fs.writeFileSync(path.join(src, FILES[0]), dat0)
+    fs.writeFileSync(path.join(src, FILES[1]), dat1)
+    fs.writeFileSync(path.join(src, FILES[2]), dat2)
+    fs.writeFileSync(path.join(src, FILES[3]), dat3)


Could use outputFile here instead of ensureFile and writeFile.

RyanZim · 2017-03-03T14:28:41Z

lib/copy/__tests__/copy-prevent-copying-into-itself.test.js

+        })
+      })
+
+      it(`should copy the directory successfully when dest is 'srcsrc/dest'`, done => {


According to the test's code, the description should say: when dest is 'src_src/dest' (missing underscore).

That's right. In fact, description is correct, but dest path in test's code should change to match the description.

RyanZim · 2017-03-03T14:31:58Z

lib/copy/__tests__/copy.test.js

@@ -159,6 +159,7 @@ describe('fs-extra', () => {
        })
      })

+      /* It is redundant. See above, we already tested for this case.


Just remove this test.

RyanZim · 2017-03-03T14:32:41Z

lib/copy/__tests__/copy.test.js

+        const filter = /.html$|.css$/i
+
+        fse.copy(srcFile1, destFile1, filter, (err) => {
+          assert(!err)


Could use assert.ifError(err) here.

RyanZim · 2017-03-03T14:35:33Z

lib/copy/copy.js

+
+  options.filter = options.filter || function () { return true }
+
+  // Warn about using preserveTimestamps on 32-bit node
  if (options.preserveTimestamps && process.arch === 'ia32') {
    console.warn(`fs-extra: Using the preserveTimestamps option in 32-bit node is not recommended;\n
    see https://github.com/jprichardson/node-fs-extra/issues/269`)


Could port this to ES6 template strings.

@RyanZim would you please say what you mean exactly? is it referring to the extra \n in the warn?

Oops, nevermind.

Is that \n necessary as with template strings we can do multi-line like normally?

@manidlou We are already using template strings (I didn't notice that when I commented here). The \n is to make a double linebreak.

Just ignore this line comment.

RyanZim · 2017-03-03T14:44:43Z

lib/copy/copy.js

-  if (typeof options === 'function' && !callback) {
-    callback = options
+const DEST_NOENT = -1
+const DEST_EXISTS = 1


Possibly could/should use ES6 Symbols here?

I like the idea of using Symbols here as one of their use cases is for unique constants.

RyanZim · 2017-03-03T14:45:47Z

lib/copy/copy.js

+      if (err) return cb(err)
+      if (options.preserveTimestamps) {
+        return utimes(dest, srcStat.atime, srcStat.mtime, cb)
+      } else return cb()


Don't need else here.

RyanZim · 2017-03-03T14:50:43Z

lib/copy/copy.js

+      if (isSrcSubdir(src, res)) return cb(new Error(`Cannot copy directory '${src}' into itself '${res}'`))
+      if (src === res) return cb()
+      return cpDir(src, dest, options, cb)
+    } else return cb()


In what case would this code path be taken? Just wondering.

If you mean line 141 } else return cb(), I guess it is not needed and should be removed as we check for all possible conditions.

RyanZim · 2017-03-03T14:52:54Z

lib/copy/copy.js

+      return cb()
+    }).catch(err => {
+      return cb(err)
+    })


Should be shortened to

})).then(() => cb()) .catch(cb)

RyanZim · 2017-03-03T14:56:47Z

lib/copy/copy.js

+        }
+        // if src points to dest
+        if (resolvedSrcPath === dest) return cb()
+        return copy(resolvedSrcPath, dest, options, cb)


I'm a bit hairy on expected behavior for symlinks; @jprichardson is this correct?

Honestly, I am not 100% sure neither. So, @jprichardson we definitely need some advice here.

@jprichardson ping?

@RyanZim I guess you are right. I don't have a good feeling about return copy(resolvedSrcPath, dest, options, cb) neither. I recall I used it because of the ability to handle weird cases like when src is a symlink and dest exists (either regular or another symlink) and is a kid of src as well.

return fs.symlink(resolvedSrcPath, dest, cb) correctly throws EEXIST: file already exists, symlink 'src' -> 'dest' for those cases.

Now, I think we should decide what we want to do here for these two cases when src is a symlink:

dest exists and is a regular file/directory

dest exists and is a symlink (file/directory)

Do we want to throw an error and notify user that dest exists?

Now, I think we should decide what we want to do here for these two cases when src is a symlink:

What is the existing behavior?

What is the existing behavior?

Currently, when src is a symlink, this is what happens for the following cases:

dest exists and is a regular file or directory

it throws Error: EINVAL: invalid argument, readlink 'dest', and this is because the existing behavior assumes when src is a symlink, dest is also a symlink (https://github.com/jprichardson/node-fs-extra/blob/master/lib/copy/ncp.js#L182).

dest exists and is a symlink

since overwrite is the default behavior, it removes dest then copies src (creates a symlink in dest that points to src).

Now, for the case 2, it works fine except if src is a kid of dest. In this case, since it removes dest, we end up losing src and consequently a broken symlink is created.

…re tests

… style

coveralls · 2017-03-08T23:34:58Z

Coverage increased (+0.6%) to 81.34% when pulling 436a587 on manidlou:rewrite-copy into b5c72dc on jprichardson:master.

RyanZim

Sorry so long in getting to this.

One old type-check in two places, other than that, LGTM.

RyanZim · 2017-03-15T12:44:54Z

lib/copy/copy.js

+    } else if (res === DEST_EXISTS) {
+      if (isSrcSubdir(src, dest)) return cb(new Error(`Cannot copy directory '${src}' into itself '${dest}'`))
+      return cpDir(src, dest, options, cb)
+    } else if (res && typeof res !== 'number') { // dest exists and is a link


This typeof res !== 'number' is obsolete since you are using symbols.

Also, I just noticed that for both cases where dest exists (regular and symlink), onDir() should check for options.overwrite and proceed its operation accordingly; the same as onFile().

Edit

Is that correct because I am not 100% sure if that should be the correct behavior for copying direcotory? 😕 Since it is mentioned in docs that overwrite would apply to both file and directory. Then, this algorithm wouldn't be consistent with docs.

@manidlou Historically, overwrite only is checked for files. There's no need to nuke an entire directory if we just need to add one file to it. I'd leave it that way.

RyanZim · 2017-03-15T12:46:01Z

lib/copy/copy.js

+        // if src points to dest
+        if (resolvedSrcPath === dest) return cb()
+        return copy(resolvedSrcPath, dest, options, cb)
+      } else if (resolvedDestPath && typeof resolvedDestPath !== 'number') { // dest is a link


Again here also, when dest exists (regular and symlink), should go with the same pattern of checking options.overwrite and operates accordingly.

manidlou · 2017-03-16T04:04:05Z

lib/copy/copy.js

+      } else if (options.errorOnExist) {
+        return cb(new Error(dest + ' already exists'))
+      } else return cb()
+    } else return cb()


This last else basically means dest exists and is a symlink. I believe the same pattern that is used for the case DEST_EXISTS, which means checking for options.overwrite and operates accordingly, should be applied for this case as well. Is that right?

I'm not sure. @jprichardson ?

Agreed @manidlou

manidlou · 2017-03-16T04:05:24Z

lib/copy/copy.js

+    } else if (res === DEST_EXISTS) {
+      if (isSrcSubdir(src, dest)) return cb(new Error(`Cannot copy directory '${src}' into itself '${dest}'`))
+      return cpDir(src, dest, options, cb)
+    } else if (res && typeof res !== 'number') { // dest exists and is a link


Also, I just noticed that for both cases where dest exists (regular and symlink), onDir() should check for options.overwrite and proceed its operation accordingly; the same as onFile().

Edit

Is that correct because I am not 100% sure if that should be the correct behavior for copying direcotory? 😕 Since it is mentioned in docs that overwrite would apply to both file and directory. Then, this algorithm wouldn't be consistent with docs.

manidlou · 2017-03-16T04:11:30Z

lib/copy/copy.js

+        // if src points to dest
+        if (resolvedSrcPath === dest) return cb()
+        return copy(resolvedSrcPath, dest, options, cb)
+      } else if (resolvedDestPath && typeof resolvedDestPath !== 'number') { // dest is a link


Again here also, when dest exists (regular and symlink), should go with the same pattern of checking options.overwrite and operates accordingly.

manidlou · 2017-03-16T04:17:20Z

lib/copy/__tests__/copy-prevent-copying-identical.test.js

+        fs.mkdirsSync(dest)
+        const subdir = path.join(TEST_DIR, 'dest', 'subdir')
+        fs.mkdirsSync(subdir)
+        fs.writeFileSync(path.join(subdir, 'file.txt'), 'some data')


Will be changed to use fs.outputFileSync().

manidlou · 2017-03-16T04:18:46Z

lib/copy/__tests__/copy-prevent-copying-identical.test.js

+      it('should not copy and return', done => {
+        src = path.join(TEST_DIR, 'src', 'somefile.txt')
+        fs.ensureFileSync(src)
+        fs.writeFileSync(src, 'some data')


Same here. It will be changed to use fs.outputFileSync() instead.

manidlou · 2017-03-16T04:19:14Z

lib/copy/__tests__/copy-prevent-copying-identical.test.js

+      it('should not copy and return', done => {
+        src = path.join(TEST_DIR, 'src', 'srcfile.txt')
+        fs.ensureFileSync(src)
+        fs.writeFileSync(src, 'src data')


manidlou · 2017-03-16T04:19:28Z

lib/copy/__tests__/copy-prevent-copying-identical.test.js

+      it('should not copy and return', done => {
+        dest = path.join(TEST_DIR, 'dest', 'somefile.txt')
+        fs.ensureFileSync(dest)
+        fs.writeFileSync(dest, 'some data')


RyanZim · 2017-04-05T01:00:26Z

@manidlou ping?

manidlou · 2017-04-05T02:33:37Z

@RyanZim I asked a question a few comments above regarding copying when src is a symlink, the case that we are not sure about the correct behavior yet, that I've been waiting for your and @jprichardson's answers. Would you please take a look at it and give me your ideas about it? 😃

…vice versa, add tests

coveralls · 2017-04-28T23:15:53Z

Coverage decreased (-0.2%) to 86.751% when pulling b9834f9 on manidlou:rewrite-copy into 096a8e1 on jprichardson:master.

manidlou · 2017-04-28T23:28:02Z

So, following our discussion on copying symlinks when src is a dest's kid and vice versa, to me for this case (which I guess is relatively a rare case) it is reasonable to throw error and let user decide on the action. Although we can deliberately handle these cases, but that makes it overly complex while it think it is not worth it for this case. I tried to find a short and yet descriptive error message for this case. I came up with ${dest} exists and contains ${src}.

@jprichardson @RyanZim what do you think then?

RyanZim

I think overall LGTM

@jprichardson Please review carefully though, I might have missed something.

RyanZim

I guess this looks OK, @jprichardson ?

manidlou · 2017-05-08T15:07:23Z

What about noRecurseOnFailedFilter? do you think it is reasonable enough to keep it or it's better to remove it for the sake of simplicity?

coveralls · 2017-05-08T15:58:49Z

Coverage decreased (-0.7%) to 86.242% when pulling cecd184 on manidlou:rewrite-copy into 096a8e1 on jprichardson:master.

…son/node-fs-extra#374) is published.

RyanZim · 2017-06-20T13:52:26Z

@jprichardson Thoughts on this?

manidlou · 2017-09-24T22:51:31Z

@jprichardson @RyanZim will you please take a look at https://github.com/manidlou/benchmark-fs-extra-copy?

I add a quick summary here too:

I've been working on an alternative implementation for copy() that uses recursive function (I couldn't find a better name) instead of Promise for copying a directory to see if we can get a faster execution. In addition, I organized the code into bunch of small functions as they can be optimized by v8 particularly when copying directories with lots of files and subdirectories as the same function needs to be called repeatedly. You've probably seen these posts or alike before: A tour of V8: Crankshaft, the optimizing compiler and NodeJS : A quick optimization advice.

So, I created a benchmark to compare the running time of these implementations:

this new recursive function
Promise
current (existing)

to see which one runs faster particularly on large input sizes, when a src directory has lots of items.

So, when src length is up to ~6000, they run pretty much the same. However, on roughly > 6000 the new recursive function runs significantly faster than other implementations.

line chart comparison, range: 110-8190

line chart comparison, range: 110-111110

any thoughts please?

RyanZim · 2017-10-18T20:14:11Z

@manidlou Sorry so terribly long in getting back (again). If you're comfortable with the recursive method; by all means add it. Just glancing at it, it doesn't look bad, and better perf is always good.

RyanZim · 2017-10-18T20:15:40Z

BTW @manidlou, please don't let me just let stuff slide like this; if I don't reply within 7 days, you're welcome to ping me (I forget stuff really easily; if I'm not responding, I could be busy, but most of the time, I just forgot about it).

manidlou · 2017-10-19T09:14:46Z

@RyanZim besides perf, overall I personally have a better feeling about recursive than Promise. So, if you don't mind, since it's been a long time that this PR been open, just for saving a bit of time resolving conflicts manually, I'll open a new PR for recursive implementation and close this one. Of course, if you think we need to keep this open, please don't hesitate to reopen it! 😄

jprichardson requested a review from RyanZim February 26, 2017 21:38

RyanZim requested changes Mar 3, 2017

View reviewed changes

RyanZim requested a review from jprichardson March 3, 2017 15:04

This was referenced Mar 4, 2017

fs.move fails with chmod error when disk under heavy use #357

Closed

Add moveSync and its tests #381

Merged

manidlou added 10 commits March 8, 2017 15:23

Rewrite copy, add more tests to copy, activate skipped move test

2dbc3b3

lib/copy/__tests__: Add ncp tests

84681ed

lib/copy/__tests__: Add more unit test

8175d0d

lib/copy: Remove options.dereference from first check on link, add mo…

364a114

…re tests

lib/copy/__tests__: Add more unit tests for symlinks

6e0cc40

lib/copy/__tests__: Add test for regex filter

10a5d11

lib/copy/__tests__/ncp: Revert ncp tests to previously es6 refactored…

d139e4f

… style

Rewrite copy: Apply requested changes for copy.js and its tests

0d81c2c

Add latest move test

469d55c

lib/copy__tests__/copy.test.js: Remove unnecessary test

436a587

RyanZim requested changes Mar 15, 2017

View reviewed changes

manidlou commented Mar 16, 2017

View reviewed changes

manidlou added 2 commits April 28, 2017 15:57

handle copying symlinks by throwing err when src is a dest's kid and …

7d1857d

…vice versa, add tests

merge master into rewrite-copy

b9834f9

RyanZim approved these changes Apr 29, 2017

View reviewed changes

put checkDest() in the block of copy()

70f2c7c

manidlou mentioned this pull request May 6, 2017

Improve filter option in copy() #419

Closed

Add improved filter option and its tests

6271e98

RyanZim approved these changes May 8, 2017

View reviewed changes

Change function names to filterDir and filterFile

cecd184

Alfredo-Delgado mentioned this pull request Jun 7, 2017

cordovaOptions glob array resources copy not being waited for stealjs/steal-cordova#14

Open

Alfredo-Delgado added a commit to stealjs/steal-cordova that referenced this pull request Jun 12, 2017

Workaround async copy bug in fs-extra until possible fix (jprichard…

3f39ed5

…son/node-fs-extra#374) is published.

Alfredo-Delgado mentioned this pull request Jun 12, 2017

Workaround async copy bug in fs-extra until possible fix stealjs/steal-cordova#17

Merged

Repository owner deleted a comment from coveralls Jun 29, 2017

RyanZim mentioned this pull request Sep 5, 2017

Rewrite lib/copy/ncp.js #292

Closed

manidlou closed this Oct 19, 2017

manidlou mentioned this pull request Oct 19, 2017

Rewrite copy #502

Merged

manidlou deleted the rewrite-copy branch October 19, 2017 10:41

RyanZim added this to the 5.0.0 milestone Nov 9, 2017

[WIP] Rewrite lib/copy/ncp.js #374

[WIP] Rewrite lib/copy/ncp.js #374

Conversation

manidlou commented Feb 26, 2017

coveralls commented Feb 26, 2017

manidlou commented Feb 26, 2017

coveralls commented Feb 26, 2017

RyanZim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanZim Mar 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanZim Mar 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 8, 2017

RyanZim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manidlou Mar 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manidlou Mar 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanZim commented Apr 5, 2017

manidlou commented Apr 5, 2017 • edited

coveralls commented Apr 28, 2017

manidlou commented Apr 28, 2017

RyanZim left a comment

Choose a reason for hiding this comment

RyanZim left a comment

Choose a reason for hiding this comment

manidlou commented May 8, 2017

coveralls commented May 8, 2017

RyanZim commented Jun 20, 2017

manidlou commented Sep 24, 2017

RyanZim commented Oct 18, 2017

RyanZim commented Oct 18, 2017

manidlou commented Oct 19, 2017

RyanZim Mar 3, 2017 •

edited

RyanZim Mar 3, 2017 •

edited

manidlou Mar 16, 2017 •

edited

manidlou Mar 16, 2017 •

edited

manidlou commented Apr 5, 2017 •

edited