From bc8ee0bf340f04fad0fed408bbdd0cc3747e9d0c Mon Sep 17 00:00:00 2001
From: SukkaW <isukkaw@gmail.com>
Date: Sat, 21 Oct 2023 23:48:49 +0800
Subject: [PATCH] Apply code review suggestions from @RyanZim

---
 lib/copy/copy.js | 58 +++++++++++++++++++-----------------------------
 1 file changed, 23 insertions(+), 35 deletions(-)

diff --git a/lib/copy/copy.js b/lib/copy/copy.js
index 3f096759..621efd5c 100644
--- a/lib/copy/copy.js
+++ b/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
@@ -42,12 +44,9 @@ async function checkParentDir (destStat, src, dest, opts) {
 
   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)
 }
@@ -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)
@@ -104,7 +100,6 @@ async function handleTimestampsAndMode (srcMode, src, dest) {
   // (through utimes call)
   if (fileIsNotWritable(srcMode)) {
     await makeFileWritable(dest, srcMode)
-    return setDestTimestampsAndMode(srcMode, src, dest)
   }
   return setDestTimestampsAndMode(srcMode, src, dest)
 }
@@ -118,33 +113,27 @@ function makeFileWritable (dest, srcMode) {
 }
 
 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) {
   // 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, srcStat.mode)
+  }
 }
 
 async function copyDir (src, dest, opts) {
@@ -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)
 }