Factor out logic to setDestTimestampsAndMode and avoid unnecessary fu…

…nction nesting
jprichardson · Jun 1, 2019 · f55125e · f55125e
1 parent bd947fd
commit f55125e
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 35 deletions.
diff --git a/lib/copy-sync/copy-sync.js b/lib/copy-sync/copy-sync.js
@@ -5,7 +5,6 @@
 const fs = require('fs')
 const path = require('path')
 const mkdirpSync = require('../mkdirs').mkdirsSync
-const utimesSync = require('../util/utimes.js').utimesMillisSync
 const stat = require('../util/stat')
 
 function copySync (src, dest, opts) {
@@ -68,18 +67,7 @@ function mayCopyFile (srcStat, src, dest, opts) {
 function copyFile (srcStat, src, dest, opts) {
   if (typeof fs.copyFileSync === 'function') {
     fs.copyFileSync(src, dest)
-    if (opts.preserveTimestamps) {
-      // 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 setDestTimestampsAndMode(srcStat, src, dest, opts)
   }
   return copyFileFallback(srcStat, src, dest, opts)
 }
@@ -98,13 +86,29 @@ function copyFileFallback (srcStat, src, dest, opts) {
     pos += bytesRead
   }
 
+  setFdDestTimestampsAndMode(srcStat, src, fdw, opts)
+
+  fs.closeSync(fdr)
+  fs.closeSync(fdw)
+}
+
+function setDestTimestampsAndMode (srcStat, src, dest, opts) {
+  if ((srcStat.mode & 0o200) === 0) {
+    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) {
   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)
   }
-
-  fs.closeSync(fdr)
-  fs.closeSync(fdw)
+  fs.fchmodSync(fdw, srcStat.mode)
 }
 
 function onDir (srcStat, destStat, src, dest, opts) {

diff --git a/lib/copy/copy.js b/lib/copy/copy.js
@@ -97,7 +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)
-      return setDestModeAndTimestamps(srcStat, src, dest, opts, cb)
+      if ((srcStat.mode & 0o200) === 0) {
+        // Make sure the file is writable before setting the timestamp
+        // otherwise utimes fails with EPERM
+        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)
@@ -106,36 +115,28 @@ 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, src, dest, opts, cb))
+      .once('close', () => setDestTimestampsAndMode(srcStat, src, dest, opts, cb))
   })
 }
 
-function setDestModeAndTimestamps (srcStat, src, dest, opts, cb) {
+function setDestTimestampsAndMode (srcStat, src, dest, opts, cb) {
   if (!opts.preserveTimestamps) {
     return fs.chmod(dest, srcStat.mode, cb)
   }
 
-  const next = (err) => {
+  // 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)
-    // 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) => {
+    return utimes(dest, srcStat.atime, srcStat.mtime, (err) => {
       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)
-      })
+      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) {