diff --git a/lib/copy-sync/__tests__/copy-sync-preserve-timestamp.test.js b/lib/copy-sync/__tests__/copy-sync-preserve-timestamp.test.js index 39cda0cb..da3aacac 100644 --- a/lib/copy-sync/__tests__/copy-sync-preserve-timestamp.test.js +++ b/lib/copy-sync/__tests__/copy-sync-preserve-timestamp.test.js @@ -3,7 +3,7 @@ const fs = require('../../') const os = require('os') const path = require('path') -const utimes = require('../../util/utimes') +const utimesSync = require('../../util/utimes').utimesMillisSync const assert = require('assert') const semver = require('semver') const nodeVersion = process.version @@ -19,21 +19,39 @@ const describeIfPractical = (process.arch === 'ia32' || nodeVersionMajor < 8) ? describeIfPractical('copySync() - preserveTimestamps option', () => { let TEST_DIR, SRC, DEST, FILES - beforeEach(done => { - TEST_DIR = path.join(os.tmpdir(), 'fs-extra', 'copy-sync-preserve-time') + function setupFixture (readonly) { + TEST_DIR = path.join(os.tmpdir(), 'fs-extra', 'copy-sync-preserve-timestamp') SRC = path.join(TEST_DIR, 'src') DEST = path.join(TEST_DIR, 'dest') FILES = ['a-file', path.join('a-folder', 'another-file'), path.join('a-folder', 'another-folder', 'file3')] - FILES.forEach(f => fs.ensureFileSync(path.join(SRC, f))) - done() - }) + const timestamp = Date.now() / 1000 - 5 + FILES.forEach(f => { + const filePath = path.join(SRC, f) + fs.ensureFileSync(filePath) + // rewind timestamps to make sure that coarser OS timestamp resolution + // does not alter results + utimesSync(filePath, timestamp, timestamp) + if (readonly) { + fs.chmodSync(filePath, 0o444) + } + }) + } afterEach(done => fs.remove(TEST_DIR, done)) describe('> when preserveTimestamps option is true', () => { - it('should have the same timestamps on copy', () => { - fs.copySync(SRC, DEST, { preserveTimestamps: true }) - FILES.forEach(testFile({ preserveTimestamps: true })) + ;[ + { subcase: 'writable', readonly: false }, + { subcase: 'readonly', readonly: true } + ].forEach(params => { + describe(`>> with ${params.subcase} source files`, () => { + beforeEach(() => setupFixture(params.readonly)) + + it('should have the same timestamps on copy', () => { + fs.copySync(SRC, DEST, { preserveTimestamps: true }) + FILES.forEach(testFile({ preserveTimestamps: true })) + }) + }) }) }) @@ -44,14 +62,9 @@ describeIfPractical('copySync() - preserveTimestamps option', () => { const fromStat = fs.statSync(a) const toStat = fs.statSync(b) if (options.preserveTimestamps) { - // https://github.com/nodejs/io.js/issues/2069 - if (process.platform !== 'win32') { - assert.strictEqual(toStat.mtime.getTime(), fromStat.mtime.getTime()) - assert.strictEqual(toStat.atime.getTime(), fromStat.atime.getTime()) - } else { - assert.strictEqual(toStat.mtime.getTime(), utimes.timeRemoveMillis(fromStat.mtime.getTime())) - assert.strictEqual(toStat.atime.getTime(), utimes.timeRemoveMillis(fromStat.atime.getTime())) - } + // Windows sub-second precision fixed: https://github.com/nodejs/io.js/issues/2069 + assert.strictEqual(toStat.mtime.getTime(), fromStat.mtime.getTime()) + assert.strictEqual(toStat.atime.getTime(), fromStat.atime.getTime()) } else { assert.notStrictEqual(toStat.mtime.getTime(), fromStat.mtime.getTime()) // the access time might actually be the same, so check only modification time diff --git a/lib/copy-sync/copy-sync.js b/lib/copy-sync/copy-sync.js index bb70e819..5460444c 100644 --- a/lib/copy-sync/copy-sync.js +++ b/lib/copy-sync/copy-sync.js @@ -68,10 +68,17 @@ function mayCopyFile (srcStat, src, dest, opts) { function copyFile (srcStat, src, dest, opts) { if (typeof fs.copyFileSync === 'function') { fs.copyFileSync(src, dest) - fs.chmodSync(dest, srcStat.mode) if (opts.preserveTimestamps) { - return utimesSync(dest, srcStat.atime, srcStat.mtime) + // Make sure the file is writable first + if ((srcStat.mode & 0o200) === 0) { + fs.chmodSync(dest, srcStat.mode | 0o200) + } + // Cannot rely on previously fetched srcStat because atime is updated by + // the read operation: https://nodejs.org/docs/latest-v8.x/api/fs.html#fs_stat_time_values + const updatedSrcStat = fs.statSync(src) + utimesSync(dest, updatedSrcStat.atime, updatedSrcStat.mtime) } + fs.chmodSync(dest, srcStat.mode) return } return copyFileFallback(srcStat, src, dest, opts) @@ -91,7 +98,10 @@ function copyFileFallback (srcStat, src, dest, opts) { pos += bytesRead } - if (opts.preserveTimestamps) fs.futimesSync(fdw, srcStat.atime, srcStat.mtime) + if (opts.preserveTimestamps) { + const updatedSrcStat = fs.statSync(src) + fs.futimesSync(dest, updatedSrcStat.atime, updatedSrcStat.mtime) + } fs.closeSync(fdr) fs.closeSync(fdw) diff --git a/lib/copy/__tests__/copy-preserve-timestamp.test.js b/lib/copy/__tests__/copy-preserve-timestamp.test.js index ba52cb43..6fd0b173 100644 --- a/lib/copy/__tests__/copy-preserve-timestamp.test.js +++ b/lib/copy/__tests__/copy-preserve-timestamp.test.js @@ -4,7 +4,7 @@ const fs = require('../../') const os = require('os') const path = require('path') const copy = require('../copy') -const utimes = require('../../util/utimes') +const utimesSync = require('../../util/utimes').utimesMillisSync const assert = require('assert') const semver = require('semver') const nodeVersion = process.version @@ -20,22 +20,41 @@ const describeIfPractical = (process.arch === 'ia32' || nodeVersionMajor < 8) ? describeIfPractical('copy() - preserve timestamp', () => { let TEST_DIR, SRC, DEST, FILES - beforeEach(done => { + function setupFixture (readonly) { TEST_DIR = path.join(os.tmpdir(), 'fs-extra', 'copy-preserve-timestamp') SRC = path.join(TEST_DIR, 'src') DEST = path.join(TEST_DIR, 'dest') FILES = ['a-file', path.join('a-folder', 'another-file'), path.join('a-folder', 'another-folder', 'file3')] - FILES.forEach(f => fs.ensureFileSync(path.join(SRC, f))) - done() - }) + const timestamp = Date.now() / 1000 - 5 + FILES.forEach(f => { + const filePath = path.join(SRC, f) + fs.ensureFileSync(filePath) + // rewind timestamps to make sure that coarser OS timestamp resolution + // does not alter results + utimesSync(filePath, timestamp, timestamp) + if (readonly) { + fs.chmodSync(filePath, 0o444) + } + }) + } afterEach(done => fs.remove(TEST_DIR, done)) - describe('> when timestamp option is true', () => { - it('should have the same timestamps on copy', done => { - copy(SRC, DEST, { preserveTimestamps: true }, () => { - FILES.forEach(testFile({ preserveTimestamps: true })) - done() + describe('> when preserveTimestamps option is true', () => { + ;[ + { subcase: 'writable', readonly: false }, + { subcase: 'readonly', readonly: true } + ].forEach(params => { + describe(`>> with ${params.subcase} source files`, () => { + beforeEach(() => setupFixture(params.readonly)) + + it('should have the same timestamps on copy', done => { + copy(SRC, DEST, { preserveTimestamps: true }, (err) => { + if (err) return done(err) + FILES.forEach(testFile({ preserveTimestamps: true })) + done() + }) + }) }) }) }) @@ -47,14 +66,9 @@ describeIfPractical('copy() - preserve timestamp', () => { const fromStat = fs.statSync(a) const toStat = fs.statSync(b) if (options.preserveTimestamps) { - // https://github.com/nodejs/io.js/issues/2069 - if (process.platform !== 'win32') { - assert.strictEqual(toStat.mtime.getTime(), fromStat.mtime.getTime()) - assert.strictEqual(toStat.atime.getTime(), fromStat.atime.getTime()) - } else { - assert.strictEqual(toStat.mtime.getTime(), utimes.timeRemoveMillis(fromStat.mtime.getTime())) - assert.strictEqual(toStat.atime.getTime(), utimes.timeRemoveMillis(fromStat.atime.getTime())) - } + // Windows sub-second precision fixed: https://github.com/nodejs/io.js/issues/2069 + assert.strictEqual(toStat.mtime.getTime(), fromStat.mtime.getTime()) + assert.strictEqual(toStat.atime.getTime(), fromStat.atime.getTime()) } else { assert.notStrictEqual(toStat.mtime.getTime(), fromStat.mtime.getTime()) // the access time might actually be the same, so check only modification time diff --git a/lib/copy/copy.js b/lib/copy/copy.js index fdab802c..5fbdb279 100644 --- a/lib/copy/copy.js +++ b/lib/copy/copy.js @@ -97,7 +97,7 @@ function copyFile (srcStat, src, dest, opts, cb) { if (typeof fs.copyFile === 'function') { return fs.copyFile(src, dest, err => { if (err) return cb(err) - return setDestModeAndTimestamps(srcStat, dest, opts, cb) + return setDestModeAndTimestamps(srcStat, src, dest, opts, cb) }) } return copyFileFallback(srcStat, src, dest, opts, cb) @@ -109,18 +109,33 @@ function copyFileFallback (srcStat, src, dest, opts, cb) { const ws = fs.createWriteStream(dest, { mode: srcStat.mode }) ws.on('error', err => cb(err)) .on('open', () => rs.pipe(ws)) - .once('close', () => setDestModeAndTimestamps(srcStat, dest, opts, cb)) + .once('close', () => setDestModeAndTimestamps(srcStat, src, dest, opts, cb)) }) } -function setDestModeAndTimestamps (srcStat, dest, opts, cb) { - fs.chmod(dest, srcStat.mode, err => { +function setDestModeAndTimestamps (srcStat, src, dest, opts, cb) { + if (!opts.preserveTimestamps) { + return fs.chmod(dest, srcStat.mode, cb) + } + + const next = (err) => { if (err) return cb(err) - if (opts.preserveTimestamps) { - return utimes(dest, srcStat.atime, srcStat.mtime, cb) - } - return cb() - }) + // Cannot rely on previously fetched srcStat because atime is updated by + // the read operation: https://nodejs.org/docs/latest-v8.x/api/fs.html#fs_stat_time_values + return fs.stat(src, (err, srcStat) => { + if (err) return cb(err) + return utimes(dest, srcStat.atime, srcStat.mtime, (err) => { + if (err) return cb(err) + return fs.chmod(dest, srcStat.mode, cb) + }) + }) + } + + if ((srcStat.mode & 0o200) === 0) { + // Make sure the file is writable first + return fs.chmod(dest, srcStat.mode | 0o200, next) + } + return next() } function onDir (srcStat, destStat, src, dest, opts, cb) {