Repository navigation
fix(cp): detect same file reached through symlinks, junctions or hard links - #1274
Open
White0xdi3 wants to merge 1 commit into
Open
White0xdi3 wants to merge 1 commit into
White0xdi3 wants to merge 1 commit into
Conversation
… links The same-file check added in shelljs#1241 compares paths with path.relative(), which is purely lexical. When two different paths reach the same file (e.g. via a symlinked or junctioned directory, or a hard link), cp() opened the destination with 'w' and truncated the source to 0 bytes while reporting success. Compare device and inode (falling back to realpath) before opening the destination, both for single files and for the recursive directory case. Symlinks that are copied as links (-P) are compared as directory entries, so copying one link onto another that points to the same target keeps working.
nfischer
reviewed
Oct 3, 2026
| // If followLinks is false, a and b are compared as directory entries (lstat). | ||
| function isSameFile(a, b, followLinks) { | ||
| try { | ||
| var statFn = followLinks ? fs.statSync : fs.lstatSync; |
Member
There was a problem hiding this comment.
How about using common.statFollowLinks()?
Line 396 in f364da6
| // reached through different paths (symlinked or junctioned directories, hard | ||
| // links). path.relative() only compares the strings, so it cannot tell. | ||
| // If followLinks is false, a and b are compared as directory entries (lstat). | ||
| function isSameFile(a, b, followLinks) { |
Member
There was a problem hiding this comment.
The documentation for this method seems contradictory. You say that this tests if files are really the same on disk, but then you offer an option for followLinks which lets a caller turn off the documented behavior.
Why do we need the case for followLinks == false?
|
|
||
| // Directory symlinks use junctions on Windows, which do not need admin rights | ||
| function linkDir(target, linkPath) { | ||
| fs.symlinkSync(path.resolve(target), linkPath, 'junction'); |
Member
There was a problem hiding this comment.
Can we achieve the same with shell.ln()?
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1274 +/- ##
==========================================
+ Coverage 97.10% 97.12% +0.02%
==========================================
Files 36 36
Lines 1518 1530 +12
==========================================
+ Hits 1474 1486 +12
Misses 44 44 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The same-file check from #1241 uses
path.relative(), which only compares strings. When two paths reach the same file through a symlinked or junctioned directory, or a hard link,cp()opens the destination with'w'and truncates the source to 0 bytes while returning success.Repro on current
main(real/parent/foo/x.txtexists,real/link_parentis a symlink or junction toreal/parent):This PR compares device and inode (falling back to
realpath) before the destination is opened for writing, for single files and for the recursive directory case. Symlinks that are copied as links (-P) are compared as directory entries, socp -P link1 link2with a shared target keeps working.Tests added in
test/cp.js:cp -Pof a symlink onto another symlink with the same target (guards against a false positive)Local run:
npx ava test/cp.jspasses except 3 existing symlink tests (Test max depth.,cp -L follows symlinks,Test with recursive option and symlinks.). They fail identically onmainon my Windows machine, which has no admin rights for file symlinks. I could not run the suite on Linux or macOS, so please rely on CI for those.