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
2 changes: 1 addition & 1 deletion src/cp.js
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
13 changes: 13 additions & 0 deletions test/cp.js
Expand Up @@ -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

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.

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

// Get the old mtime for dest
const oldTime = fs.statSync(dest).mtimeMs;
// Now, copy the old dir to the new one
shell.cp('-Ru', source, dest);
// 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)

const newTime = fs.statSync(dest).mtimeMs;
t.is(oldTime, newTime);
});

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
Empty file.
Empty file.
Empty file.