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

Set new directory mode after copying files (MacOS fix) #600

Merged

Conversation

@mbargiel
Copy link
Contributor

@mbargiel mbargiel commented Jul 6, 2018

Fix for #599

There are two distinct changes needed to fix this:

  • When creating a new directory, don't specify the file mode
  • Switch chmod and recursive copy so chmod happens after

I ran the tests locally on my MacBook. 3 tests (related to preserveTimestamps and hasMillisResSync) failed both before and after the change. I included a new test (hopefully close enough to the style already in use) that fails without my fix, and succeeds with it.

@coveralls
Copy link

@coveralls coveralls commented Jul 6, 2018

Coverage Status

Coverage remained the same at 86.826% when pulling 6ef9210 on mbargiel:feature/fix-copy-with-readonly-dir into a5b5159 on jprichardson:master.

Loading

@jprichardson jprichardson requested a review from manidlou Jul 7, 2018
Copy link
Collaborator

@manidlou manidlou left a comment

@mbargiel thanks for your contribution! The same change needs to be applied to copySync as well.

Loading

@mbargiel
Copy link
Contributor Author

@mbargiel mbargiel commented Jul 9, 2018

@manidlou Done! I also slightly improved the original test to ensure the modes were preserved. That is one feature of the copy after all.

Loading

@manidlou
Copy link
Collaborator

@manidlou manidlou commented Jul 10, 2018

@mbargiel LGTM. Will you squash this please?

Loading

@mbargiel mbargiel force-pushed the feature/fix-copy-with-readonly-dir branch from 69969ab to 6ef9210 Jul 10, 2018
@mbargiel
Copy link
Contributor Author

@mbargiel mbargiel commented Jul 10, 2018

Done, and slightly rephrased commit message for clarity

Loading

@mbargiel
Copy link
Contributor Author

@mbargiel mbargiel commented Jul 10, 2018

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 384aae0 into jprichardson:v7-dev Jul 17, 2018
3 checks passed
Loading
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

4 participants