Workaround fs.utimes and util/utimeMillis catch-22

jprichardson · Jul 31, 2019 · fb89a29 · fb89a29
1 parent 2b7c0e5
commit fb89a29
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 40 deletions.
diff --git a/lib/copy-sync/copy-sync.js b/lib/copy-sync/copy-sync.js
@@ -5,6 +5,7 @@
 const fs = require('fs')
 const path = require('path')
 const mkdirpSync = require('../mkdirs').mkdirsSync
+const utimesMillisSync = require('../util/utimes').utimesMillisSync
 const stat = require('../util/stat')
 
 function copySync (src, dest, opts) {
@@ -67,7 +68,13 @@ function mayCopyFile (srcStat, src, dest, opts) {
 function copyFile (srcStat, src, dest, opts) {
   if (typeof fs.copyFileSync === 'function') {
     fs.copyFileSync(src, dest)
-    return setDestTimestampsAndMode(srcStat, src, dest, opts)
+    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 setDestTimestampsAndMode(srcStat, dest, opts)
   }
   return copyFileFallback(srcStat, src, dest, opts)
 }
@@ -77,7 +84,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) {
@@ -86,31 +93,19 @@ function copyFileFallback (srcStat, src, dest, opts) {
     pos += bytesRead
   }
 
-  setFdDestTimestampsAndMode(srcStat, src, fdw, opts)
+  setDestTimestampsAndMode(srcStat, fdw, opts)
 
   fs.closeSync(fdr)
   fs.closeSync(fdw)
 }
 
-function setDestTimestampsAndMode (srcStat, src, dest, opts) {
-  if ((srcStat.mode & 0o200) === 0) {
-    // Make sure the file is writable before setting the timestamp
-    // otherwise openSync below fails with EPERM when invoked with 'r+'
-    fs.chmodSync(dest, srcStat.mode | 0o200)
-  }
-  const fdw = fs.openSync(dest, 'r+')
-  setFdDestTimestampsAndMode(srcStat, src, fdw, opts)
-  fs.closeSync(fdw)
-}
-
-function setFdDestTimestampsAndMode (srcStat, src, fdw, opts) {
+function setDestTimestampsAndMode (srcStat, dest, opts) {
+  const utimesSync = typeof dest === 'string' ? utimesMillisSync : fs.futimesSync
+  const chmodSync = typeof dest === 'string' ? fs.chmodSync : fs.fchmodSync
   if (opts.preserveTimestamps) {
-    // 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)
-    fs.futimesSync(fdw, updatedSrcStat.atime, updatedSrcStat.mtime)
+    utimesSync(dest, srcStat.atime, srcStat.mtime)
   }
-  fs.fchmodSync(fdw, srcStat.mode)
+  chmodSync(dest, srcStat.mode)
 }
 
 function onDir (srcStat, destStat, src, dest, opts) {

diff --git a/lib/copy/copy.js b/lib/copy/copy.js
@@ -6,7 +6,7 @@ const fs = require('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) {
@@ -97,17 +97,16 @@ function copyFile (srcStat, src, dest, opts, cb) {
   if (typeof fs.copyFile === 'function') {
     return fs.copyFile(src, dest, err => {
       if (err) return cb(err)
-      if ((srcStat.mode & 0o200) === 0) {
+      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 fs.chmod(dest, srcStat.mode | 0o200, (err) => {
+          if (err) return cb(err)
+          return setDestTimestampsAndMode(srcStat, dest, opts, cb)
+        })
       }
-      return setDestTimestampsAndMode(srcStat, src, dest, opts, cb)
+      return setDestTimestampsAndMode(srcStat, dest, opts, cb)
     })
   }
   return copyFileFallback(srcStat, src, dest, opts, cb)
@@ -120,24 +119,20 @@ function copyFileFallback (srcStat, src, dest, opts, cb) {
     const ws = fs.createWriteStream(dest)
     ws.on('error', err => cb(err))
       .on('open', () => rs.pipe(ws))
-      .once('close', () => setDestTimestampsAndMode(srcStat, src, dest, opts, cb))
+      .once('close', () => setDestTimestampsAndMode(srcStat, dest, opts, cb))
   })
 }
 
-function setDestTimestampsAndMode (srcStat, src, dest, opts, cb) {
-  if (!opts.preserveTimestamps) {
-    return fs.chmod(dest, srcStat.mode, 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)
+function setDestTimestampsAndMode (srcStat, dest, opts, cb) {
+  const utimes = typeof dest === 'string' ? utimesMillis : fs.futimes
+  const chmod = typeof dest === 'string' ? fs.chmod : fs.fchmod
+  if (opts.preserveTimestamps) {
     return utimes(dest, srcStat.atime, srcStat.mtime, (err) => {
       if (err) return cb(err)
-      return fs.chmod(dest, srcStat.mode, cb)
+      return chmod(dest, srcStat.mode, cb)
     })
-  })
+  }
+  return chmod(dest, srcStat.mode, cb)
 }
 
 function onDir (srcStat, destStat, src, dest, opts, cb) {