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 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 self-assigned this Aug 12, 2018
@manidlou manidlou added this to the 8.0.0 milestone Aug 12, 2018
@coveralls
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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

Cool It is fixed now! 😁

@rossj
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
Copy link
Collaborator Author

@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
Copy link
Collaborator

RyanZim commented Aug 28, 2018

@manidlou ping?

@manidlou
Copy link
Collaborator Author

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

@manidlou
Copy link
Collaborator Author

manidlou commented Aug 31, 2018

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

Copy link
Collaborator

@RyanZim RyanZim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM

@manidlou
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
@jprichardson jprichardson deleted the copy-fix-bind-mounted branch September 2, 2018 18:33
@jprichardson
Copy link
Owner

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 mentioned this pull request May 10, 2019
RyanZim pushed 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
Development

Successfully merging this pull request may close these issues.

None yet

6 participants