Skip to content

Commit 3dac360

Browse files
author
Maxime Bargiel
authored
Update dest atime after copy/copy-sync (#633)
1 parent a6b1a44 commit 3dac360

File tree

4 files changed

+113
-56
lines changed

4 files changed

+113
-56
lines changed

‎lib/copy-sync/__tests__/copy-sync-preserve-timestamp.test.js

Lines changed: 32 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,8 @@
33
const fs = require('../../')
44
const os = require('os')
55
const path = require('path')
6-
const utimes = require('../../util/utimes')
6+
const copySync = require('../copy-sync')
7+
const utimesSync = require('../../util/utimes').utimesMillisSync
78
const assert = require('assert')
89
const semver = require('semver')
910
const nodeVersion = process.version
@@ -19,21 +20,39 @@ const describeIfPractical = (process.arch === 'ia32' || nodeVersionMajor < 8) ?
1920
describeIfPractical('copySync() - preserveTimestamps option', () => {
2021
let TEST_DIR, SRC, DEST, FILES
2122

22-
beforeEach(done => {
23-
TEST_DIR = path.join(os.tmpdir(), 'fs-extra', 'copy-sync-preserve-time')
23+
function setupFixture (readonly) {
24+
TEST_DIR = path.join(os.tmpdir(), 'fs-extra', 'copy-sync-preserve-timestamp')
2425
SRC = path.join(TEST_DIR, 'src')
2526
DEST = path.join(TEST_DIR, 'dest')
2627
FILES = ['a-file', path.join('a-folder', 'another-file'), path.join('a-folder', 'another-folder', 'file3')]
27-
FILES.forEach(f => fs.ensureFileSync(path.join(SRC, f)))
28-
done()
29-
})
28+
const timestamp = Date.now() / 1000 - 5
29+
FILES.forEach(f => {
30+
const filePath = path.join(SRC, f)
31+
fs.ensureFileSync(filePath)
32+
// rewind timestamps to make sure that coarser OS timestamp resolution
33+
// does not alter results
34+
utimesSync(filePath, timestamp, timestamp)
35+
if (readonly) {
36+
fs.chmodSync(filePath, 0o444)
37+
}
38+
})
39+
}
3040

3141
afterEach(done => fs.remove(TEST_DIR, done))
3242

3343
describe('> when preserveTimestamps option is true', () => {
34-
it('should have the same timestamps on copy', () => {
35-
fs.copySync(SRC, DEST, { preserveTimestamps: true })
36-
FILES.forEach(testFile({ preserveTimestamps: true }))
44+
;[
45+
{ subcase: 'writable', readonly: false },
46+
{ subcase: 'readonly', readonly: true }
47+
].forEach(params => {
48+
describe(`>> with ${params.subcase} source files`, () => {
49+
beforeEach(() => setupFixture(params.readonly))
50+
51+
it('should have the same timestamps on copy', () => {
52+
copySync(SRC, DEST, { preserveTimestamps: true })
53+
FILES.forEach(testFile({ preserveTimestamps: true }))
54+
})
55+
})
3756
})
3857
})
3958

@@ -44,16 +63,11 @@ describeIfPractical('copySync() - preserveTimestamps option', () => {
4463
const fromStat = fs.statSync(a)
4564
const toStat = fs.statSync(b)
4665
if (options.preserveTimestamps) {
47-
// https://github.com/nodejs/io.js/issues/2069
48-
if (process.platform !== 'win32') {
49-
assert.strictEqual(toStat.mtime.getTime(), fromStat.mtime.getTime())
50-
assert.strictEqual(toStat.atime.getTime(), fromStat.atime.getTime())
51-
} else {
52-
assert.strictEqual(toStat.mtime.getTime(), utimes.timeRemoveMillis(fromStat.mtime.getTime()))
53-
assert.strictEqual(toStat.atime.getTime(), utimes.timeRemoveMillis(fromStat.atime.getTime()))
54-
}
66+
// Windows sub-second precision fixed: https://github.com/nodejs/io.js/issues/2069
67+
assert.strictEqual(toStat.mtime.getTime(), fromStat.mtime.getTime(), 'different mtime values')
68+
assert.strictEqual(toStat.atime.getTime(), fromStat.atime.getTime(), 'different atime values')
5569
} else {
56-
assert.notStrictEqual(toStat.mtime.getTime(), fromStat.mtime.getTime())
70+
assert.notStrictEqual(toStat.mtime.getTime(), fromStat.mtime.getTime(), 'same mtime values')
5771
// the access time might actually be the same, so check only modification time
5872
}
5973
}

‎lib/copy-sync/copy-sync.js

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
const fs = require('graceful-fs')
44
const path = require('path')
55
const mkdirpSync = require('../mkdirs').mkdirsSync
6-
const utimesSync = require('../util/utimes.js').utimesMillisSync
6+
const utimesMillisSync = require('../util/utimes').utimesMillisSync
77
const stat = require('../util/stat')
88

99
function copySync (src, dest, opts) {
@@ -66,11 +66,13 @@ function mayCopyFile (srcStat, src, dest, opts) {
6666
function copyFile (srcStat, src, dest, opts) {
6767
if (typeof fs.copyFileSync === 'function') {
6868
fs.copyFileSync(src, dest)
69-
fs.chmodSync(dest, srcStat.mode)
70-
if (opts.preserveTimestamps) {
71-
return utimesSync(dest, srcStat.atime, srcStat.mtime)
69+
if (opts.preserveTimestamps && (srcStat.mode & 0o200) === 0) {
70+
// Make sure the file is writable before setting the timestamp
71+
// otherwise openSync fails with EPERM when invoked with 'r+'
72+
// (through utimes call)
73+
fs.chmodSync(dest, srcStat.mode | 0o200)
7274
}
73-
return
75+
return setDestTimestampsAndMode(srcStat, src, dest, opts)
7476
}
7577
return copyFileFallback(srcStat, src, dest, opts)
7678
}
@@ -80,7 +82,7 @@ function copyFileFallback (srcStat, src, dest, opts) {
8082
const _buff = require('../util/buffer')(BUF_LENGTH)
8183

8284
const fdr = fs.openSync(src, 'r')
83-
const fdw = fs.openSync(dest, 'w', srcStat.mode)
85+
const fdw = fs.openSync(dest, 'w')
8486
let pos = 0
8587

8688
while (pos < srcStat.size) {
@@ -89,12 +91,24 @@ function copyFileFallback (srcStat, src, dest, opts) {
8991
pos += bytesRead
9092
}
9193

92-
if (opts.preserveTimestamps) fs.futimesSync(fdw, srcStat.atime, srcStat.mtime)
94+
setDestTimestampsAndMode(srcStat, src, fdw, opts)
9395

9496
fs.closeSync(fdr)
9597
fs.closeSync(fdw)
9698
}
9799

100+
function setDestTimestampsAndMode (srcStat, src, dest, opts) {
101+
const utimesSync = typeof dest === 'string' ? utimesMillisSync : fs.futimesSync
102+
const chmodSync = typeof dest === 'string' ? fs.chmodSync : fs.fchmodSync
103+
if (opts.preserveTimestamps) {
104+
// The initial srcStat.atime cannot be trusted because it is modified by the read(2) system call
105+
// (See https://nodejs.org/api/fs.html#fs_stat_time_values)
106+
const updatedSrcStat = fs.statSync(src)
107+
utimesSync(dest, updatedSrcStat.atime, updatedSrcStat.mtime)
108+
}
109+
chmodSync(dest, srcStat.mode)
110+
}
111+
98112
function onDir (srcStat, destStat, src, dest, opts) {
99113
if (!destStat) return mkDirAndCopy(srcStat, src, dest, opts)
100114
if (destStat && !destStat.isDirectory()) {

‎lib/copy/__tests__/copy-preserve-timestamp.test.js

Lines changed: 33 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ const fs = require('../../')
44
const os = require('os')
55
const path = require('path')
66
const copy = require('../copy')
7-
const utimes = require('../../util/utimes')
7+
const utimesSync = require('../../util/utimes').utimesMillisSync
88
const assert = require('assert')
99
const semver = require('semver')
1010
const nodeVersion = process.version
@@ -20,22 +20,41 @@ const describeIfPractical = (process.arch === 'ia32' || nodeVersionMajor < 8) ?
2020
describeIfPractical('copy() - preserve timestamp', () => {
2121
let TEST_DIR, SRC, DEST, FILES
2222

23-
beforeEach(done => {
23+
function setupFixture (readonly) {
2424
TEST_DIR = path.join(os.tmpdir(), 'fs-extra', 'copy-preserve-timestamp')
2525
SRC = path.join(TEST_DIR, 'src')
2626
DEST = path.join(TEST_DIR, 'dest')
2727
FILES = ['a-file', path.join('a-folder', 'another-file'), path.join('a-folder', 'another-folder', 'file3')]
28-
FILES.forEach(f => fs.ensureFileSync(path.join(SRC, f)))
29-
done()
30-
})
28+
const timestamp = Date.now() / 1000 - 5
29+
FILES.forEach(f => {
30+
const filePath = path.join(SRC, f)
31+
fs.ensureFileSync(filePath)
32+
// rewind timestamps to make sure that coarser OS timestamp resolution
33+
// does not alter results
34+
utimesSync(filePath, timestamp, timestamp)
35+
if (readonly) {
36+
fs.chmodSync(filePath, 0o444)
37+
}
38+
})
39+
}
3140

3241
afterEach(done => fs.remove(TEST_DIR, done))
3342

34-
describe('> when timestamp option is true', () => {
35-
it('should have the same timestamps on copy', done => {
36-
copy(SRC, DEST, { preserveTimestamps: true }, () => {
37-
FILES.forEach(testFile({ preserveTimestamps: true }))
38-
done()
43+
describe('> when preserveTimestamps option is true', () => {
44+
;[
45+
{ subcase: 'writable', readonly: false },
46+
{ subcase: 'readonly', readonly: true }
47+
].forEach(params => {
48+
describe(`>> with ${params.subcase} source files`, () => {
49+
beforeEach(() => setupFixture(params.readonly))
50+
51+
it('should have the same timestamps on copy', done => {
52+
copy(SRC, DEST, { preserveTimestamps: true }, (err) => {
53+
if (err) return done(err)
54+
FILES.forEach(testFile({ preserveTimestamps: true }))
55+
done()
56+
})
57+
})
3958
})
4059
})
4160
})
@@ -47,16 +66,11 @@ describeIfPractical('copy() - preserve timestamp', () => {
4766
const fromStat = fs.statSync(a)
4867
const toStat = fs.statSync(b)
4968
if (options.preserveTimestamps) {
50-
// https://github.com/nodejs/io.js/issues/2069
51-
if (process.platform !== 'win32') {
52-
assert.strictEqual(toStat.mtime.getTime(), fromStat.mtime.getTime())
53-
assert.strictEqual(toStat.atime.getTime(), fromStat.atime.getTime())
54-
} else {
55-
assert.strictEqual(toStat.mtime.getTime(), utimes.timeRemoveMillis(fromStat.mtime.getTime()))
56-
assert.strictEqual(toStat.atime.getTime(), utimes.timeRemoveMillis(fromStat.atime.getTime()))
57-
}
69+
// Windows sub-second precision fixed: https://github.com/nodejs/io.js/issues/2069
70+
assert.strictEqual(toStat.mtime.getTime(), fromStat.mtime.getTime(), 'different mtime values')
71+
assert.strictEqual(toStat.atime.getTime(), fromStat.atime.getTime(), 'different atime values')
5872
} else {
59-
assert.notStrictEqual(toStat.mtime.getTime(), fromStat.mtime.getTime())
73+
assert.notStrictEqual(toStat.mtime.getTime(), fromStat.mtime.getTime(), 'same mtime values')
6074
// the access time might actually be the same, so check only modification time
6175
}
6276
}

‎lib/copy/copy.js

Lines changed: 27 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ const fs = require('graceful-fs')
44
const path = require('path')
55
const mkdirp = require('../mkdirs').mkdirs
66
const pathExists = require('../path-exists').pathExists
7-
const utimes = require('../util/utimes').utimesMillis
7+
const utimesMillis = require('../util/utimes').utimesMillis
88
const stat = require('../util/stat')
99

1010
function copy (src, dest, opts, cb) {
@@ -95,7 +95,16 @@ function copyFile (srcStat, src, dest, opts, cb) {
9595
if (typeof fs.copyFile === 'function') {
9696
return fs.copyFile(src, dest, err => {
9797
if (err) return cb(err)
98-
return setDestModeAndTimestamps(srcStat, dest, opts, cb)
98+
if (opts.preserveTimestamps && (srcStat.mode & 0o200) === 0) {
99+
// Make sure the file is writable before setting the timestamp
100+
// otherwise openSync fails with EPERM when invoked with 'r+'
101+
// (through utimes call)
102+
return fs.chmod(dest, srcStat.mode | 0o200, (err) => {
103+
if (err) return cb(err)
104+
return setDestTimestampsAndMode(srcStat, src, dest, opts, cb)
105+
})
106+
}
107+
return setDestTimestampsAndMode(srcStat, src, dest, opts, cb)
99108
})
100109
}
101110
return copyFileFallback(srcStat, src, dest, opts, cb)
@@ -104,21 +113,27 @@ function copyFile (srcStat, src, dest, opts, cb) {
104113
function copyFileFallback (srcStat, src, dest, opts, cb) {
105114
const rs = fs.createReadStream(src)
106115
rs.on('error', err => cb(err)).once('open', () => {
107-
const ws = fs.createWriteStream(dest, { mode: srcStat.mode })
116+
// explicitly not setting srcStat mode - will do that last
117+
const ws = fs.createWriteStream(dest)
108118
ws.on('error', err => cb(err))
109119
.on('open', () => rs.pipe(ws))
110-
.once('close', () => setDestModeAndTimestamps(srcStat, dest, opts, cb))
120+
.once('close', () => setDestTimestampsAndMode(srcStat, src, dest, opts, cb))
111121
})
112122
}
113123

114-
function setDestModeAndTimestamps (srcStat, dest, opts, cb) {
115-
fs.chmod(dest, srcStat.mode, err => {
116-
if (err) return cb(err)
117-
if (opts.preserveTimestamps) {
118-
return utimes(dest, srcStat.atime, srcStat.mtime, cb)
119-
}
120-
return cb()
121-
})
124+
function setDestTimestampsAndMode (srcStat, src, dest, opts, cb) {
125+
if (opts.preserveTimestamps) {
126+
// The initial srcStat.atime cannot be trusted because it is modified by the read(2) system call
127+
// (See https://nodejs.org/api/fs.html#fs_stat_time_values)
128+
return fs.stat(src, (err, updatedSrcStat) => {
129+
if (err) return cb(err)
130+
return utimesMillis(dest, updatedSrcStat.atime, updatedSrcStat.mtime, (err2) => {
131+
if (err2) return cb(err2)
132+
return fs.chmod(dest, srcStat.mode, cb)
133+
})
134+
})
135+
}
136+
return fs.chmod(dest, srcStat.mode, cb)
122137
}
123138

124139
function onDir (srcStat, destStat, src, dest, opts, cb) {

0 commit comments

Comments
 (0)