From 64a682e1923b0d4fc32aa3178f5f1caa6848daf0 Mon Sep 17 00:00:00 2001 From: Brandon Freitag Date: Wed, 9 May 2018 22:03:48 -0700 Subject: [PATCH 1/7] test(rm): add tests for removing symlinks --- test/rm.js | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/test/rm.js b/test/rm.js index ccd2c6ba..6d77fa09 100644 --- a/test/rm.js +++ b/test/rm.js @@ -57,6 +57,15 @@ test('invalid option', t => { t.is(result.stderr, 'rm: option not recognized: @'); }); +test('remove symbolic link to a dir without -r fails', t => { + const result = shell.rm(`${t.context.tmp}/rm/link_to_a_dir/`); + t.truthy(shell.error()); + t.is(result.code, 1); + t.is(result.stderr, 'rm: path is a directory'); + t.truthy(fs.existsSync(`${t.context.tmp}/rm/link_to_a_dir`)); + t.truthy(fs.existsSync(`${t.context.tmp}/rm/a_dir`)); +}); + // // Valids // @@ -308,3 +317,14 @@ test('remove fifo', t => { t.is(result.code, 0); }); }); + +test('remove symbolic link', t => { + utils.skipOnWin(t, () => { + t.truthy(shell.test('-L', `${t.context.tmp}/link`)); + const result = shell.rm(`${t.context.tmp}/link`); + t.falsy(shell.error()); + t.is(result.code, 0); + t.falsy(shell.test('-L', `${t.context.tmp}/link`)); + t.falsy(fs.existsSync(`${t.context.tmp}/link`)); + }); +}); From c678839233cc89d9f76bf0058ae941529f1158a5 Mon Sep 17 00:00:00 2001 From: Brandon Freitag Date: Wed, 9 May 2018 22:28:41 -0700 Subject: [PATCH 2/7] test(rm): skip symlink test on windows --- test/rm.js | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/test/rm.js b/test/rm.js index 6d77fa09..af1f9e32 100644 --- a/test/rm.js +++ b/test/rm.js @@ -58,12 +58,14 @@ test('invalid option', t => { }); test('remove symbolic link to a dir without -r fails', t => { - const result = shell.rm(`${t.context.tmp}/rm/link_to_a_dir/`); - t.truthy(shell.error()); - t.is(result.code, 1); - t.is(result.stderr, 'rm: path is a directory'); - t.truthy(fs.existsSync(`${t.context.tmp}/rm/link_to_a_dir`)); - t.truthy(fs.existsSync(`${t.context.tmp}/rm/a_dir`)); + utils.skipOnWin(t, () => { + const result = shell.rm(`${t.context.tmp}/rm/link_to_a_dir/`); + t.truthy(shell.error()); + t.is(result.code, 1); + t.is(result.stderr, 'rm: path is a directory'); + t.truthy(fs.existsSync(`${t.context.tmp}/rm/link_to_a_dir`)); + t.truthy(fs.existsSync(`${t.context.tmp}/rm/a_dir`)); + }); }); // From 1438bf1e0e96fa21d8cf09e81785d0dcc2a8365f Mon Sep 17 00:00:00 2001 From: Brandon Freitag Date: Thu, 10 May 2018 22:27:09 -0700 Subject: [PATCH 3/7] Rename and move test, add assertions Renames 'remove symbolic link' test to 'remove symbolic link to a file', moves the test near a related test, and adds assertions to the test 'remove symbolic link to a dir'. --- test/rm.js | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/test/rm.js b/test/rm.js index af1f9e32..1b1475b0 100644 --- a/test/rm.js +++ b/test/rm.js @@ -270,10 +270,22 @@ test( } ); +test('remove symbolic link to a file', t => { + t.truthy(shell.test('-L', `${t.context.tmp}/link`)); + const result = shell.rm(`${t.context.tmp}/link`); + t.falsy(shell.error()); + t.is(result.code, 0); + t.falsy(shell.test('-L', `${t.context.tmp}/link`)); + t.falsy(fs.existsSync(`${t.context.tmp}/link`)); + t.truthy(fs.existsSync(`${t.context.tmp}/file1`)); +}); + test('remove symbolic link to a dir', t => { + t.truthy(shell.test('-L', `${t.context.tmp}/rm/link_to_a_dir`)); const result = shell.rm(`${t.context.tmp}/rm/link_to_a_dir`); t.falsy(shell.error()); t.is(result.code, 0); + t.falsy(shell.test('-L', `${t.context.tmp}/rm/link_to_a_dir`)); t.falsy(fs.existsSync(`${t.context.tmp}/rm/link_to_a_dir`)); t.truthy(fs.existsSync(`${t.context.tmp}/rm/a_dir`)); }); @@ -319,14 +331,3 @@ test('remove fifo', t => { t.is(result.code, 0); }); }); - -test('remove symbolic link', t => { - utils.skipOnWin(t, () => { - t.truthy(shell.test('-L', `${t.context.tmp}/link`)); - const result = shell.rm(`${t.context.tmp}/link`); - t.falsy(shell.error()); - t.is(result.code, 0); - t.falsy(shell.test('-L', `${t.context.tmp}/link`)); - t.falsy(fs.existsSync(`${t.context.tmp}/link`)); - }); -}); From 416703357c36462c97b84e3da6c74a4b87056fb7 Mon Sep 17 00:00:00 2001 From: Brandon Freitag Date: Thu, 10 May 2018 23:28:01 -0700 Subject: [PATCH 4/7] Add tests for rm -r/-f on links to dirs --- test/rm.js | 71 ++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 58 insertions(+), 13 deletions(-) diff --git a/test/rm.js b/test/rm.js index 1b1475b0..05a37db4 100644 --- a/test/rm.js +++ b/test/rm.js @@ -35,11 +35,14 @@ test('file does not exist', t => { t.is(result.stderr, 'rm: no such file or directory: asdfasdf'); }); -test('cannot delete a directoy without recursive flag', t => { +test('cannot delete a directory without recursive flag', t => { const result = shell.rm(`${t.context.tmp}/rm`); t.truthy(shell.error()); t.is(result.code, 1); t.is(result.stderr, 'rm: path is a directory'); + t.truthy(fs.existsSync(`${t.context.tmp}/rm`)); + const contents = fs.readdirSync(t.context.tmp); + t.true(contents.length > 0); }); test('only an option', t => { @@ -57,16 +60,23 @@ test('invalid option', t => { t.is(result.stderr, 'rm: option not recognized: @'); }); -test('remove symbolic link to a dir without -r fails', t => { - utils.skipOnWin(t, () => { - const result = shell.rm(`${t.context.tmp}/rm/link_to_a_dir/`); - t.truthy(shell.error()); - t.is(result.code, 1); - t.is(result.stderr, 'rm: path is a directory'); - t.truthy(fs.existsSync(`${t.context.tmp}/rm/link_to_a_dir`)); - t.truthy(fs.existsSync(`${t.context.tmp}/rm/a_dir`)); - }); -}); +test( + 'cannot remove a symbolic link to a directory with trailing slash without -r flag', + t => { + utils.skipOnWin(t, () => { + // the trailing slash signifies that we want to delete the source + // directory and its contents, which can only be done with the -r flag + const result = shell.rm(`${t.context.tmp}/rm/link_to_a_dir/`); + t.truthy(shell.error()); + t.is(result.code, 1); + t.is(result.stderr, 'rm: path is a directory'); + t.truthy(fs.existsSync(`${t.context.tmp}/rm/link_to_a_dir`)); + t.truthy(fs.existsSync(`${t.context.tmp}/rm/a_dir`)); + const contents = fs.readdirSync(`${t.context.tmp}/rm/a_dir`); + t.true(contents.length > 0); + }); + } +); // // Valids @@ -290,12 +300,23 @@ test('remove symbolic link to a dir', t => { t.truthy(fs.existsSync(`${t.context.tmp}/rm/a_dir`)); }); -test('rm -rf on a symbolic link to a dir deletes its contents', t => { +test('rm -r, symlink to a dir, trailing slash', t => { + utils.skipOnWin(t, () => { + const result = shell.rm('-r', `${t.context.tmp}/rm/link_to_a_dir/`); + t.truthy(shell.error()); + t.is(result.code, 1); + // 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`)); + t.falsy(fs.existsSync(`${t.context.tmp}/rm/a_dir/a_file`)); + }); +}); + +test('rm -rf, symlink to a dir, trailing slash', t => { utils.skipOnWin(t, () => { const result = shell.rm('-rf', `${t.context.tmp}/rm/link_to_a_dir/`); t.falsy(shell.error()); t.is(result.code, 0); - // 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`)); @@ -303,6 +324,30 @@ test('rm -rf on a symbolic link to a dir deletes its contents', t => { }); }); +test('rm -r, symlink to a dir, no trailing slash', t => { + utils.skipOnWin(t, () => { + const result = shell.rm('-rf', `${t.context.tmp}/rm/link_to_a_dir`); + t.falsy(shell.error()); + t.is(result.code, 0); + // The link should be deleted, but the dir and contents remain + t.falsy(fs.existsSync(`${t.context.tmp}/rm/link_to_a_dir`)); + t.truthy(fs.existsSync(`${t.context.tmp}/rm/a_dir`)); + t.truthy(fs.existsSync(`${t.context.tmp}/rm/a_dir/a_file`)); + }); +}); + +test('rm -rf, symlink to a dir, no trailing slash', t => { + utils.skipOnWin(t, () => { + const result = shell.rm('-rf', `${t.context.tmp}/rm/link_to_a_dir`); + t.falsy(shell.error()); + t.is(result.code, 0); + // The link should be deleted, but the dir and contents remain + t.falsy(fs.existsSync(`${t.context.tmp}/rm/link_to_a_dir`)); + t.truthy(fs.existsSync(`${t.context.tmp}/rm/a_dir`)); + t.truthy(fs.existsSync(`${t.context.tmp}/rm/a_dir/a_file`)); + }); +}); + test('remove broken symbolic link', t => { utils.skipOnWin(t, () => { t.truthy(shell.test('-L', `${t.context.tmp}/rm/fake.lnk`)); From 042d17c0beb1514aad1e264639caa99236756334 Mon Sep 17 00:00:00 2001 From: Brandon Freitag Date: Wed, 11 Jul 2018 22:23:51 -0700 Subject: [PATCH 5/7] Properly handle rm -r on links to dirs with a trailing slash --- src/rm.js | 7 ++++++- test/rm.js | 1 + 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/rm.js b/src/rm.js index 90409ac6..f6dde795 100644 --- a/src/rm.js +++ b/src/rm.js @@ -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) { + 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. diff --git a/test/rm.js b/test/rm.js index 05a37db4..63fb2459 100644 --- a/test/rm.js +++ b/test/rm.js @@ -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`)); From 4639ca9f10b1973f88aed2acad933e7bfa2d3ccf Mon Sep 17 00:00:00 2001 From: Brandon Freitag Date: Tue, 17 Jul 2018 22:21:15 -0700 Subject: [PATCH 6/7] Rephrase comment for clarity --- test/rm.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/rm.js b/test/rm.js index 63fb2459..84f3187b 100644 --- a/test/rm.js +++ b/test/rm.js @@ -64,8 +64,9 @@ test( 'cannot remove a symbolic link to a directory with trailing slash without -r flag', t => { utils.skipOnWin(t, () => { - // the trailing slash signifies that we want to delete the source - // directory and its contents, which can only be done with the -r flag + // the trailing slash signifies that we want to delete the contents of the source directory, + // without removing the source directory itself or the link, which can only be done with the + // -r flag const result = shell.rm(`${t.context.tmp}/rm/link_to_a_dir/`); t.truthy(shell.error()); t.is(result.code, 1); From 44ec92008592d370be323b015752c645d02fdfb8 Mon Sep 17 00:00:00 2001 From: Brandon Freitag Date: Tue, 17 Jul 2018 22:34:53 -0700 Subject: [PATCH 7/7] Use -r in -r test --- test/rm.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/rm.js b/test/rm.js index 84f3187b..0c92f0af 100644 --- a/test/rm.js +++ b/test/rm.js @@ -328,7 +328,7 @@ test('rm -rf, symlink to a dir, trailing slash', t => { test('rm -r, symlink to a dir, no trailing slash', t => { utils.skipOnWin(t, () => { - const result = shell.rm('-rf', `${t.context.tmp}/rm/link_to_a_dir`); + const result = shell.rm('-r', `${t.context.tmp}/rm/link_to_a_dir`); t.falsy(shell.error()); t.is(result.code, 0); // The link should be deleted, but the dir and contents remain