Merge d288d84 into d1a01e7

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
       }
     }

lib/copy-sync/__tests__/copyfilesync-fallback.test.js

@@ -0,0 +1,28 @@
+const fs = require('fs')
+const path = require('path')
+
+/* global describe, beforeEach, afterEach */
+
+if (typeof fs.copyFile === 'function') {
+  // Also load copy-sync tests without fs.copyFileSync
+  describe('> when using copyFileSyncFallback', () => {
+    const originalCopyFileSync = fs.copyFileSync
+
+    beforeEach(() => {
+      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)
+      })
+  })
+}

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) {
@@ -63,15 +63,22 @@ function mayCopyFile (srcStat, src, dest, opts) {
   }
 }
 
+let copyFileUsed = false
+let copyFileFallbackUsed = false
+
 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
+    if (!copyFileUsed) { copyFileUsed = true; console.log('copyFile used (sync)') }
+    return setDestTimestampsAndMode(srcStat, src, dest, opts)
   }
+  if (!copyFileFallbackUsed) { copyFileFallbackUsed = true; console.log('copyFileFallback used (sync)') }
   return copyFileFallback(srcStat, src, dest, opts)
 }
 
@@ -80,7 +87,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 +96,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, srcStat.mtime)
+  }
+  chmodSync(dest, srcStat.mode)
+}
+
 function onDir (srcStat, destStat, src, dest, opts) {
   if (!destStat) return mkDirAndCopy(srcStat, src, dest, opts)
   if (destStat && !destStat.isDirectory()) {

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
       }
     }

lib/copy/__tests__/copyfile-fallback.test.js

@@ -0,0 +1,28 @@
+const fs = require('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(() => {
+      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)
+      })
+  })
+}

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) {
@@ -91,34 +91,56 @@ function mayCopyFile (srcStat, src, dest, opts, cb) {
   } else return cb()
 }
 
+let copyFileUsed = false
+let copyFileFallbackUsed = false
+
 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)
+        })
+      }
+      if (!copyFileUsed) { copyFileUsed = true; console.log('copyFile used') }
+      return setDestTimestampsAndMode(srcStat, src, dest, opts, cb)
     })
   }
+  if (!copyFileFallbackUsed) { copyFileFallbackUsed = true; console.log('copyFileFallback used') }
   return copyFileFallback(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) {
+  const utimes = typeof dest === 'string' ? utimesMillis : fs.futimes
+  const chmod = typeof dest === 'string' ? fs.chmod : fs.fchmod
+  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 utimes(dest, updatedSrcStat.atime, srcStat.mtime, (err2) => {
+        if (err2) return cb(err2)
+        return chmod(dest, srcStat.mode, cb)
+      })
+    })
+  }
+  return chmod(dest, srcStat.mode, cb)
 }
 
 function onDir (srcStat, destStat, src, dest, opts, cb) {