-
Notifications
You must be signed in to change notification settings - Fork 733
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
Add '-p' (preserve) option to 'cp', fixes #771 #841
Conversation
I kicked off a new appveyor build because the failure doesn't look related to your PR. Hopefully that's just a fluke. |
test/cp.js
Outdated
t.is(result.code, 0); | ||
|
||
t.is(stat.mtime.getTime(), statOfResult.mtime.getTime()); | ||
t.is(stat.atime.getTime(), statOfResult.atime.getTime()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/cp.js
Outdated
fs.chownSync(destFile, srcStat.uid, srcStat.gid); | ||
fs.utimesSync(destFile, srcStat.atime, srcStat.mtime); | ||
} | ||
fs.chmodSync(destFile, srcStat.mode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of changing the mode after writing the file, we can set the mode on the initial call to fs.openSync
on line 65:
fdw = fs.openSync(destFile, 'w', srcStat.mode);
@@ -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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Also about the appveyor build failed, I checked the difference between the build result between this PR and master branch. In the [build of this PR]((https://ci.appveyor.com/project/shelljs/shelljs/build/1610/job/dgdr1ws2ote8fu9v), appveyor tried to install npm v6.0 on node JS v5 while it installed npm v5.8 in master build. |
Not your responsibility. I filed #844, I'll handle this ASAP.
Sleeping is supposed to break flaky tests. The suspicion is that there's a race condition, so you should add the sleep for all platforms (especially those which appear to pass). I didn't look at test output though, but examining that should make it obvious if this was because of a race. |
@nfischer Thanks a lot. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your help! I'll land this and upload a follow-up PR to address my remaining comments.
@@ -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); |
There was a problem hiding this comment.
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.
test/cp.js
Outdated
@@ -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`); |
There was a problem hiding this comment.
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).
Close in favor of #869. Thanks for your help! |
Thanks for carrying on. |
This fixes #771.
WIP
This is still work in progress. But I want to make sure I am on the right track so I just create pull request first.
I don't plan to implement the full
--preserve=ATTR_LIST
feature in this PR. I think that could be a follow-up work.TODO
npm run gendocs
cp -p
on directory