Apply code review suggestions from @RyanZim

lib/copy/copy.js

@@ -7,12 +7,14 @@ const { pathExists } = require('../path-exists')
 const { utimesMillisAsync } = require('../util/utimes')
 const stat = require('../util/stat')
 
-async function copy (src, dest, opts) {
+async function copy(src, dest, opts = {}) {
   if (typeof opts === 'function') {
     opts = { filter: opts }
   }
 
-  opts = opts || {}
+  opts = typeof opts === 'function'
+    ? { filter: opts }
+    : opts
 
   opts.clobber = 'clobber' in opts ? !!opts.clobber : true // default to true for now
   opts.overwrite = 'overwrite' in opts ? !!opts.overwrite : opts.clobber // overwrite falls back to clobber
@@ -37,27 +39,24 @@ async function copy (src, dest, opts) {
   return checkParentDir(destStat, src, dest, opts)
 }
 
-async function checkParentDir (destStat, src, dest, opts) {
+async function checkParentDir(destStat, src, dest, opts) {
   const destParent = path.dirname(dest)
 
   const dirExists = await pathExists(destParent)
 
-  if (dirExists) return getStats(destStat, src, dest, opts)
-
-  const parentDirExists = await pathExists(destParent)
-  if (parentDirExists) return getStats(destStat, src, dest, opts)
-
-  await mkdirs(destParent)
+  if (!dirExists) {
+    await mkdirs(destParent)
+  }
 
   return getStats(destStat, src, dest, opts)
 }
 
-async function runFilter (src, dest, opts) {
+async function runFilter(src, dest, opts) {
   if (!opts.filter) return true
   return opts.filter(src, dest)
 }
 
-async function getStats (destStat, src, dest, opts) {
+async function getStats(destStat, src, dest, opts) {
   const statFn = opts.dereference ? fs.stat : fs.lstat
   const srcStat = await statFn(src)
 
@@ -75,12 +74,9 @@ async function getStats (destStat, src, dest, opts) {
   throw new Error(`Unknown file: ${src}`)
 }
 
-function onFile (srcStat, destStat, src, dest, opts) {
+async function onFile(srcStat, destStat, src, dest, opts) {
   if (!destStat) return copyFile(srcStat, src, dest, opts)
-  return mayCopyFile(srcStat, src, dest, opts)
-}
 
-async function mayCopyFile (srcStat, src, dest, opts) {
   if (opts.overwrite) {
     await fs.unlink(dest)
     return copyFile(srcStat, src, dest, opts)
@@ -90,75 +86,68 @@ async function mayCopyFile (srcStat, src, dest, opts) {
   }
 }
 
-async function copyFile (srcStat, src, dest, opts) {
+async function copyFile(srcStat, src, dest, opts) {
   await fs.copyFile(src, dest)
   if (opts.preserveTimestamps) {
     return handleTimestampsAndMode(srcStat.mode, src, dest)
   }
   return setDestMode(dest, srcStat.mode)
 }
 
-async function handleTimestampsAndMode (srcMode, src, dest) {
+async function handleTimestampsAndMode(srcMode, src, dest) {
   // Make sure the file is writable before setting the timestamp
   // otherwise open fails with EPERM when invoked with 'r+'
   // (through utimes call)
   if (fileIsNotWritable(srcMode)) {
     await makeFileWritable(dest, srcMode)
-    return setDestTimestampsAndMode(srcMode, src, dest)
   }
   return setDestTimestampsAndMode(srcMode, src, dest)
 }
 
-function fileIsNotWritable (srcMode) {
+function fileIsNotWritable(srcMode) {
   return (srcMode & 0o200) === 0
 }
 
-function makeFileWritable (dest, srcMode) {
+function makeFileWritable(dest, srcMode) {
   return setDestMode(dest, srcMode | 0o200)
 }
 
-async function setDestTimestampsAndMode (srcMode, src, dest) {
-  await setDestTimestamps(src, dest)
-  return setDestMode(dest, srcMode)
-}
-
-function setDestMode (dest, srcMode) {
-  return fs.chmod(dest, srcMode)
-}
-
-async function setDestTimestamps (src, dest) {
+async function setDestTimestampsAndMode(srcMode, src, dest) {
   // 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)
+  // (See https://nodejs.org/api/fs.html#fs_stat_time_values)
   const updatedSrcStat = await fs.stat(src)
+  await utimesMillisAsync(dest, updatedSrcStat.atime, updatedSrcStat.mtime)
 
-  return utimesMillisAsync(dest, updatedSrcStat.atime, updatedSrcStat.mtime)
+  return setDestMode(dest, srcMode)
 }
 
-function onDir (srcStat, destStat, src, dest, opts) {
-  if (!destStat) return mkDirAndCopy(srcStat.mode, src, dest, opts)
-  return copyDir(src, dest, opts)
+function setDestMode(dest, srcMode) {
+  return fs.chmod(dest, srcMode)
 }
 
-async function mkDirAndCopy (srcMode, src, dest, opts) {
-  await fs.mkdir(dest)
+async function onDir(srcStat, destStat, src, dest, opts) {
+  if (!destStat) {
+    await fs.mkdir(dest)
+  }
   await copyDir(src, dest, opts)
-
-  return setDestMode(dest, srcMode)
+  if (!destStat) {
+    await setDestMode(dest, srcMode)
+  }
 }
 
-async function copyDir (src, dest, opts) {
+async function copyDir(src, dest, opts) {
   const items = await fs.readdir(src)
   return copyDirItems(items, src, dest, opts)
 }
 
-function copyDirItems (items, src, dest, opts) {
+function copyDirItems(items, src, dest, opts) {
   const item = items.pop()
   if (!item) return
   return copyDirItem(items, item, src, dest, opts)
 }
 
-async function copyDirItem (items, item, src, dest, opts) {
+async function copyDirItem(items, item, src, dest, opts) {
   const srcItem = path.join(src, item)
   const destItem = path.join(dest, item)
 
@@ -172,7 +161,7 @@ async function copyDirItem (items, item, src, dest, opts) {
   return copyDirItems(items, src, dest, opts)
 }
 
-async function onLink (destStat, src, dest, opts) {
+async function onLink(destStat, src, dest, opts) {
   let resolvedSrc = await fs.readlink(src)
   if (opts.dereference) {
     resolvedSrc = path.resolve(process.cwd(), resolvedSrc)
@@ -185,6 +174,9 @@ async function onLink (destStat, src, dest, opts) {
   try {
     resolvedDest = await fs.readlink(dest)
   } catch (e) {
+    // dest exists and is a regular file or directory,
+    // Windows may throw UNKNOWN error. If dest already exists,
+    // fs throws error anyway, so no need to guard against it here.
     if (e.code === 'EINVAL' || e.code === 'UNKNOWN') return fs.symlink(resolvedSrc, dest)
     throw e
   }
@@ -202,10 +194,6 @@ async function onLink (destStat, src, dest, opts) {
     throw new Error(`Cannot overwrite '${resolvedDest}' with '${resolvedSrc}'.`)
   }
 
-  return copyLink(resolvedSrc, dest)
-}
-
-async function copyLink (resolvedSrc, dest) {
   await fs.unlink(dest)
   return fs.symlink(resolvedSrc, dest)
 }