Skip to content

refactor(cp): clean up code and fix #376#380

Merged
ariporad merged 1 commit intomasterfrom
fix-cp-r-option
Mar 6, 2016
Merged

refactor(cp): clean up code and fix #376#380
ariporad merged 1 commit intomasterfrom
fix-cp-r-option

Conversation

@nfischer
Copy link
Copy Markdown
Member

@nfischer nfischer commented Mar 1, 2016

Fixes issue in #376. Simplifies the code, and slight perf win.

This fixes the semantics of cp(), which had deviated from unix cp a bit. If anyone is available to review, that would be good.

Goals of this PR:

  • improve performance
  • improve correctness

@nfischer nfischer added fix Bug/defect, or a fix for such a problem perf bash compat Compatibility issues with bash or POSIX behavior labels Mar 1, 2016
@nfischer nfischer added this to the v0.7.0 milestone Mar 1, 2016
@nfischer
Copy link
Copy Markdown
Member Author

nfischer commented Mar 4, 2016

@ariporad can you take a look at this? Let me know what you think, or if there's any other perf improvements we can squeeze out.

shell.config.silent = true;

shell.rm('-rf', 'tmp');
shell.mkdir('tmp');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is this here? It seems completely unrelated.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

One of the tests assumed incorrect semantics of cp(). When I fixed the semantics, I realized this test was failing.

The directory actually gets created with a cp() command on this line

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ok.

@ariporad
Copy link
Copy Markdown
Contributor

ariporad commented Mar 6, 2016

@nfischer: One tiny nit, and could you please rebase off master? Otherwise LGTM.

@ariporad
Copy link
Copy Markdown
Contributor

ariporad commented Mar 6, 2016

@nfischer: Can you rebase of master? Otherwise LGTM!

Fixes issue in #376. Simplifies the code, and slight perf win.
@nfischer
Copy link
Copy Markdown
Member Author

nfischer commented Mar 6, 2016

@ariporad I just re-pushed

ariporad added a commit that referenced this pull request Mar 6, 2016
refactor(cp): clean up code and fix #376
@ariporad ariporad merged commit 5f4d390 into master Mar 6, 2016
@ariporad ariporad deleted the fix-cp-r-option branch March 6, 2016 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bash compat Compatibility issues with bash or POSIX behavior fix Bug/defect, or a fix for such a problem perf

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants