8000 Added verbosity flag (-v) for cp, mv and rm commands by nicky1038 · Pull Request #1129 · shelljs/shelljs · GitHub
[go: up one dir, main page]
Skip to content

Added verbosity flag (-v) for cp, mv and rm commands #1129

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 1 commit into
base: main
Choose a base branch
from
Open

Added verbosity flag (-v) for cp, mv and rm commands #1129

Show file tree
Hide file tree
Changes from all commits 8000
Commits
Show all changes
1 commit
Select commit
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
8 changes: 8 additions & 0 deletions src/cp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ common.register('cp', _cp, {
'L': 'followsymlink',
'P': 'noFollowsymlink',
'p': 'preserve',
'v': 'verbose',
},
wrapOutput: false,
});
Expand Down Expand Up @@ -84,6 +85,10 @@ function copyFileSync(srcFile, destFile, options) {
fs.closeSync(fdr);
fs.closeSync(fdw);
}

if (options.verbose) {
console.log("copied '" + srcFile + "' -> '" + destFile + "'");
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should drop the "copied" prefix. When I tested this on my linux machine, I don't see this in the output:

$ cp -vr src/ dest/
'src/' -> 'dest/'
'src/file.txt' -> 'dest/file.txt'
'src/sub1' -> 'dest/sub1'
'src/sub1/sub2' -> 'dest/sub1/sub2'
'src/sub1/sub2/file2.txt' -> 'dest/sub1/sub2/file2.txt'

On the other hand, keeping "copied" would be more consistent with the other commands, since those do say "removed" or "renamed" before the file name.

What do you think?

}
}

// Recursively copies 'sourceDir' into 'destDir'
Expand All @@ -108,6 +113,9 @@ function cpdirSyncRecursive(sourceDir, destDir, currentDepth, opts) {
var checkDir = common.statFollowLinks(sourceDir);
try {
fs.mkdirSync(destDir);
if (opts.verbose) {
console.log("created directory '" + sourceDir + "' -> '" + destDir + "'");
}
} catch (e) {
// if the directory already exists, that's okay
if (e.code !== 'EEXIST') throw e;
Expand Down
8 changes: 6 additions & 2 deletions src/mv.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ common.register('mv', _mv, {
cmdOptions: {
'f': '!no_force',
'n': 'no_force',
'v': 'verbose',
},
});

Expand Down Expand Up @@ -102,15 +103,18 @@ function _mv(options, sources, dest) {

try {
fs.renameSync(src, thisDest);
if (options.verbose) {
console.log("renamed '" + src + "' -> '" + thisDest + "'");
}
} catch (e) {
/* istanbul ignore next */
if (e.code === 'EXDEV') {
// If we're trying to `mv` to an external partition, we'll actually need
// to perform a copy and then clean up the original file. If either the
// copy or the rm fails with an exception, we should allow this
// exception to pass up to the top level.
cp({ recursive: true }, src, thisDest);
rm({ recursive: true, force: true }, src);
cp({ recursive: true, verbose: options.verbose }, src, thisDest);
rm({ recursive: true, force: true, verbose: options.verbose }, src);
8000 Copy link
Member

Choose a reason for hiding this comment

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

Good catch! 😄

}
}
}); // forEach(src)
Expand Down
38 changes: 29 additions & 9 deletions src/rm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ common.register('rm', _rm, {
'f': 'force',
'r': 'recursive',
'R': 'recursive',
'v': 'verbose',
},
});

Expand All @@ -17,7 +18,7 @@ common.register('rm', _rm, {
//
// Licensed under the MIT License
// http://www.opensource.org/licenses/mit-license.php
function rmdirSyncRecursive(dir, force, fromSymlink) {
function rmdirSyncRecursive(dir, force, verbose, fromSymlink) {
var files;

files = fs.readdirSync(dir);
Expand All @@ -28,11 +29,14 @@ function rmdirSyncRecursive(dir, force, fromSymlink) {
var currFile = common.statNoFollowLinks(file);

if (currFile.isDirectory()) { // Recursive function back to the beginning
rmdirSyncRecursive(file, force);
rmdirSyncRecursive(file, force, verbose);
} else if (force || isWriteable(file)) {
// Assume it's a file - perhaps a try/catch belongs here?
try {
common.unlinkSync(file);
if (verbose) {
console.log("removed '" + file + "'");
}
} catch (e) {
/* istanbul ignore next */
common.error('could not remove file (code ' + e.code + '): ' + file, {
Expand All @@ -59,6 +63,9 @@ function rmdirSyncRecursive(dir, force, fromSymlink) {
try {
result = fs.rmdirSync(dir);
if (fs.existsSync(dir)) throw { code: 'EAGAIN' };
if (verbose) {
console.log("removed directory'" + dir + "'");
Copy link
Member

Choose a reason for hiding this comment

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

Add a space between directory and '

8000

}
break;
} catch (er) {
/* istanbul ignore next */
Expand Down Expand Up @@ -98,6 +105,9 @@ function handleFile(file, options) {
if (options.force || isWriteable(file)) {
// -f was passed, or file is writable, so it can be removed
common.unlinkSync(file);
if (options.verbose) {
console.log("removed '" + file + "'");
}
} else {
common.error('permission denied: ' + file, { continue: true });
}
Expand All @@ -106,43 +116,53 @@ function handleFile(file, options) {
function handleDirectory(file, options) {
if (options.recursive) {
// -r was passed, so directory can be removed
rmdirSyncRecursive(file, options.force);
rmdirSyncRecursive(file, options.force, options.verbose);
} else {
common.error('path is a directory', { continue: true });
}
}

function handleSymbolicLink(file, options) {
function _unlink() {
common.unlinkSync(file);
if (options.verbose) {
console.log("removed '" + file + "'");
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you change this so that file is a method parameter? I'd also suggest passing options as a second parameter and then moving this function definition to the global scope, similar to what you've done with handleFIFO().

}

var stats;
try {
stats = common.statFollowLinks(file);
} catch (e) {
// symlink is broken, so remove the symlink itself
common.unlinkSync(file);
_unlink();
return;
}

if (stats.isFile()) {
common.unlinkSync(file);
_unlink();
} else if (stats.isDirectory()) {
if (file[file.length - 1] === '/') {
// trailing separator, so remove the contents, not the link
if (options.recursive) {
// -r was passed, so directory can be removed
var fromSymlink = true;
rmdirSyncRecursive(file, options.force, fromSymlink);
rmdirSyncRecursive(file, options.force, options.verbose, fromSymlink);
} else {
common.error('path is a directory', { continue: true });
}
} else {
// no trailing separator, so remove the link
common.unlinkSync(file);
_unlink();
}
}
}

function handleFIFO(file) {
function handleFIFO(file, options) {
common.unlinkSync(file);
if (options.verbose) {
console.log("removed '" + file + "'");
}
}

//@
Expand Down Expand Up @@ -193,7 +213,7 @@ function _rm(options, files) {
} else if (lstats.isSymbolicLink()) {
handleSymbolicLink(file, options);
} else if (lstats.isFIFO()) {
handleFIFO(file);
handleFIFO(file, options);
}
}); // forEach(file)
return '';
Expand Down
0