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

Remove unneeded and buggy stats check #976

Merged
merged 1 commit into from Nov 19, 2022
Merged

Remove unneeded and buggy stats check #976

merged 1 commit into from Nov 19, 2022

Conversation

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

RyanZim commented Oct 31, 2022

@manidlou @bcoe bump

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

manidlou commented Nov 12, 2022

@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

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

Thank you @RyanZim! That makes sense.

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.

Port copy fix from Node.js core
2 participants