Skip to content

Remove unneeded and buggy stats check#976

Merged
RyanZim merged 1 commit intomasterfrom
ryan/cp-backport
Nov 19, 2022
Merged

Remove unneeded and buggy stats check#976
RyanZim merged 1 commit intomasterfrom
ryan/cp-backport

Conversation

@RyanZim
Copy link
Copy Markdown
Collaborator

@RyanZim RyanZim commented Oct 25, 2022

As per nodejs/node#39372 (comment); haven't gotten a response from @bcoe there yet

Resolves #918

Hope this logic is right.

@RyanZim RyanZim added this to the 11.0.0 milestone Oct 25, 2022
@RyanZim RyanZim requested a review from manidlou October 25, 2022 20:05
@RyanZim
Copy link
Copy Markdown
Collaborator Author

RyanZim commented Oct 31, 2022

@manidlou @bcoe bump

@RyanZim
Copy link
Copy Markdown
Collaborator Author

RyanZim commented Nov 10, 2022

I have been trying to write a test case for this; I cannot get to a place where this condition is ever reached. Do we need this at all? Or what am I missing?

@manidlou
Copy link
Copy Markdown
Collaborator

manidlou commented Nov 12, 2022 •
edited
Loading

@RyanZim thank you for your efforts! I really appreciate it!

This is basically a wild edge case where we're trying to avoid broken copy when src is a link and its resolved path is inside the dest directory. We already have this test

node-fs-extra/lib/copy/__tests__/copy-prevent-copying-into-itself.test.js

Lines 282 to 296 in e6a9505

it('should error when resolved src path is a subdir of dest', done => {
const dest = path.join(TEST_DIR, 'dest')
const resolvedSrcPath = path.join(dest, 'contains', 'src')
const srcLink = path.join(TEST_DIR, 'src-symlink')
fs.copySync(src, resolvedSrcPath)
// make symlink that points to a subdir in dest
fs.symlinkSync(resolvedSrcPath, srcLink, 'dir')
fs.copy(srcLink, dest, err => {
assert(err)
done()
})
})

so, what's wrong with this test?

@RyanZim
Copy link
Copy Markdown
Collaborator Author

RyanZim commented Nov 12, 2022

@manidlou The problem is that that test doesn't actually exercise this condition. We'd expect the error to be of the format:

Cannot overwrite '${resolvedDest}' with '${resolvedSrc}'.

Instead, if you insert a log statement in that test, the actual error thrown is:

Error: Cannot overwrite directory '/tmp/fs-extra/copy-prevent-copying-into-itself/dest' with non-directory '/tmp/fs-extra/copy-prevent-copying-into-itself/src-symlink'.

@RyanZim
Copy link
Copy Markdown
Collaborator Author

RyanZim commented Nov 19, 2022

I'm going to merge this as-is, without an explicit test, as it's certainly better than what we have now. However, we should still look into properly testing this later.

@RyanZim RyanZim merged commit 1a3205d into master Nov 19, 2022
@RyanZim RyanZim deleted the ryan/cp-backport branch November 19, 2022 18:09
@RyanZim RyanZim mentioned this pull request Nov 19, 2022
Closed
2 tasks
@manidlou
Copy link
Copy Markdown
Collaborator

manidlou commented Nov 19, 2022

Thank you @RyanZim! That makes sense.

valentineus pushed a commit to valentineus/strapi-plugin-checkbox-list that referenced this pull request Feb 5, 2026
@valentineus
Update dependency fs-extra to v11 (#6)
9012c5c 
This PR contains the following updates:

| Package | Change | [Age](https://docs.renovatebot.com/merge-confidence/) | [Confidence](https://docs.renovatebot.com/merge-confidence/) |
|---|---|---|---|
| [fs-extra](https://github.com/jprichardson/node-fs-extra) | [`^10.0.0` → `^11.0.0`](https://renovatebot.com/diffs/npm/fs-extra/10.1.0/11.3.3) | ![age](https://developer.mend.io/api/mc/badges/age/npm/fs-extra/11.3.3?slim=true) | ![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/fs-extra/10.1.0/11.3.3?slim=true) |

---

### Release Notes

<details>
<summary>jprichardson/node-fs-extra (fs-extra)</summary>

### [`v11.3.3`](https://github.com/jprichardson/node-fs-extra/blob/HEAD/CHANGELOG.md#1133--2025-12-18)

[Compare Source](jprichardson/node-fs-extra@11.3.2...11.3.3)

- Fix copying symlink when destination is a symlink to the same target ([#&#8203;1019](jprichardson/node-fs-extra#1019), [#&#8203;1060](jprichardson/node-fs-extra#1060))

### [`v11.3.2`](https://github.com/jprichardson/node-fs-extra/blob/HEAD/CHANGELOG.md#1132--2025-09-15)

[Compare Source](jprichardson/node-fs-extra@11.3.1...11.3.2)

- Fix spurrious `UnhandledPromiseRejectionWarning` that could occur when calling `.copy()` in some cases ([#&#8203;1056](jprichardson/node-fs-extra#1056), [#&#8203;1058](jprichardson/node-fs-extra#1058))

### [`v11.3.1`](https://github.com/jprichardson/node-fs-extra/blob/HEAD/CHANGELOG.md#1131--2025-08-05)

[Compare Source](jprichardson/node-fs-extra@11.3.0...11.3.1)

- Fix case where `move`/`moveSync` could incorrectly think files are identical on Windows ([#&#8203;1050](jprichardson/node-fs-extra#1050))

### [`v11.3.0`](https://github.com/jprichardson/node-fs-extra/blob/HEAD/CHANGELOG.md#1130--2025-01-15)

[Compare Source](jprichardson/node-fs-extra@11.2.0...11.3.0)

- Add promise support for newer `fs` methods ([#&#8203;1044](jprichardson/node-fs-extra#1044), [#&#8203;1045](jprichardson/node-fs-extra#1045))
- Use `fs.opendir` in `copy()`/`copySync()` for better perf/scalability ([#&#8203;972](jprichardson/node-fs-extra#972), [#&#8203;1028](jprichardson/node-fs-extra#1028))

### [`v11.2.0`](https://github.com/jprichardson/node-fs-extra/blob/HEAD/CHANGELOG.md#1120--2023-11-27)

[Compare Source](jprichardson/node-fs-extra@11.1.1...11.2.0)

- Copy directory contents in parallel for better performance ([#&#8203;1026](jprichardson/node-fs-extra#1026))
- Refactor internal code to use `async`/`await` ([#&#8203;1020](jprichardson/node-fs-extra#1020))

### [`v11.1.1`](https://github.com/jprichardson/node-fs-extra/blob/HEAD/CHANGELOG.md#1111--2023-03-20)

[Compare Source](jprichardson/node-fs-extra@11.1.0...11.1.1)

- Preserve timestamps when moving files across devices ([#&#8203;992](jprichardson/node-fs-extra#992), [#&#8203;994](jprichardson/node-fs-extra#994))

### [`v11.1.0`](https://github.com/jprichardson/node-fs-extra/blob/HEAD/CHANGELOG.md#1110--2022-11-29)

[Compare Source](jprichardson/node-fs-extra@11.0.0...11.1.0)

- Re-add `main` field to `package.json` for better TypeScript compatibility ([#&#8203;979](jprichardson/node-fs-extra#979), [#&#8203;981](jprichardson/node-fs-extra#981))

### [`v11.0.0`](https://github.com/jprichardson/node-fs-extra/blob/HEAD/CHANGELOG.md#1100--2022-11-28)

[Compare Source](jprichardson/node-fs-extra@10.1.0...11.0.0)

##### Breaking Changes

- Don't allow requiring `fs-extra/lib/SOMETHING` (switched to `exports`) ([#&#8203;974](jprichardson/node-fs-extra#974))
- Require Node v14.14+ ([#&#8203;968](jprichardson/node-fs-extra#968), [#&#8203;969](jprichardson/node-fs-extra#969))

##### New Features

- Add `fs-extra/esm` for ESM named export support; see [docs](https://github.com/jprichardson/node-fs-extra#esm) for details ([#&#8203;746](jprichardson/node-fs-extra#746), [#&#8203;974](jprichardson/node-fs-extra#974))
- Add promise support for `fs.readv()` ([#&#8203;970](jprichardson/node-fs-extra#970))

##### Bugfixes

- Don't `stat` filtered items in `copy*` ([#&#8203;965](jprichardson/node-fs-extra#965), [#&#8203;971](jprichardson/node-fs-extra#971))
- Remove buggy stats check in `copy` ([#&#8203;918](jprichardson/node-fs-extra#918), [#&#8203;976](jprichardson/node-fs-extra#976))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My4wLjUiLCJ1cGRhdGVkSW5WZXIiOiI0My4wLjUiLCJ0YXJnZXRCcmFuY2giOiJtYXN0ZXIiLCJsYWJlbHMiOlsiYXV0b21hdGVkIiwiZGVwZW5kZW5jaWVzIl19-->

Reviewed-on: https://code.popov.link/valentineus/strapi-plugin-checkbox-list/pulls/6
Co-authored-by: renovate[bot] <[email protected]>
Co-committed-by: renovate[bot] <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@manidlou manidlou Awaiting requested review from manidlou

Assignees

No one assigned

Labels

backport bug feature-copy

Projects

None yet

Milestone

11.0.0

Development

Successfully merging this pull request may close these issues.

Port copy fix from Node.js core

2 participants

@RyanZim @manidlou