Remove contents of symlink to dir with rm -rf#688
Conversation
src/rm.js
Outdated
| try { | ||
| stats = fs.statSync(file); | ||
| } catch (e) { | ||
| // symlink is bad, so just remove it |
There was a problem hiding this comment.
Change this to something like "symlink is broken, so only delete the link, not its contents"
| var dirpath = path.resolve(path.dirname(link), dir); | ||
| var fromSymlink = true; | ||
| rmdirSyncRecursive(dirpath, options.force, fromSymlink); | ||
| } else { |
There was a problem hiding this comment.
I think you can ignore the options.recursive check and use handleDirectory instead.
Although this won't catch the "link ➡️ link ➡️ directory" case, it only catches the "link ➡️ directory" case.
There was a problem hiding this comment.
Also, do we really need to do all this work here? Does it really not work to call fs.readdirSync() on the link itself? That works for me when I tried on Node v7. Then we probably don't need the fromSymlink stuff.
There was a problem hiding this comment.
You're right, fs.readdirSync() works when passed symlinks, so that can be simplified. I think the fromSymlink logic still needs to be there, so that rmdirSyncRecursive knows not to delete the directory.
|
|
||
| // if was directory was referenced through a symbolic link, | ||
| // do not remove the directory itself | ||
| if (fromSymlink) return; |
There was a problem hiding this comment.
Can you specify that while we do not remove the directory itself, we do in fact remove its contents?
03602b2 to
accd3b2
Compare
|
@nfischer Can you help diagnose the failing test on windows? I don't have a windows machine to test with. |
|
@freitagbr probably has to do with how Windows handles sym links differently. I'd recommend skipping the tests on Windows. |
Codecov Report
@@ Coverage Diff @@
## master #688 +/- ##
==========================================
+ Coverage 94.33% 94.81% +0.48%
==========================================
Files 33 33
Lines 1183 1293 +110
==========================================
+ Hits 1116 1226 +110
Misses 67 67
Continue to review full report at Codecov.
|
Fix the behavior of
rmsuch that:rm('-rf', 'link_to_dir/')deletes contents of real dir, but keeps dir and symlinkrm('-rf', 'link_to_dir')deletes the symlink itselfFixes #587