From 3121749c051718887fc66052eeb55f19f2eb309a Mon Sep 17 00:00:00 2001 From: Maxime Bargiel Date: Fri, 12 Oct 2018 15:30:13 -0400 Subject: [PATCH] Update dest `atime` after `copy`/`copy-sync` The `copyFileFallback` flow is now also tested on more recent Node, so it is now included in the code coverage metrics. --- .../copy-sync-preserve-timestamp.test.js | 50 +++++++++++------- .../__tests__/copyfilesync-fallback.test.js | 29 +++++++++++ lib/copy-sync/copy-sync.js | 28 +++++++--- .../__tests__/copy-preserve-timestamp.test.js | 52 ++++++++++++------- lib/copy/__tests__/copyfile-fallback.test.js | 29 +++++++++++ lib/copy/copy.js | 39 +++++++++----- 6 files changed, 171 insertions(+), 56 deletions(-) create mode 100644 lib/copy-sync/__tests__/copyfilesync-fallback.test.js create mode 100644 lib/copy/__tests__/copyfile-fallback.test.js 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..6612b3c9 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,8 @@ const fs = require('../../') const os = require('os') const path = require('path') -const utimes = require('../../util/utimes') +const copySync = require('../copy-sync') +const utimesSync = require('../../util/utimes').utimesMillisSync const assert = require('assert') const semver = require('semver') const nodeVersion = process.version @@ -19,21 +20,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', () => { + copySync(SRC, DEST, { preserveTimestamps: true }) + FILES.forEach(testFile({ preserveTimestamps: true })) + }) + }) }) }) @@ -44,16 +63,11 @@ 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(), 'different mtime values') + assert.strictEqual(toStat.atime.getTime(), fromStat.atime.getTime(), 'different atime values') } else { - assert.notStrictEqual(toStat.mtime.getTime(), fromStat.mtime.getTime()) + assert.notStrictEqual(toStat.mtime.getTime(), fromStat.mtime.getTime(), 'same mtime values') // the access time might actually be the same, so check only modification time } } diff --git a/lib/copy-sync/__tests__/copyfilesync-fallback.test.js b/lib/copy-sync/__tests__/copyfilesync-fallback.test.js new file mode 100644 index 00000000..582437d9 --- /dev/null +++ b/lib/copy-sync/__tests__/copyfilesync-fallback.test.js @@ -0,0 +1,29 @@ +const fs = require('graceful-fs') +const path = require('path') + +/* global describe, beforeEach, afterEach */ + +if (typeof fs.copyFileSync === 'function') { + // Also load copy tests without fs.copyFileSync + describe('> when using copyFileFallback', () => { + const originalCopyFileSync = fs.copyFileSync + + beforeEach(() => { + // reset stubs + delete fs.copyFileSync + }) + + afterEach(() => { + fs.copyFileSync = originalCopyFileSync + }) + + const fallbackTest = path.basename(__filename) + fs.readdirSync(__dirname) + .filter(filename => filename.endsWith('.test.js') && filename !== fallbackTest) + .map(filename => { + const testModule = `./${filename}` + delete require.cache[require.resolve(testModule)] + require(testModule) + }) + }) +} diff --git a/lib/copy-sync/copy-sync.js b/lib/copy-sync/copy-sync.js index f2831b32..bb6e743a 100644 --- a/lib/copy-sync/copy-sync.js +++ b/lib/copy-sync/copy-sync.js @@ -3,7 +3,7 @@ const fs = require('graceful-fs') const path = require('path') const mkdirpSync = require('../mkdirs').mkdirsSync -const utimesSync = require('../util/utimes.js').utimesMillisSync +const utimesMillisSync = require('../util/utimes').utimesMillisSync const stat = require('../util/stat') function copySync (src, dest, opts) { @@ -66,11 +66,13 @@ 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) + if (opts.preserveTimestamps && (srcStat.mode & 0o200) === 0) { + // Make sure the file is writable before setting the timestamp + // otherwise openSync fails with EPERM when invoked with 'r+' + // (through utimes call) + fs.chmodSync(dest, srcStat.mode | 0o200) } - return + return setDestTimestampsAndMode(srcStat, src, dest, opts) } return copyFileFallback(srcStat, src, dest, opts) } @@ -80,7 +82,7 @@ function copyFileFallback (srcStat, src, dest, opts) { const _buff = require('../util/buffer')(BUF_LENGTH) const fdr = fs.openSync(src, 'r') - const fdw = fs.openSync(dest, 'w', srcStat.mode) + const fdw = fs.openSync(dest, 'w') let pos = 0 while (pos < srcStat.size) { @@ -89,12 +91,24 @@ function copyFileFallback (srcStat, src, dest, opts) { pos += bytesRead } - if (opts.preserveTimestamps) fs.futimesSync(fdw, srcStat.atime, srcStat.mtime) + setDestTimestampsAndMode(srcStat, src, fdw, opts) fs.closeSync(fdr) fs.closeSync(fdw) } +function setDestTimestampsAndMode (srcStat, src, dest, opts) { + const utimesSync = typeof dest === 'string' ? utimesMillisSync : fs.futimesSync + const chmodSync = typeof dest === 'string' ? fs.chmodSync : fs.fchmodSync + if (opts.preserveTimestamps) { + // The initial srcStat.atime cannot be trusted because it is modified by the read(2) system call + // (See https://nodejs.org/api/fs.html#fs_stat_time_values) + const updatedSrcStat = fs.statSync(src) + utimesSync(dest, updatedSrcStat.atime, updatedSrcStat.mtime) + } + chmodSync(dest, srcStat.mode) +} + function onDir (srcStat, destStat, src, dest, opts) { if (!destStat) return mkDirAndCopy(srcStat, src, dest, opts) if (destStat && !destStat.isDirectory()) { diff --git a/lib/copy/__tests__/copy-preserve-timestamp.test.js b/lib/copy/__tests__/copy-preserve-timestamp.test.js index ba52cb43..5447e706 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,16 +66,11 @@ 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(), 'different mtime values') + assert.strictEqual(toStat.atime.getTime(), fromStat.atime.getTime(), 'different atime values') } else { - assert.notStrictEqual(toStat.mtime.getTime(), fromStat.mtime.getTime()) + assert.notStrictEqual(toStat.mtime.getTime(), fromStat.mtime.getTime(), 'same mtime values') // the access time might actually be the same, so check only modification time } } diff --git a/lib/copy/__tests__/copyfile-fallback.test.js b/lib/copy/__tests__/copyfile-fallback.test.js new file mode 100644 index 00000000..3340ca77 --- /dev/null +++ b/lib/copy/__tests__/copyfile-fallback.test.js @@ -0,0 +1,29 @@ +const fs = require('graceful-fs') +const path = require('path') + +/* global describe, beforeEach, afterEach */ + +if (typeof fs.copyFile === 'function') { + // Also load copy tests without fs.copyFile + describe('> when using copyFileFallback', () => { + const originalCopyFile = fs.copyFile + + beforeEach(() => { + // reset stubs + delete fs.copyFile + }) + + afterEach(() => { + fs.copyFile = originalCopyFile + }) + + const fallbackTest = path.basename(__filename) + fs.readdirSync(__dirname) + .filter(filename => filename.endsWith('.test.js') && filename !== fallbackTest) + .map(filename => { + const testModule = `./${filename}` + delete require.cache[require.resolve(testModule)] + require(testModule) + }) + }) +} diff --git a/lib/copy/copy.js b/lib/copy/copy.js index 411fc64c..20a524cb 100644 --- a/lib/copy/copy.js +++ b/lib/copy/copy.js @@ -4,7 +4,7 @@ const fs = require('graceful-fs') const path = require('path') const mkdirp = require('../mkdirs').mkdirs const pathExists = require('../path-exists').pathExists -const utimes = require('../util/utimes').utimesMillis +const utimesMillis = require('../util/utimes').utimesMillis const stat = require('../util/stat') function copy (src, dest, opts, cb) { @@ -95,7 +95,16 @@ 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) + if (opts.preserveTimestamps && (srcStat.mode & 0o200) === 0) { + // Make sure the file is writable before setting the timestamp + // otherwise openSync fails with EPERM when invoked with 'r+' + // (through utimes call) + return fs.chmod(dest, srcStat.mode | 0o200, (err) => { + if (err) return cb(err) + return setDestTimestampsAndMode(srcStat, src, dest, opts, cb) + }) + } + return setDestTimestampsAndMode(srcStat, src, dest, opts, cb) }) } return copyFileFallback(srcStat, src, dest, opts, cb) @@ -104,21 +113,27 @@ function copyFile (srcStat, src, dest, opts, cb) { function copyFileFallback (srcStat, src, dest, opts, cb) { const rs = fs.createReadStream(src) rs.on('error', err => cb(err)).once('open', () => { - const ws = fs.createWriteStream(dest, { mode: srcStat.mode }) + // explicitly not setting srcStat mode - will do that last + const ws = fs.createWriteStream(dest) ws.on('error', err => cb(err)) .on('open', () => rs.pipe(ws)) - .once('close', () => setDestModeAndTimestamps(srcStat, dest, opts, cb)) + .once('close', () => setDestTimestampsAndMode(srcStat, src, dest, opts, cb)) }) } -function setDestModeAndTimestamps (srcStat, dest, opts, cb) { - fs.chmod(dest, srcStat.mode, err => { - if (err) return cb(err) - if (opts.preserveTimestamps) { - return utimes(dest, srcStat.atime, srcStat.mtime, cb) - } - return cb() - }) +function setDestTimestampsAndMode (srcStat, src, dest, opts, cb) { + if (opts.preserveTimestamps) { + // The initial srcStat.atime cannot be trusted because it is modified by the read(2) system call + // (See https://nodejs.org/api/fs.html#fs_stat_time_values) + return fs.stat(src, (err, updatedSrcStat) => { + if (err) return cb(err) + return utimesMillis(dest, updatedSrcStat.atime, updatedSrcStat.mtime, (err2) => { + if (err2) return cb(err2) + return fs.chmod(dest, srcStat.mode, cb) + }) + }) + } + return fs.chmod(dest, srcStat.mode, cb) } function onDir (srcStat, destStat, src, dest, opts, cb) {