Skip to content

Commit 7feabda

Browse files
freitagbrfreitagbr
authored
Remove contents of symlink to dir with rm -rf (shelljs#688)
* rm -rf on a symlink to a dir deletes contents * Fix comment typo * Clarify comments * Pass symlink directly to rmdirSync * Skip rm -rf symlink test on windows
1 parent e12da9d commit 7feabda

2 files changed

Lines changed: 80 additions & 29 deletions

File tree

‎src/rm.js‎

Lines changed: 69 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ common.register('rm', _rm, {
1717
//
1818
// Licensed under the MIT License
1919
// http://www.opensource.org/licenses/mit-license.php
20-
function rmdirSyncRecursive(dir, force) {
20+
function rmdirSyncRecursive(dir, force, fromSymlink) {
2121
var files;
2222

2323
files = fs.readdirSync(dir);
@@ -43,6 +43,10 @@ function rmdirSyncRecursive(dir, force) {
4343
}
4444
}
4545

46+
// if was directory was referenced through a symbolic link,
47+
// the contents should be removed, but not the directory itself
48+
if (fromSymlink) return;
49+
4650
// Now that we know everything in the sub-tree has been deleted, we can delete the main directory.
4751
// Huzzah for the shopkeep.
4852

@@ -91,6 +95,57 @@ function isWriteable(file) {
9195
return writePermission;
9296
}
9397

98+
function handleFile(file, options) {
99+
if (options.force || isWriteable(file)) {
100+
// -f was passed, or file is writable, so it can be removed
101+
common.unlinkSync(file);
102+
} else {
103+
common.error('permission denied: ' + file, { continue: true });
104+
}
105+
}
106+
107+
function handleDirectory(file, options) {
108+
if (options.recursive) {
109+
// -r was passed, so directory can be removed
110+
rmdirSyncRecursive(file, options.force);
111+
} else {
112+
common.error('path is a directory', { continue: true });
113+
}
114+
}
115+
116+
function handleSymbolicLink(file, options) {
117+
var stats;
118+
try {
119+
stats = fs.statSync(file);
120+
} catch (e) {
121+
// symlink is broken, so remove the symlink itself
122+
common.unlinkSync(file);
123+
return;
124+
}
125+
126+
if (stats.isFile()) {
127+
common.unlinkSync(file);
128+
} else if (stats.isDirectory()) {
129+
if (file[file.length - 1] === '/') {
130+
// trailing separator, so remove the contents, not the link
131+
if (options.recursive) {
132+
// -r was passed, so directory can be removed
133+
var fromSymlink = true;
134+
rmdirSyncRecursive(file, options.force, fromSymlink);
135+
} else {
136+
common.error('path is a directory', { continue: true });
137+
}
138+
} else {
139+
// no trailing separator, so remove the link
140+
common.unlinkSync(file);
141+
}
142+
}
143+
}
144+
145+
function handleFIFO(file) {
146+
common.unlinkSync(file);
147+
}
148+
94149
//@
95150
//@ ### rm([options,] file [, file ...])
96151
//@ ### rm([options,] file_array)
@@ -115,9 +170,12 @@ function _rm(options, files) {
115170
files = [].slice.call(arguments, 1);
116171

117172
files.forEach(function (file) {
118-
var stats;
173+
var lstats;
119174
try {
120-
stats = fs.lstatSync(file); // test for existence
175+
var filepath = (file[file.length - 1] === '/')
176+
? file.slice(0, -1) // remove the '/' so lstatSync can detect symlinks
177+
: file;
178+
lstats = fs.lstatSync(filepath); // test for existence
121179
} catch (e) {
122180
// Path does not exist, no force flag given
123181
if (!options.force) {
@@ -127,22 +185,14 @@ function _rm(options, files) {
127185
}
128186

129187
// If here, path exists
130-
if (stats.isFile()) {
131-
if (options.force || isWriteable(file)) {
132-
// -f was passed, or file is writable, so it can be removed
133-
common.unlinkSync(file);
134-
} else {
135-
common.error('permission denied: ' + file, { continue: true });
136-
}
137-
} else if (stats.isDirectory()) {
138-
if (options.recursive) {
139-
// -r was passed, so directory can be removed
140-
rmdirSyncRecursive(file, options.force);
141-
} else {
142-
common.error('path is a directory', { continue: true });
143-
}
144-
} else if (stats.isSymbolicLink() || stats.isFIFO()) {
145-
common.unlinkSync(file);
188+
if (lstats.isFile()) {
189+
handleFile(file, options);
190+
} else if (lstats.isDirectory()) {
191+
handleDirectory(file, options);
192+
} else if (lstats.isSymbolicLink()) {
193+
handleSymbolicLink(file, options);
194+
} else if (lstats.isFIFO()) {
195+
handleFIFO(file);
146196
}
147197
}); // forEach(file)
148198
return '';

‎test/rm.js‎

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ test('recursive dir removal', t => {
150150
t.is(result.code, 0);
151151
const contents = fs.readdirSync(t.context.tmp);
152152
t.is(contents.length, 1);
153-
t.is(contents[0], '.hidden'); // shouldn't remove hiddden if no .* given
153+
t.is(contents[0], '.hidden'); // shouldn't remove hidden if no .* given
154154
});
155155

156156
test('recursive dir removal #2', t => {
@@ -267,16 +267,17 @@ test('remove symbolic link to a dir', t => {
267267
t.truthy(fs.existsSync(`${t.context.tmp}/rm/a_dir`));
268268
});
269269

270-
// TODO(nfischer): fix this behavior in rm
271-
test.skip('rm -rf on a symbolic link to a dir deletes its contents', t => {
272-
const result = shell.rm('-rf', `${t.context.tmp}/rm/link_to_a_dir/`);
273-
t.falsy(shell.error());
274-
t.is(result.code, 0);
270+
test('rm -rf on a symbolic link to a dir deletes its contents', t => {
271+
if (process.platform !== 'win32') {
272+
const result = shell.rm('-rf', `${t.context.tmp}/rm/link_to_a_dir/`);
273+
t.falsy(shell.error());
274+
t.is(result.code, 0);
275275

276-
// Both the link and original dir should remain, but contents are deleted
277-
t.truthy(fs.existsSync(`${t.context.tmp}/rm/link_to_a_dir`));
278-
t.truthy(fs.existsSync(`${t.context.tmp}/rm/a_dir`));
279-
t.falsy(fs.existsSync(`${t.context.tmp}/rm/a_dir/a_file`));
276+
// Both the link and original dir should remain, but contents are deleted
277+
t.truthy(fs.existsSync(`${t.context.tmp}/rm/link_to_a_dir`));
278+
t.truthy(fs.existsSync(`${t.context.tmp}/rm/a_dir`));
279+
t.falsy(fs.existsSync(`${t.context.tmp}/rm/a_dir/a_file`));
280+
}
280281
});
281282

282283
test('remove broken symbolic link', t => {

0 commit comments

Comments
 (0)