bugfix/492 - Replace fs.link/unlink with fs.rename in fse.move

[Faline10]

Fix for: #492
"Folders become inoperable after fse.move, because fs.link isn't returning EPERM error [Mac]"

Previous move logic:

• If overwriting, use rename
• If not, use fs.link, then fs.unlink. fs.link will throw an EPERM error if it's a directory, in which case, recursively copy.

The downside to the previous logic is that the recursive copy for the folder is a heavy operation, and it took several different fs operations instead of just one. Also, in the case of this bug on Macs, fs.link would partially succeed in moving a folder instead of throwing an error, leading to an inoperable state on the folder.

Updated move logic:

• Use fs.existsSync to prevent overwriting
• Use fs.rename, which should work on both files and folders

Unlike fs.link, fs.rename will overwrite by default, so must do an existence check first to prevent this. I believe this is a decent tradeoff for fixing this bug and avoiding heavy recursive copy operations on folders.

Other Changes:

• Updated tests
• Got rid of ENOTSUP error checking since this isn't returned by rename nor link, and wasn't a unit test case, so not sure what it was doing there: http://man7.org/linux/man-pages/man2/rename.2.html, http://man7.org/linux/man-pages/man2/link.2.html
• Similarly, there was an 'EISDIR' check for fs.link, when that does not appear to be an error thrown by fs.link. There is an 'EISDIR' for rename, though the definition ("newpath is an existing directory, but oldpath is not a directory") does not suit the use case of calling moveAcrossDevice()/recursive copy, so I have removed this check as well.

Ran tests/linter on both Mac and Windows.

[RyanZim]

It seems we added the ESDIR error logic to handle some quirky virtual filesystems: 24948a4 We also have mock tests for this error, which are now failing with your changes. @jprichardson This happened before I was with the project; do you remember any specific details about this? Interested to know your thoughts.

I'm not really comfortable with using a sync method inside an asynchronous function; this could have a significant perf hit.

I also don't like manually constructing fake system-level errors, but if it's a necessary evil, then so be it.

[Faline10]

@RyanZim Ah, apologies, I was too excited and forgot to run the tests after the last minute change. Added the EISDIR logic back in. However, the EISDIR logic was previously in the fs.link error handler, so I am not sure if the use case still applies. I am unsure how to manually test the quirky virtual filesystem scenario. Hopefully the test case will suffice. Please let me know if you or JP have any further thoughts here.

I understand your reservations about the sync method. To be clear, since the boolean overwrite check short circuits, it will only run when overwrite is not true. I can try to find a folder with many files and profile move to compare performance in this scenario.

[coveralls]

Coverage decreased (-0.1%) to 87.278% when pulling 09236ab on Faline10:fawn/bugfix/492 into 2599b67 on jprichardson:master.

[manidlou]

How is this going?! ping @RyanZim @Faline10?

[RyanZim]

@manidlou I'm not comfortable merging a PR that adds sync fs calls to an async method.

[manidlou]

I'm not comfortable merging a PR that adds sync fs calls to an async method.

@RyanZim I definitely agree with you.

So, @Faline10 will you modify your PR not to use sync function please?

[Faline10]

@manidlou @RyanZim This fix is based off the premise of using fs.rename, which is a fix I used for this bug at work. It works in our scenario, but for fs-extra, there needs to be an existsSync check before it because otherwise it will always overwrite. It only does this check when the overwrite option is false, but that could be a majority of the time. It could be that the previous way this worked on folders, through recursively copying, would be just as slow. Due to the holidays, I haven't had time to test this theory. If that sync check doesn't suit this code base, I'm fine with closing this PR and just referencing it as a "work around" solution in the bug, in case anyone encounters the same problem, and leave it up to you as to whether to close the bug or not.

[RyanZim]

Superseded by #549