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.

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

Fix cp to match unix behavior #271

Merged
merged 3 commits into from Jan 16, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 5 additions & 1 deletion src/cp.js
Expand Up @@ -140,8 +140,12 @@ function _cp(options, sources, dest) {
// Recursive allows the shortcut syntax "sourcedir/" for "sourcedir/*"
// (see Github issue #15)
sources.forEach(function(src, i) {
if (src[src.length - 1] === '/')
if (src[src.length - 1] === '/') {
sources[i] += '*';
// If src is a directory and dest doesn't exist, 'cp -r src dest' should copy src/* into dest
} else if (fs.statSync(src).isDirectory() && !exists) {
sources[i] += '/*';
}
});

// Create dest
Expand Down
10 changes: 8 additions & 2 deletions test/cp.js
Expand Up @@ -125,17 +125,23 @@ assert.equal(shell.cat('resources/cp/dir_a/z'), shell.cat('tmp/cp/dir_a/z')); //

//recursive, creates dest dir since it's only one level deep (see Github issue #44)
shell.rm('-rf', 'tmp/*');
shell.cp('-r', 'resources/issue44/*', 'tmp/dir2');
shell.cp('-r', 'resources/issue44', 'tmp/dir2');
Copy link
Member

Choose a reason for hiding this comment

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

Should these test cases be changed? What was wrong with the previous behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In *nix, cp -r resources/issue44/* tmp/dir2 is not valid. If there is only one file in resources/issue44, it will be copied into tmp and be renamed dir2. If there is more than one file in resources/issue44, the command will fail.

The intention here is to copy all of the files in resources/issue44 to tmp/dir2, where dir2 will be created. This is achieved with the new command (and in *nix as well).

Copy link
Member

Choose a reason for hiding this comment

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

Awesome explanation! The logic sounds right to me. It sounds like this is indeed a bug in the test. Thanks for fixing!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely!

assert.equal(shell.error(), null);
assert.equal(shell.ls('-R', 'resources/issue44') + '', shell.ls('-R', 'tmp/dir2') + '');
assert.equal(shell.cat('resources/issue44/main.js'), shell.cat('tmp/dir2/main.js'));

//recursive, does *not* create dest dir since it's too deep (see Github issue #44)
shell.rm('-rf', 'tmp/*');
shell.cp('-r', 'resources/issue44/*', 'tmp/dir2/dir3');
shell.cp('-r', 'resources/issue44', 'tmp/dir2/dir3');
Copy link
Member

Choose a reason for hiding this comment

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

Same question applies here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, cp -r resources/issue44/* tmp/dir2/dir3 is not valid.

assert.ok(shell.error());
assert.equal(fs.existsSync('tmp/dir2'), false);

//recursive, creates dest dir, implicitly copies contents of source dir
shell.rm('-rf', 'tmp/*');
shell.cp('-r', 'resources/cp/dir_a', 'tmp/dest');
assert.equal(shell.error(), null);
assert.equal(fs.existsSync('tmp/dest/z'), true);

//preserve mode bits
shell.rm('-rf', 'tmp/*');
var execBit = parseInt('001', 8);
Expand Down
1 change: 1 addition & 0 deletions test/rm.js
Expand Up @@ -179,6 +179,7 @@ assert.equal(fs.existsSync('tmp/tree4'), false);

// remove symbolic link to a dir
shell.rm('-rf', 'tmp');
shell.mkdir('tmp');
shell.cp('-R', 'resources/rm', 'tmp');
shell.rm('-f', 'tmp/rm/link_to_a_dir');
assert.equal(shell.error(), null);
Expand Down