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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 11 commits into from Nov 13, 2018
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c40496a
Propogate the -u option when copying recursively
joshi-sh Sep 18, 2018
4ca5694
Added test for cp -Ru
joshi-sh Sep 18, 2018
dface6c
Fixed lint errors
joshi-sh Sep 18, 2018
8d5811d
Better description of test case
joshi-sh Sep 19, 2018
3f2d0fd
Fixed test to check contents. Added test for succesful updates
joshi-sh Sep 20, 2018
3283d5e
Fixed undefined reference error
joshi-sh Sep 20, 2018
ee59552
Fixed lint errors
joshi-sh Sep 20, 2018
cdb269f
Cleaned up comments and strings to make them more descriptive
joshi-sh Oct 28, 2018
d4c27fc
Created constant for magic number (two days in ms)
joshi-sh Oct 28, 2018
0b1579b
Merge branch 'master' into issue-808
joshi-sh Oct 28, 2018
f590747
Removed trailing spaces
joshi-sh Oct 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/cp.js
View file
Open in desktop
Expand Up @@ -260,7 +260,7 @@ function _cp(options, sources, dest) {

try {
common.statFollowLinks(path.dirname(dest));
cpdirSyncRecursive(src, newDest, 0, { no_force: options.no_force, followsymlink: options.followsymlink });
cpdirSyncRecursive(src, newDest, 0, { no_force: options.no_force, followsymlink: options.followsymlink, update: options.update });
} catch (e) {
/* istanbul ignore next */
common.error("cannot create directory '" + dest + "': No such file or directory");
Expand Down
39 changes: 39 additions & 0 deletions test/cp.js
View file
Open in desktop
Expand Up @@ -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.

const dir = `${t.context.tmp}/cp-Ru`;
const sourceDir = `${dir}/old`;
const sourceFile = `${sourceDir}/file`;
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?

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.

// 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.

shell.touch('-m', oldTime - (2 * 24 * 60 * 60 * 1000), sourceFile);
// Now, copy the old dir to the new one
shell.cp('-Ru', sourceDir, destDir);
// Check that dest has not been updated
t.is(shell.cat(destFile).stdout, 'bar\n');
});

test('-Ru respects the -u flag recursively (update older file)', t => {
const dir = `${t.context.tmp}/cp-Ru`;
const sourceDir = `${dir}/old`;
const sourceFile = `${sourceDir}/file`;
const destDir = `${dir}/new`;
const destFile = `${destDir}/file`;
[sourceDir, destDir].forEach(d => shell.mkdir('-p', d));
shell.ShellString('foo\n').to(sourceFile);
shell.ShellString('bar\n').to(destFile);
// Get the old mtime for dest
const oldTime = fs.statSync(destFile).mtimeMs;
// Send the source file to two days ahead
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)

// Check that dest has been updated
t.is(shell.cat(sourceFile).stdout, 'foo\n');
});

joshi-sh marked this conversation as resolved.
Show resolved Hide resolved
test('using -P explicitly works', t => {
utils.skipOnWin(t, () => {
shell.cp('-P', 'test/resources/cp/links/sym.lnk', t.context.tmp);
Expand Down