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. |
| 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.
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.
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.
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);
|
|
||
| if (options.preserve) { | ||
| fs.fchownSync(fdw, srcStat.uid, srcStat.gid); | ||
| fs.futimesSync(fdw, srcStat.atime, srcStat.mtime); |
There was a problem hiding this comment.
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.
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. |
nfischer
left a comment
nfischer
left a comment
There was a problem hiding this comment.
Thanks for your help! I'll land this and upload a follow-up PR to address my remaining comments.
|
|
||
| if (options.preserve) { | ||
| fs.fchownSync(fdw, srcStat.uid, srcStat.gid); | ||
| fs.futimesSync(fdw, srcStat.atime, srcStat.mtime); |
There was a problem hiding this comment.
Huh, ok. This implementation is fine. I will add a comment explaining that utimesSync doesn't work but futimesSync does.
test/cp.js
Outdated
| }); | ||
|
|
||
| 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.
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. |
3 participants
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_LISTfeature in this PR. I think that could be a follow-up work.TODO
npm run gendocscp -pon directory