Skip to content
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

copy*(): fix copying bind-mounted directories #618

Merged
merged 4 commits into from Sep 2, 2018

Conversation

@manidlou
Copy link
Collaborator

manidlou commented Aug 12, 2018

fix #613.

Fixed the issue of infinite loop that would used to happen for cases like bind-mounted directories with subdirectory that would end up creating a lot of recursive same subdirectories and finally failing with ENAMETOOLONG error.

With this fix, we catch these weird cases and throw error with descriptive message.

Also, cleaned up some tests.

@manidlou manidlou requested review from jprichardson, JPeer264 and RyanZim Aug 12, 2018
@manidlou manidlou self-assigned this Aug 12, 2018
@manidlou manidlou added this to the 8.0.0 milestone Aug 12, 2018
@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 12, 2018

Coverage Status

Coverage increased (+0.2%) to 86.189% when pulling 60e0210 on copy-fix-bind-mounted into 1657c61 on v8-dev.

@manidlou

This comment has been minimized.

Copy link
Collaborator Author

manidlou commented Aug 12, 2018

These tests that are failing are not related to these changes and are from case-insensitive-paths test. I realized the case-insensitive-paths test file was buggy and actually not getting the platform string correctly. So, I fixed that and tests passed on my linux machine, but apparently I have to look at it to see what's the issue with mac and windows!

@manidlou

This comment has been minimized.

Copy link
Collaborator Author

manidlou commented Aug 12, 2018

Cool It is fixed now! 😁

@rossj

This comment has been minimized.

Copy link

rossj commented Aug 12, 2018

Just some thoughts:

Might it be necessary to check up the whole dest path? For example, in the case of:

mkdir -p src/sub1/sub2
ln -s src/ dest
fs.copy('src', 'dest/sub1/sub2', cb)

If so, perhaps the stats / inode information can be cached, to prevent having to re-fetch this information every time a dir in src is copied. Or, perhaps this check only needs to be made for the main src and dest, and not again inside copyDirItem() for every subdir in src?

@manidlou

This comment has been minimized.

Copy link
Collaborator Author

manidlou commented Aug 16, 2018

@rossj that's a good point! I actually had that in mind when I started working on it, but somehow slipped through the cracks! I'd say we probably need to check only the main src and dest.

@RyanZim

This comment has been minimized.

Copy link
Collaborator

RyanZim commented Aug 28, 2018

@manidlou ping?

@manidlou

This comment has been minimized.

Copy link
Collaborator Author

manidlou commented Aug 29, 2018

@RyanZim thanks for pinging! I'll try to finish it this weekend.

@manidlou manidlou force-pushed the copy-fix-bind-mounted branch from 90a762f to 60e0210 Aug 31, 2018
@manidlou

This comment has been minimized.

Copy link
Collaborator Author

manidlou commented Aug 31, 2018

It is ready to be reviewed! @jprichardson @RyanZim @JPeer264 @rossj

Copy link
Collaborator

RyanZim left a comment

Code LGTM

@manidlou

This comment has been minimized.

Copy link
Collaborator Author

manidlou commented Sep 1, 2018

@jprichardson @JPeer264 do you have any concerns about this?! Because I have the move*() PR ready too and that'd be better if we merge this first before that.

@jprichardson jprichardson merged commit e0093f5 into v8-dev Sep 2, 2018
5 checks passed
5 checks passed
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 86.189%
Details
@jprichardson jprichardson deleted the copy-fix-bind-mounted branch Sep 2, 2018
@jprichardson

This comment has been minimized.

Copy link
Owner

jprichardson commented Sep 2, 2018

Nice work @manidlou!

manidlou added a commit that referenced this pull request Apr 1, 2019
* copy*(): fix copying bind-mounted dirs

* copy*(): fix case-insensitive-paths tests

* copy*(): refactor to check paths more efficiently

* destructure stats object after checking err
manidlou added a commit that referenced this pull request Apr 4, 2019
* copy*(): fix copying bind-mounted dirs

* copy*(): fix case-insensitive-paths tests

* copy*(): refactor to check paths more efficiently

* destructure stats object after checking err
@RyanZim RyanZim referenced this pull request May 10, 2019
RyanZim added a commit that referenced this pull request May 11, 2019
* Remove secure-random from dev-deps (#610)

* fix ensureDir() doc

* moveSync: refactor to use renameSync

* copy*(): fix copying bind-mounted directories (#618)

* copy*(): fix copying bind-mounted dirs

* copy*(): fix case-insensitive-paths tests

* copy*(): refactor to check paths more efficiently

* destructure stats object after checking err

* move*(): check paths before moving

* move*(): add case-insensitive paths test

* remove unnecessary done callback from test

* copy*(): add new option checkPathsBeforeCopying

* update copy*() docs to include checkPathsBeforeCopying

* some reformatting

* copy*(): use fs.stat with bigint option

* move*(): refactor to use the internal stat functions

* move*(): add test for prevent moving identical

* disable graceful-fs in copy and move tests

* fix parsing node version

* tiny reformat

* update copy*() docs

* refactor parsing node version

* use semver to parse node version in tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.