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 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.

Copy link
Collaborator

@manidlou manidlou left a comment

Choose a reason for hiding this comment

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

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

@mbargiel
Copy link
Contributor Author

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.

@manidlou
Copy link
Collaborator

@mbargiel LGTM. Will you squash this please?

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

Done, and slightly rephrased commit message for clarity

@mbargiel
Copy link
Contributor Author

@manidlou ^

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.

LGTM

@RyanZim RyanZim added this to the 7.0.0 milestone Jul 17, 2018
@RyanZim RyanZim changed the base branch from master to v7-dev July 17, 2018 00:57
@RyanZim RyanZim merged commit 384aae0 into jprichardson:v7-dev Jul 17, 2018
facebook-github-bot pushed a commit to facebook/flipper that referenced this pull request 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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants