fs: fix cp, cpSync handle existing symlinks

[jakecastelli]

Fixes: #58468

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.22%. Comparing base (44a5375) to head (cde63b2).
Report is 103 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #58476      +/-   ##
==========================================
+ Coverage   90.19%   90.22%   +0.03%     
==========================================
  Files         635      635              
  Lines      187394   187608     +214     
  Branches    36800    36857      +57     
==========================================
+ Hits       169014   169265     +251     
+ Misses      11134    11133       -1     
+ Partials     7246     7210      -36     

Files with missing lines	Coverage Δ
lib/internal/fs/cp/cp-sync.js	92.20% <100.00%> (+0.06%)	⬆️
lib/internal/fs/cp/cp.js	91.19% <100.00%> (+0.87%)	⬆️

... and 40 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[dario-piotrowicz]

This looks great! thanks for the fix @jakecastelli 🫶

By the way, could you please rebase this to the latest main and basically revert the changes I made here? 🙂 #58472

[dario-piotrowicz]

I don't think that this PR fully addresses #58468 since the issue mentions the bug both in cp and cpSync, @jakecastelli could you solve the bug in both functions? 🙂 (or alternatively I can split the issue into two if you prefer?)

[jakecastelli]

Thanks for the pointer, I was a bit running late last night and didn't pay close attention to the cp repo code, I have fixed both places with newly added tests, and reverted 52430b9 🙏 PTAL

[dario-piotrowicz]

Looks great! thanks a lot again @jakecastelli 🙏

I just left a couple of comments about the tests comments, I hope those can be addressed before landing this 🙂🙏

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/67166/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/67183/

[nodejs-github-bot]

Landed in 0c6e16b...68bac3d

[aduh95]

test/parallel/test-fs-cp.mjs is crashing on v22.x-staging with this change

[dario-piotrowicz]

@aduh95 do you think we could/should just revert the change only for v22?

[aduh95]

@dario-piotrowicz it will be included only if we can build and the tests are passing. If it's not worth backporting, we should add the dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. instead

[dario-piotrowicz]

@dario-piotrowicz it will be included only if we can build and the tests are passing.

got it, thanks 🙂👍

If it's not worth backporting, we should add the dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. instead

Sorry you're right, I was asking more in the sense of this being a small bug fix, so if it were problematic in v22 we could avoid the backport, but yeah ideally it should land in v22 as well...