8000 Add tests for rm removing symlinks by freitagbr · Pull Request #849 · shelljs/shelljs · GitHub
[go: up one dir, main page]
Skip to content

Add tests for rm removing symlinks #849

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.

Sign up for GitHub

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Add tests for rm removing symlinks #849

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
64a682e
test(rm): add tests for removing symlinks
freitagbr May 10, 2018
c678839
test(rm): skip symlink test on windows
freitagbr May 10, 2018
1438bf1
Rename and move test, add assertions
freitagbr May 11, 2018
4167033
Add tests for rm -r/-f on links to dirs
freitagbr May 11, 2018
042d17c
Properly handle rm -r on links to dirs with a trailing slash
freitagbr Jul 12, 2018
4639ca9
Rephrase comment for clarity
freitagbr Jul 18, 2018
44ec920
Use -r in -r test
freitagbr Jul 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Properly handle rm -r on links to dirs with a trailing slash
  • Loading branch information
@freitagbr
freitagbr committed Jul 12, 2018
commit 042d17c0beb1514aad1e264639caa99236756334
7 changes: 6 additions & 1 deletion src/rm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,12 @@ function rmdirSyncRecursive(dir, force, fromSymlink) {

// if was directory was referenced through a symbolic link,
// the contents should be removed, but not the directory itself
if (fromSymlink) return;
if (fromSymlink) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand, the issue is that link_to_a_dir refers to something, but link_to_a_dir/ does not (because only directories may be optionally referred to with a trailing slash). But in both cases, link_to_a_dir references a directory, and so I would expect fromSymlink to be in true in both cases.

It sounds like we should rename the variable, or we should add a comment explaining what it actually represents.

if (!force) {
common.error("cannot remove '" + dir + "': Not a directory");
}
return;
}

// Now that we know everything in the sub-tree has been deleted, we can delete the main directory.
// Huzzah for the shopkeep.
Expand Down
1 change: 1 addition & 0 deletions test/rm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,7 @@ test('rm -r, symlink to a dir, trailing slash', t => {
const result = shell.rm('-r', `${t.context.tmp}/rm/link_to_a_dir/`);
t.truthy(shell.error());
t.is(result.code, 1);
t.is(result.stderr, `rm: cannot remove '${t.context.tmp}/rm/link_to_a_dir/': Not a directory`);
// Both the link and original dir should remain, but contents are deleted
t.truthy(fs.existsSync(`${t.context.tmp}/rm/link_to_a_dir`));
t.truthy(fs.existsSync(`${t.context.tmp}/rm/a_dir`));
Expand Down
0