Skip to content

Fix cp to match unix behavior#271

Merged
ariporad merged 3 commits intoshelljs:masterfrom
freitagbr:fix-cp-behavior
Jan 16, 2016
Merged

Fix cp to match unix behavior#271
ariporad merged 3 commits intoshelljs:masterfrom
freitagbr:fix-cp-behavior

Conversation

@freitagbr
Copy link
Copy Markdown
Contributor

Fixes #256, #101.

@nfischer
Copy link
Copy Markdown
Member

nfischer commented Jan 6, 2016

It might be helpful to add unit tests for this behavior. When I ran a quick test, it seemed to work as expected.

@ariporad ariporad added the fix Bug/defect, or a fix for such a problem label Jan 9, 2016
@nfischer
Copy link
Copy Markdown
Member

@freitagbr Could you rebase off master?

Brandon Freitag and others added 2 commits January 11, 2016 01:32
In unix, if src is a directory and dest doesn't exist, 'cp -r src dest' will copy src/* into dest. Fix cp('-r', 'src', 'dest') to behave the same way.
@freitagbr
Copy link
Copy Markdown
Contributor Author

@nfischer Done.

@nfischer
Copy link
Copy Markdown
Member

Looks like some of the tests are still failing. If we could get these fixed (or have the tests adjusted, if appropriate), that'd be good

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Definitely!

@nfischer
Copy link
Copy Markdown
Member

LGTM! @ariporad

ariporad added a commit that referenced this pull request Jan 16, 2016
@ariporad ariporad merged commit 2c63ecf into shelljs:master Jan 16, 2016
@freitagbr freitagbr deleted the fix-cp-behavior branch January 26, 2016 04:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Bug/defect, or a fix for such a problem

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants