Skip to content

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

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

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

Conversation

@nfischer

@nfischer nfischer commented Apr 6, 2016

Copy link
Copy Markdown
Member

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
Comment thread 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

ariporad commented Apr 6, 2016

Copy link
Copy Markdown
Contributor

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

@nfischer

nfischer commented Apr 6, 2016

Copy link
Copy Markdown
Member Author

Also, could you rebase this?

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

@ariporad

ariporad commented Apr 7, 2016

Copy link
Copy Markdown
Contributor

@nfischer: Alas, it does not 😢.

@nfischer

nfischer commented Apr 7, 2016

Copy link
Copy Markdown
Member Author

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

@ariporad

ariporad commented Apr 8, 2016

Copy link
Copy Markdown
Contributor

@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

nfischer commented Apr 8, 2016

Copy link
Copy Markdown
Member Author

Hmm ok. Does this look good now?

@ariporad

ariporad commented Apr 8, 2016

Copy link
Copy Markdown
Contributor

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