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

Add '-p' (preserve) option to 'cp', fixes #771 #841

Closed
wants to merge 4 commits into from
Closed

Add '-p' (preserve) option to 'cp', fixes #771 #841

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bafc9a9
Add '-p' (preserve) option to 'cp'
dwi2 May 2, 2018
d84236d
Address review comment and add sleep in test to workaround possible r…
dwi2 May 6, 2018
3ede418
Update time before fd close
dwi2 May 6, 2018
4760a76
Use resources/cp/file1 and add sleep to workaround possible racing
dwi2 May 17, 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
11 changes: 8 additions & 3 deletions src/cp.js
View file
Open in desktop
Expand Up @@ -11,6 +11,7 @@ common.register('cp', _cp, {
'r': 'recursive',
'L': 'followsymlink',
'P': 'noFollowsymlink',
'p': 'preserve',
},
wrapOutput: false,
});
Expand Down Expand Up @@ -51,6 +52,7 @@ function copyFileSync(srcFile, destFile, options) {
var pos = 0;
var fdr = null;
var fdw = null;
var srcStat = common.statFollowLinks(srcFile);

try {
fdr = fs.openSync(srcFile, 'r');
Expand All @@ -60,7 +62,7 @@ function copyFileSync(srcFile, destFile, options) {
}

try {
fdw = fs.openSync(destFile, 'w');
fdw = fs.openSync(destFile, 'w', srcStat.mode);
} catch (e) {
/* istanbul ignore next */
common.error('copyFileSync: could not write to dest file (code=' + e.code + '):' + destFile);
Expand All @@ -72,10 +74,13 @@ function copyFileSync(srcFile, destFile, options) {
pos += bytesRead;
}

if (options.preserve) {
fs.fchownSync(fdw, srcStat.uid, srcStat.gid);
fs.futimesSync(fdw, srcStat.atime, srcStat.mtime);
Copy link
Contributor Author

@dwi2 dwi2 May 6, 2018 •
edited

Choose a reason for hiding this comment

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

On linux, fs.utimesSync() always produces timestamps rounded off to seconds but original files could have timestamps of millisecond level. But I still don't why is that so I probably need more time to figure it out.
However fs.futimesSync() could produce timestamps of millisecond level. So I put it this way for now.

Also I tried to add sleep in test but it didn't work.

Copy link
Member

Choose a reason for hiding this comment

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

Huh, ok. This implementation is fine. I will add a comment explaining that utimesSync doesn't work but futimesSync does.

}

fs.closeSync(fdr);
fs.closeSync(fdw);

fs.chmodSync(destFile, common.statFollowLinks(srcFile).mode);
}
}

Expand Down
26 changes: 26 additions & 0 deletions test/cp.js
View file
Open in desktop
Expand Up @@ -756,3 +756,29 @@ test('should not overwrite recently created files (not give error no-force mode)
// Ensure First file is copied
t.is(shell.cat(`${t.context.tmp}/file1`).toString(), 'test1');
});

test('cp -p should preserve mode, ownership, and timestamp', t => {
const result = shell.cp('-p', 'test/resources/file1', `${t.context.tmp}/preservedFile1`);
Copy link
Member

Choose a reason for hiding this comment

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

I'll follow-up to this to touch() the original file. Otherwise, we might have a race where the test setup creates file1 and the test nearly-immediately copies it--depending on the timing/precision, a regular cp() call might even pass this test.

We should probably also chmod the file too, to double-check the mode gets copied correctly (and it isn't the default mode).

const stat = common.statFollowLinks('test/resources/file1');
const statOfResult = common.statFollowLinks(`${t.context.tmp}/preservedFile1`);

t.is(result.code, 0);
t.is(stat.mtime.getTime(), statOfResult.mtime.getTime());
t.is(stat.atime.getTime(), statOfResult.atime.getTime());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://travis-ci.org/shelljs/shelljs/builds/374922671?utm_source=github_status&utm_medium=notification
Seems like there are some differences on atime and mtime between original file and its copy on Linux platform.
Mac OSX seems fine.
I am still figuring out why.

Copy link
Member

Choose a reason for hiding this comment

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

Probably a race. Try adding a sleep to the test (see touch tests for example), which will hopefully provide a reliable repro.

t.is(stat.mode, statOfResult.mode);
t.is(stat.uid, statOfResult.uid);
t.is(stat.gid, statOfResult.gid);
});

test('cp -p should preserve mode, ownership, and timestamp of symlink', t => {
const result = shell.cp('-p', 'test/resources/link', `${t.context.tmp}/copiedLink`);
const stat = common.statFollowLinks('test/resources/link');
const statOfResult = common.statFollowLinks(`${t.context.tmp}/copiedLink`);

t.is(result.code, 0);
t.is(stat.mtime.getTime(), statOfResult.mtime.getTime());
t.is(stat.atime.getTime(), statOfResult.atime.getTime());
t.is(stat.mode, statOfResult.mode);
t.is(stat.uid, statOfResult.uid);
t.is(stat.rid, statOfResult.rid);
});