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

cp -Ru respects the -R but not the -u #808

Closed
jaawerth opened this issue Nov 18, 2017 · 10 comments
Closed

cp -Ru respects the -R but not the -u #808

jaawerth opened this issue Nov 18, 2017 · 10 comments
Assignees
Milestone

Comments

@jaawerth
Copy link

@jaawerth jaawerth commented Nov 18, 2017

Love the lib - was looking to use this to reduce copy times on builds, but ran into the following:

Node version (or tell us if you're using electron or some other framework):

8.6.0

ShellJS version (the most recent version/Github branch you see the bug on):

shelljs 0.7.8, tested both directly and via shx 0.2.2

Operating system:

Arch Linux (kernel 4.13.12-1)

Description of the bug:

When combining recursive -R and update -u options in both shelljs and shx, "recursive" overrides and ignores "update" - files are recursively copied regardless of whether the src files are newer than dist.

Example ShellJS command to reproduce the error:

Test case (assuming src/static and dist/static have contents)

const shell = require('shelljs');

// overwrite dist/static, verify that all timestamps in dist are now newer than those in src
shell.cp('-R', 'src/static', 'dist/');
console.log(shell.ls('-l', 'src/static').stdout, '\n' + shell.ls('-l', 'dist/static').stdout);
console.log('----------');

setTimeout(() => {
  shell.cp('-Ru', 'src/static/', 'dist/');
  console.log(shell.ls('-l', 'dist/static').stdout);

  // timestamps shouldn't change, but are ~1s newer
}, 1000);

Expected result

Same as

cp -R src/static/ dist/ && ls -l src/static && ls -l dist/static
echo '\n-----\n'
sleep 1
cp -Ru src/static/ dist/ && ls -l dist/static

I think I see where the fix is - just wanted to file this before I forgot in case I don't end up having time in the near future to create a PR.

@freitagbr
Copy link
Contributor

@freitagbr freitagbr commented Nov 22, 2017

Hi @jaawerth, thanks for reporting this. I am able to reproduce this on Ubuntu 16.04 with node 6.12 and ShellJS 0.7.8.

Could you point out where you think the fix is?

@jaawerth
Copy link
Author

@jaawerth jaawerth commented Dec 4, 2017

@freitagbr - I took another look, and I don't think the fix is where I thought it was - I thought the recursive code was failing to account for "update" but it's just wrapping the function that does so. It seems like some of the code here could be simplified some by breaking things down into small functions, but that's admittedly beyond the scope of fixing this bug..

Now it's gonna but me, so I'll tinker with it some more this week, time permitting, and see what I can find!

@nfischer
Copy link
Member

@nfischer nfischer commented Jan 11, 2018

@freitagbr can you take a look at this later? Not a big priority, but it'd be good to test and fix.

@freitagbr
Copy link
Contributor

@freitagbr freitagbr commented Jan 11, 2018

Yeah, I will take a look when I get the chance.

@joshi-sh
Copy link
Contributor

@joshi-sh joshi-sh commented Sep 17, 2018

https://github.com/shelljs/shelljs/blob/master/src/cp.js#L263

options.update is not passed to the cpdirSyncRecursive function, and subsequently is ignored

@nfischer
Copy link
Member

@nfischer nfischer commented Sep 17, 2018

@joshi-sh thanks for the pointer. Feel free to send a PR. At first glance, we should just pass options (simpler and future proof), but I'm not sure if there are options which are unsafe to pass recursively.


Related: we should rewrite cmdOptions:

cmdOptions: {
    'f': 'force', // <-- change double-negative ("not no force") to positive
    'n': '!force',
    // ...
    'L': 'followSymlinks', // <-- camel case
    'P': 'preserveSymlinks', // <-- it'd be nice to rename to !followSymlink, but I don't think this is technically feasible.
  },

@joshi-sh
Copy link
Contributor

@joshi-sh joshi-sh commented Sep 17, 2018

--copy-contents is the only option that explicitly changes in recursive mode (from man cp: copy contents of special files when recursive), every other option doesn't seem to conflict with -R

@nfischer
Copy link
Member

@nfischer nfischer commented Sep 18, 2018

I think -P and -L also change. I'm not sure if we explicitly test this.

nfischer added a commit that referenced this issue Nov 13, 2018
Fixed `cp -Ru` ignoring `-u`, and added a test.

Fixes #808
@lorenzleutgeb
Copy link

@lorenzleutgeb lorenzleutgeb commented Apr 1, 2019

@nfischer I am affected by this bug. Could you please release shelljs (it looks like version 0.8.4 is due) such that I can get a version with a fix via the NPM registry?

@nfischer
Copy link
Member

@nfischer nfischer commented Jun 4, 2019

We've landed breaking changes on master branch, but v0.9.0 isn't ready.

Cutting v0.8.4 would be nontrivial, because I would need to create a release branch and cherry-pick changes. I'm not opposed to doing this, I just haven't had the time.

@nfischer nfischer added this to the v0.8.x milestone Jun 4, 2019
@nfischer nfischer removed this from the v0.8.x milestone Jun 4, 2019
@nfischer nfischer added this to the v0.9.0 milestone Jun 4, 2019
@nfischer nfischer assigned nfischer and unassigned freitagbr Jun 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants