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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BREAKING] copy*(): use ino to check identical paths #582

Merged
merged 2 commits into from Jul 17, 2018
Merged

Conversation

@manidlou
Copy link
Collaborator

@manidlou manidlou commented May 16, 2018

...copy*() and long journey of dealing with lots of edge cases and trying to find a proper and yet simple approach to catch as many of those 馃悰s as we can!

fix #565. Also possibly fix #546 as we don't use fs.readlink anymore to check the paths.

I guess it's breaking as for the case of symlinks, we throw error with descriptive message instead of silently return.

  • use ino to check if src and dest are the same (Thanks to @TanninOne for bringing that idea). This is beyond checking just paths. Since fs.lstat stats the link itself not the actual file that it points to, we have to do an extra fs.stat to get the actual ino of both src and dest. This essentially relies on node's internal api to give us correct inos! This also catches the cases like when dest parent is a symlink that points to the parent of src! some 馃槻 stuff! (we used to only check src and dest themselves not their parents!)

  • check isSrcSubdir when checking paths before start copying (achieved the same results with shorter code!)

@manidlou manidlou self-assigned this May 16, 2018
@coveralls
Copy link

@coveralls coveralls commented May 16, 2018

Coverage Status

Coverage decreased (-0.6%) to 86.228% when pulling 456241a on copy-same-path into 3eceb91 on master.

Loading

Copy link
Collaborator

@RyanZim RyanZim left a comment

LGTM

Loading

@RyanZim RyanZim added this to the 7.0.0 milestone Jul 17, 2018
@RyanZim RyanZim changed the base branch from master to v7-dev Jul 17, 2018
@RyanZim RyanZim merged commit e340861 into v7-dev Jul 17, 2018
4 of 5 checks passed
Loading
@manidlou manidlou deleted the copy-same-path branch Jul 17, 2018
facebook-github-bot added a commit to facebook/flipper that referenced this issue Nov 14, 2018
Summary:
Changes are mostly bug fixes, that shouldn't affect us. From the change log:

7.0.1 / 2018-11-07
------------------

- Fix `removeSync()` on Windows, in some cases, it would error out with `ENOTEMPTY` ([#646](jprichardson/node-fs-extra#646))
- Document `mode` option for `ensureDir*()` ([#587](jprichardson/node-fs-extra#587))
- Don't include documentation files in npm package tarball ([#642](jprichardson/node-fs-extra#642), [#643](jprichardson/node-fs-extra#643))

7.0.0 / 2018-07-16
------------------

- **BREAKING:** Refine `copy*()` handling of symlinks to properly detect symlinks that point to the same file. ([#582](jprichardson/node-fs-extra#582))
- Fix bug with copying write-protected directories ([#600](jprichardson/node-fs-extra#600))
- Universalify `fs.lchmod()` ([#596](jprichardson/node-fs-extra#596))
- Add `engines` field to `package.json` ([#580](jprichardson/node-fs-extra#580))

6.0.1 / 2018-05-09
------------------

- Fix `fs.promises` `ExperimentalWarning` on Node v10.1.0 ([#578](jprichardson/node-fs-extra#578))

6.0.0 / 2018-05-01
------------------

- Drop support for Node.js versions 4, 5, & 7 ([#564](jprichardson/node-fs-extra#564))
- Rewrite `move` to use `fs.rename` where possible ([#549](jprichardson/node-fs-extra#549))
- Don't convert relative paths to absolute paths for `filter` ([#554](jprichardson/node-fs-extra#554))
- `copy*`'s behavior when `preserveTimestamps` is `false` has been OS-dependent since 5.0.0, but that's now explicitly noted in the docs ([#563](jprichardson/node-fs-extra#563))
- Fix subdirectory detection for `copy*` & `move*` ([#541](jprichardson/node-fs-extra#541))
- Handle case-insensitive paths correctly in `copy*` ([#568](jprichardson/node-fs-extra#568))

Reviewed By: jknoxville

Differential Revision: D13023753

fbshipit-source-id: 1ecc6f40be4c8146f92dd29ede846b5ab56765ea
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants