Skip to content

feat(cp): -P option, plus better handling of symlinks#421

Merged
ariporad merged 2 commits intomasterfrom
feat-cp-p-option
Apr 8, 2016
Merged

feat(cp): -P option, plus better handling of symlinks#421
ariporad merged 2 commits intomasterfrom
feat-cp-p-option

Conversation

@nfischer
Copy link
Copy Markdown
Member

@nfischer nfischer commented Apr 6, 2016

Fixes #413

Ok, so this wound up being a bigger PR than I anticipated. I originally wanted to implement the -P option for cp(), but I realized that ShellJS didn't have very good semantics for handling links. So this PR fixes a variety of bugs surrounding behavior of links.

  • rm('badlink') used to not work, now it does
  • cp('link', 'dest') will follow the link
  • cp('-R', 'link', 'dest') will not follow the link (regardless of whether it points to a directory or not)
  • cp('-P', 'link', 'dest') will of course not follow the link (regardless of whether or not -R is used)
  • touch('badlink') creates the file it points to (as touch should)
  • mkdir('badlink') outputs an error message instead of crashing
  • copying a directory that has links within it works properly (used to break)

@nfischer nfischer added feature fix Bug/defect, or a fix for such a problem medium priority labels Apr 6, 2016
@nfischer nfischer added this to the v0.7.0 milestone Apr 6, 2016
src/cp.js Outdated
}
var srcStat = fs.statSync(src);
if (srcStat.isDirectory()) {
if ((!options.noFollowsymlink) && srcStat.isDirectory()) {
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.

Really nitty nitpick: The parens are redundant.

@ariporad
Copy link
Copy Markdown
Contributor

ariporad commented Apr 6, 2016

One tiny nit, otherwise looks good. Also, could you rebase this?

@nfischer
Copy link
Copy Markdown
Member Author

nfischer commented Apr 6, 2016

Also, could you rebase this?

I think the new squash-merge takes care of that, right?

@ariporad
Copy link
Copy Markdown
Contributor

ariporad commented Apr 7, 2016

@nfischer: Alas, it does not 😢.

@nfischer
Copy link
Copy Markdown
Member Author

nfischer commented Apr 7, 2016

Oh, where does it put the one PR commit then if not on top of master?

@ariporad
Copy link
Copy Markdown
Contributor

ariporad commented Apr 8, 2016

@nfischer: It gives you two options:

  1. Rebase your commit.
  2. Merge (not rebase, merge), master into your branch, them rebase master onto your branch.

@nfischer
Copy link
Copy Markdown
Member Author

nfischer commented Apr 8, 2016

Hmm ok. Does this look good now?

@ariporad
Copy link
Copy Markdown
Contributor

ariporad commented Apr 8, 2016

LGTM!

@ariporad ariporad merged commit 56fbf5c into master Apr 8, 2016
@ariporad ariporad deleted the feat-cp-p-option branch April 8, 2016 22:34
@nfischer nfischer mentioned this pull request Apr 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature fix Bug/defect, or a fix for such a problem medium priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for cp -P option

2 participants