Skip to content
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

Jump to bottom

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

Open
wants to merge 1 commit into
base: master
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
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/cp.js
View file
Open in desktop
Expand Up @@ -12,6 +12,7 @@
'L': 'followsymlink',
'P': 'noFollowsymlink',
'p': 'preserve',
'v': 'verbose',
},
wrapOutput: false,
});
Expand Down Expand Up @@ -84,6 +85,10 @@
fs.closeSync(fdr);
fs.closeSync(fdw);
}

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

Check warning on line 90 in src/cp.js

View check run for this annotation

Codecov / codecov/patch

src/cp.js#L90 

Added line #L90 was not covered by tests
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 @@
var checkDir = common.statFollowLinks(sourceDir);
try {
fs.mkdirSync(destDir);
if (opts.verbose) {
console.log("created directory '" + sourceDir + "' -> '" + destDir + "'");

Check warning on line 117 in src/cp.js

View check run for this annotation

Codecov / codecov/patch

src/cp.js#L117 

Added line #L117 was not covered by tests
}
} 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
Expand Up @@ -8,6 +8,7 @@
cmdOptions: {
'f': '!no_force',
'n': 'no_force',
'v': 'verbose',
},
});

Expand Down Expand Up @@ -102,15 +103,18 @@

try {
fs.renameSync(src, thisDest);
if (options.verbose) {
console.log("renamed '" + src + "' -> '" + thisDest + "'");

Check warning on line 107 in src/mv.js

View check run for this annotation

Codecov / codecov/patch

src/mv.js#L107 

Added line #L107 was not covered by tests
}
} 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);
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
Expand Up @@ -6,6 +6,7 @@
'f': 'force',
'r': 'recursive',
'R': 'recursive',
'v': 'verbose',
},
});

Expand All @@ -17,7 +18,7 @@
//
// 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 @@
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 + "'");

Check warning on line 38 in src/rm.js

View check run for this annotation

Codecov / codecov/patch

src/rm.js#L38 

Added line #L38 was not covered by tests
}
} catch (e) {
/* istanbul ignore next */
common.error('could not remove file (code ' + e.code + '): ' + file, {
Expand All @@ -59,6 +63,9 @@
try {
result = fs.rmdirSync(dir);
if (fs.existsSync(dir)) throw { code: 'EAGAIN' };
if (verbose) {
console.log("removed directory'" + dir + "'");

Check warning on line 67 in src/rm.js

View check run for this annotation

Codecov / codecov/patch

src/rm.js#L67 

Added line #L67 was not covered by tests
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 '

}
break;
} catch (er) {
/* istanbul ignore next */
Expand Down Expand Up @@ -98,6 +105,9 @@
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 + "'");

Check warning on line 109 in src/rm.js

View check run for this annotation

Codecov / codecov/patch

src/rm.js#L109 

Added line #L109 was not covered by tests
}
} else {
common.error('permission denied: ' + file, { continue: true });
}
Expand All @@ -106,43 +116,53 @@
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 + "'");

Check warning on line 129 in src/rm.js

View check run for this annotation

Codecov / codecov/patch

src/rm.js#L129 

Added line #L129 was not covered by tests
}
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();

Check warning on line 143 in src/rm.js

View check run for this annotation

Codecov / codecov/patch

src/rm.js#L143 

Added line #L143 was not covered by tests
} 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 + "'");

Check warning on line 164 in src/rm.js

View check run for this annotation

Codecov / codecov/patch

src/rm.js#L164 

Added line #L164 was not covered by tests
}
}

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