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

feat(cp): support update flag when recursing #889

Merged
merged 11 commits into from Nov 13, 2018

Conversation

joshi-sh
Copy link
Contributor

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

Fixed cp -Ru ignoring -u, and added a test.

Fixes #808

@nfischer nfischer changed the title Issue 808 feat(cp): support update flag when recursing Sep 18, 2018
test/cp.js Outdated
@@ -451,6 +451,19 @@ test('-R implies -P', t => {
});
});

test('-Ru no longer ignores -u', t => {
Copy link
Member

Choose a reason for hiding this comment

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

nit: -Ru respects the -u flag recursively

test/cp.js Outdated
test('-Ru no longer ignores -u', t => {
const [source, dest] = ['foo', 'bar'].map(s => `test/resources/cp/cp-Ru/${s}/file`);
// First, update the dest file
shell.touch(dest);
Copy link
Member

Choose a reason for hiding this comment

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

You also need to update the source directory to an older time stamp, right? Otherwise, we'll see race conditions in very-fresh checkouts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In a later comment, you've mentioned checking file contents. That would resolve this issue as well.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, file contents is fine to use here as well.

test/cp.js Outdated
@@ -451,6 +451,19 @@ test('-R implies -P', t => {
});
});

test('-Ru no longer ignores -u', t => {
const [source, dest] = ['foo', 'bar'].map(s => `test/resources/cp/cp-Ru/${s}/file`);
Copy link
Member

Choose a reason for hiding this comment

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

You should copy to t.context.tmp. Treat test/resources/ as read-only.

test/cp.js Show resolved Hide resolved
@nfischer
Copy link
Member

@joshi-sh can you check the npm run gendocs warning?

@joshi-sh
Copy link
Contributor Author

joshi-sh commented Sep 19, 2018

Running npm run gendocs on Ubuntu 16.04.3 on top of WSL with node v8.12.0 produces no errors

@nfischer
Copy link
Member

@joshi-sh it might not be a docs issue, but there might be modified or extra files leftover after running tests + lint + generate docs. Seems to be v8.x.y specific.

Copy link
Member

@nfischer nfischer left a comment

Choose a reason for hiding this comment

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

Most comments are optional, but I'd like to see if we can cleanup test setup.

test/cp.js Outdated
const destFile = `${destDir}/file`;
[sourceDir, destDir].forEach(d => shell.mkdir('-p', d));
shell.ShellString('foo\n').to(sourceFile);
shell.ShellString('bar\n').to(destFile);
Copy link
Member

Choose a reason for hiding this comment

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

I would like to separate setup from test logic. Two (alternative) suggestions:

  1. Instead of setting up at runtime, commit this directory structure under test/resources/, or
  2. Add an empty line between setup code and test logic (possibly add a comment above such as // Setup code)

You'll need to continue modifying the time stamps at runtime, as you have here.

test/cp.js Outdated
shell.ShellString('bar\n').to(destFile);
// Get the old mtime for dest
const oldTime = fs.statSync(destFile).mtimeMs;
// Send the old file to two days ago
Copy link
Member

Choose a reason for hiding this comment

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

nit: you can better document this by creating a constant TWO_DAYS_IN_MS.

You could then make this comment more descriptive: // Set the sourceFile to be older than destFile.

test/cp.js Outdated
shell.touch('-m', oldTime + (2 * 24 * 60 * 60 * 1000), sourceFile);
// Now, copy the old dir to the new one
shell.cp('-Ru', sourceDir, destDir);
// Get the new mtime for dest
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think this comment is leftover from a previous commit (you're not checking mtimes anymore)

test/cp.js Outdated
const destDir = `${dir}/new`;
const destFile = `${destDir}/file`;
[sourceDir, destDir].forEach(d => shell.mkdir('-p', d));
shell.ShellString('foo\n').to(sourceFile);
Copy link
Member

Choose a reason for hiding this comment

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

nit: file contents could be more descriptive. How about source file contents and destination file contents?

@@ -451,6 +451,45 @@ test('-R implies -P', t => {
});
});

test('-Ru respects the -u flag recursively (don\'t update newer file)', t => {
Copy link
Member

Choose a reason for hiding this comment

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

You might be able to replace this with a template literal to avoid escaping the apostrophe. I can't remember if our lint currently forbids this (see #788). If lint complains, just leave as-is.

This was referenced Oct 10, 2018
@nfischer
Copy link
Member

Running npm run gendocs on Ubuntu 16.04.3 on top of WSL with node v8.12.0 produces no errors

Fixed the issue in #896. Please rebase.

@codecov-io
Copy link

Codecov Report

Merging #889 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #889   +/-   ##
=======================================
  Coverage   95.81%   95.81%           
=======================================
  Files          34       34           
  Lines        1266     1266           
=======================================
  Hits         1213     1213           
  Misses         53       53
Impacted Files Coverage Δ
src/cp.js 86.4% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b3b781...f590747. Read the comment docs.

@nfischer nfischer merged commit 18d8bbf into shelljs:master Nov 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants