From 56428e3fac8456046a37b2c0b27215ac79e75422 Mon Sep 17 00:00:00 2001 From: SukkaW Date: Thu, 19 Oct 2023 23:24:01 +0800 Subject: [PATCH 01/15] Refactor `copy` API in async/await --- lib/copy/copy.js | 312 ++++++++++++++++++++++----------------------- lib/copy/index.js | 2 +- lib/util/stat.js | 55 ++++++++ lib/util/utimes.js | 34 +++++ 4 files changed, 240 insertions(+), 163 deletions(-) diff --git a/lib/copy/copy.js b/lib/copy/copy.js index bc188fb63..7dca983a4 100644 --- a/lib/copy/copy.js +++ b/lib/copy/copy.js @@ -2,20 +2,29 @@ const fs = require('graceful-fs') const path = require('path') -const mkdirs = require('../mkdirs').mkdirs -const pathExists = require('../path-exists').pathExists -const utimesMillis = require('../util/utimes').utimesMillis +const { mkdirs } = require('../mkdirs') +const { pathExists } = require('../path-exists') +const { utimesMillisAsync } = require('../util/utimes') const stat = require('../util/stat') -function copy (src, dest, opts, cb) { - if (typeof opts === 'function' && !cb) { - cb = opts - opts = {} - } else if (typeof opts === 'function') { +const { promisify } = require('util') + +// TODO: re-exports promisified version of graceful-fs functions from lib/fs/promise.js +const fs$stat = promisify(fs.stat) +const fs$lstat = promisify(fs.lstat) +const fs$unlink = promisify(fs.unlink) +const fs$copyFile = promisify(fs.copyFile) +const fs$mkdir = promisify(fs.mkdir) +const fs$chmod = promisify(fs.chmod) +const fs$readdir = promisify(fs.readdir) +const fs$readlink = promisify(fs.readlink) +const fs$symlink = promisify(fs.symlink) + +async function copy (src, dest, opts) { + if (typeof opts === 'function') { opts = { filter: opts } } - cb = cb || function () {} opts = opts || {} opts.clobber = 'clobber' in opts ? !!opts.clobber : true // default to true for now @@ -30,209 +39,188 @@ function copy (src, dest, opts, cb) { ) } - stat.checkPaths(src, dest, 'copy', opts, (err, stats) => { - if (err) return cb(err) - const { srcStat, destStat } = stats - stat.checkParentPaths(src, srcStat, dest, 'copy', err => { - if (err) return cb(err) - runFilter(src, dest, opts, (err, include) => { - if (err) return cb(err) - if (!include) return cb() + const { srcStat, destStat } = await stat.checkPathsAsync(src, dest, 'copy', opts) - checkParentDir(destStat, src, dest, opts, cb) - }) - }) - }) + await stat.checkParentPathsAsync(src, srcStat, dest, 'copy') + + const include = await runFilter(src, dest, opts) + + if (!include) return + + return checkParentDir(destStat, src, dest, opts) } -function checkParentDir (destStat, src, dest, opts, cb) { +async function checkParentDir (destStat, src, dest, opts) { const destParent = path.dirname(dest) - pathExists(destParent, (err, dirExists) => { - if (err) return cb(err) - if (dirExists) return getStats(destStat, src, dest, opts, cb) - mkdirs(destParent, err => { - if (err) return cb(err) - return getStats(destStat, src, dest, opts, cb) - }) - }) + + 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) + + return getStats(destStat, src, dest, opts) } -function runFilter (src, dest, opts, cb) { - if (!opts.filter) return cb(null, true) - Promise.resolve(opts.filter(src, dest)) - .then(include => cb(null, include), error => cb(error)) +async function runFilter (src, dest, opts) { + if (!opts.filter) return true + return opts.filter(src, dest) } -function getStats (destStat, src, dest, opts, cb) { - const stat = opts.dereference ? fs.stat : fs.lstat - stat(src, (err, srcStat) => { - if (err) return cb(err) +async function getStats (destStat, src, dest, opts) { + const statFn = opts.dereference ? fs$stat : fs$lstat + const srcStat = await statFn(src) - if (srcStat.isDirectory()) return onDir(srcStat, destStat, src, dest, opts, cb) - else if (srcStat.isFile() || - srcStat.isCharacterDevice() || - srcStat.isBlockDevice()) return onFile(srcStat, destStat, src, dest, opts, cb) - else if (srcStat.isSymbolicLink()) return onLink(destStat, src, dest, opts, cb) - else if (srcStat.isSocket()) return cb(new Error(`Cannot copy a socket file: ${src}`)) - else if (srcStat.isFIFO()) return cb(new Error(`Cannot copy a FIFO pipe: ${src}`)) - return cb(new Error(`Unknown file: ${src}`)) - }) + if (srcStat.isDirectory()) return onDir(srcStat, destStat, src, dest, opts) + + if ( + srcStat.isFile() || + srcStat.isCharacterDevice() || + srcStat.isBlockDevice() + ) return onFile(srcStat, destStat, src, dest, opts) + + if (srcStat.isSymbolicLink()) return onLink(destStat, src, dest, opts) + if (srcStat.isSocket()) throw new Error(`Cannot copy a socket file: ${src}`) + if (srcStat.isFIFO()) throw new Error(`Cannot copy a FIFO pipe: ${src}`) + throw new Error(`Unknown file: ${src}`) } -function onFile (srcStat, destStat, src, dest, opts, cb) { - if (!destStat) return copyFile(srcStat, src, dest, opts, cb) - return mayCopyFile(srcStat, src, dest, opts, cb) +function onFile (srcStat, destStat, src, dest, opts) { + if (!destStat) return copyFile(srcStat, src, dest, opts) + return mayCopyFile(srcStat, src, dest, opts) } -function mayCopyFile (srcStat, src, dest, opts, cb) { +async function mayCopyFile (srcStat, src, dest, opts) { if (opts.overwrite) { - fs.unlink(dest, err => { - if (err) return cb(err) - return copyFile(srcStat, src, dest, opts, cb) - }) - } else if (opts.errorOnExist) { - return cb(new Error(`'${dest}' already exists`)) - } else return cb() + await fs$unlink(dest) + return copyFile(srcStat, src, dest, opts) + } + if (opts.errorOnExist) { + throw new Error(`'${dest}' already exists`) + } } -function copyFile (srcStat, src, dest, opts, cb) { - fs.copyFile(src, dest, err => { - if (err) return cb(err) - if (opts.preserveTimestamps) return handleTimestampsAndMode(srcStat.mode, src, dest, cb) - return setDestMode(dest, srcStat.mode, cb) - }) +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) } -function handleTimestampsAndMode (srcMode, src, dest, cb) { +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)) { - return makeFileWritable(dest, srcMode, err => { - if (err) return cb(err) - return setDestTimestampsAndMode(srcMode, src, dest, cb) - }) + await makeFileWritable(dest, srcMode) + return setDestTimestampsAndMode(srcMode, src, dest) } - return setDestTimestampsAndMode(srcMode, src, dest, cb) + return setDestTimestampsAndMode(srcMode, src, dest) } function fileIsNotWritable (srcMode) { return (srcMode & 0o200) === 0 } -function makeFileWritable (dest, srcMode, cb) { - return setDestMode(dest, srcMode | 0o200, cb) +function makeFileWritable (dest, srcMode) { + return setDestMode(dest, srcMode | 0o200) } -function setDestTimestampsAndMode (srcMode, src, dest, cb) { - setDestTimestamps(src, dest, err => { - if (err) return cb(err) - return setDestMode(dest, srcMode, cb) - }) +async function setDestTimestampsAndMode (srcMode, src, dest) { + await setDestTimestamps(src, dest) + return setDestMode(dest, srcMode) } -function setDestMode (dest, srcMode, cb) { - return fs.chmod(dest, srcMode, cb) +function setDestMode (dest, srcMode) { + return fs$chmod(dest, srcMode) } -function setDestTimestamps (src, dest, cb) { +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) - fs.stat(src, (err, updatedSrcStat) => { - if (err) return cb(err) - return utimesMillis(dest, updatedSrcStat.atime, updatedSrcStat.mtime, cb) - }) + // (See https://nodejs.org/api/fs.html#fs$stat_time_values) + const updatedSrcStat = await fs$stat(src) + + return utimesMillisAsync(dest, updatedSrcStat.atime, updatedSrcStat.mtime) } -function onDir (srcStat, destStat, src, dest, opts, cb) { - if (!destStat) return mkDirAndCopy(srcStat.mode, src, dest, opts, cb) - return copyDir(src, dest, opts, cb) +function onDir (srcStat, destStat, src, dest, opts) { + if (!destStat) return mkDirAndCopy(srcStat.mode, src, dest, opts) + return copyDir(src, dest, opts) } -function mkDirAndCopy (srcMode, src, dest, opts, cb) { - fs.mkdir(dest, err => { - if (err) return cb(err) - copyDir(src, dest, opts, err => { - if (err) return cb(err) - return setDestMode(dest, srcMode, cb) - }) - }) +async function mkDirAndCopy (srcMode, src, dest, opts) { + await fs$mkdir(dest) + await copyDir(src, dest, opts) + + return setDestMode(dest, srcMode) } -function copyDir (src, dest, opts, cb) { - fs.readdir(src, (err, items) => { - if (err) return cb(err) - return copyDirItems(items, src, dest, opts, cb) - }) +async function copyDir (src, dest, opts) { + const items = await fs$readdir(src) + return copyDirItems(items, src, dest, opts) } -function copyDirItems (items, src, dest, opts, cb) { +function copyDirItems (items, src, dest, opts) { const item = items.pop() - if (!item) return cb() - return copyDirItem(items, item, src, dest, opts, cb) + if (!item) return + return copyDirItem(items, item, src, dest, opts) } -function copyDirItem (items, item, src, dest, opts, cb) { +async function copyDirItem (items, item, src, dest, opts) { const srcItem = path.join(src, item) const destItem = path.join(dest, item) - runFilter(srcItem, destItem, opts, (err, include) => { - if (err) return cb(err) - if (!include) return copyDirItems(items, src, dest, opts, cb) - - stat.checkPaths(srcItem, destItem, 'copy', opts, (err, stats) => { - if (err) return cb(err) - const { destStat } = stats - getStats(destStat, srcItem, destItem, opts, err => { - if (err) return cb(err) - return copyDirItems(items, src, dest, opts, cb) - }) - }) - }) -} - -function onLink (destStat, src, dest, opts, cb) { - fs.readlink(src, (err, resolvedSrc) => { - if (err) return cb(err) - if (opts.dereference) { - resolvedSrc = path.resolve(process.cwd(), resolvedSrc) - } - - if (!destStat) { - return fs.symlink(resolvedSrc, dest, cb) - } else { - fs.readlink(dest, (err, resolvedDest) => { - if (err) { - // 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 (err.code === 'EINVAL' || err.code === 'UNKNOWN') return fs.symlink(resolvedSrc, dest, cb) - return cb(err) - } - if (opts.dereference) { - resolvedDest = path.resolve(process.cwd(), resolvedDest) - } - if (stat.isSrcSubdir(resolvedSrc, resolvedDest)) { - return cb(new Error(`Cannot copy '${resolvedSrc}' to a subdirectory of itself, '${resolvedDest}'.`)) - } - - // do not copy if src is a subdir of dest since unlinking - // dest in this case would result in removing src contents - // and therefore a broken symlink would be created. - if (stat.isSrcSubdir(resolvedDest, resolvedSrc)) { - return cb(new Error(`Cannot overwrite '${resolvedDest}' with '${resolvedSrc}'.`)) - } - return copyLink(resolvedSrc, dest, cb) - }) - } - }) -} - -function copyLink (resolvedSrc, dest, cb) { - fs.unlink(dest, err => { - if (err) return cb(err) - return fs.symlink(resolvedSrc, dest, cb) - }) + + const include = await runFilter(srcItem, destItem, opts) + if (!include) return copyDirItems(items, src, dest, opts) + + const { destStat } = await stat.checkPathsAsync(srcItem, destItem, 'copy', opts) + + await getStats(destStat, srcItem, destItem, opts) + + return copyDirItems(items, 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) + } + if (!destStat) { + return fs$symlink(resolvedSrc, dest) + } + + let resolvedDest = null + try { + resolvedDest = await fs$readlink(dest) + } catch (e) { + if (e.code === 'EINVAL' || e.code === 'UNKNOWN') return fs$symlink(resolvedSrc, dest) + throw e + } + if (opts.dereference) { + resolvedDest = path.resolve(process.cwd(), resolvedDest) + } + if (stat.isSrcSubdir(resolvedSrc, resolvedDest)) { + throw new Error(`Cannot copy '${resolvedSrc}' to a subdirectory of itself, '${resolvedDest}'.`) + } + + // do not copy if src is a subdir of dest since unlinking + // dest in this case would result in removing src contents + // and therefore a broken symlink would be created. + if (stat.isSrcSubdir(resolvedDest, resolvedSrc)) { + 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) } module.exports = copy diff --git a/lib/copy/index.js b/lib/copy/index.js index 45c07a2fb..2e31d2786 100644 --- a/lib/copy/index.js +++ b/lib/copy/index.js @@ -1,6 +1,6 @@ 'use strict' -const u = require('universalify').fromCallback +const u = require('universalify').fromPromise module.exports = { copy: u(require('./copy')), copySync: require('./copy-sync') diff --git a/lib/util/stat.js b/lib/util/stat.js index 2efd1c33f..e375bfff5 100644 --- a/lib/util/stat.js +++ b/lib/util/stat.js @@ -32,6 +32,7 @@ function getStatsSync (src, dest, opts) { return { srcStat, destStat } } +// TODO: remove callback-based internal utility `checkPaths` once all API has migrated to Promise-based `checkPathsAsync` function checkPaths (src, dest, funcName, opts, cb) { util.callbackify(getStats)(src, dest, opts, (err, stats) => { if (err) return cb(err) @@ -63,6 +64,34 @@ function checkPaths (src, dest, funcName, opts, cb) { }) } +async function checkPathsAsync (src, dest, funcName, opts) { + const { srcStat, destStat } = await getStats(src, dest, opts) + if (destStat) { + if (areIdentical(srcStat, destStat)) { + const srcBaseName = path.basename(src) + const destBaseName = path.basename(dest) + if (funcName === 'move' && + srcBaseName !== destBaseName && + srcBaseName.toLowerCase() === destBaseName.toLowerCase()) { + return { srcStat, destStat, isChangingCase: true } + } + throw new Error('Source and destination must not be the same.') + } + if (srcStat.isDirectory() && !destStat.isDirectory()) { + throw new Error(`Cannot overwrite non-directory '${dest}' with directory '${src}'.`) + } + if (!srcStat.isDirectory() && destStat.isDirectory()) { + throw new Error(`Cannot overwrite directory '${dest}' with non-directory '${src}'.`) + } + } + + if (srcStat.isDirectory() && isSrcSubdir(src, dest)) { + throw new Error(errMsg(src, dest, funcName)) + } + + return { srcStat, destStat } +} + function checkPathsSync (src, dest, funcName, opts) { const { srcStat, destStat } = getStatsSync(src, dest, opts) @@ -95,6 +124,7 @@ function checkPathsSync (src, dest, funcName, opts) { // It works for all file types including symlinks since it // checks the src and dest inodes. It starts from the deepest // parent and stops once it reaches the src parent or the root path. +// TODO: remove callback-based internal utility `checkPaths` once all API has migrated to Promise-based `checkPathsAsync` function checkParentPaths (src, srcStat, dest, funcName, cb) { const srcParent = path.resolve(path.dirname(src)) const destParent = path.resolve(path.dirname(dest)) @@ -111,6 +141,26 @@ function checkParentPaths (src, srcStat, dest, funcName, cb) { }) } +async function checkParentPathsAsync (src, srcStat, dest, funcName) { + const srcParent = path.resolve(path.dirname(src)) + const destParent = path.resolve(path.dirname(dest)) + if (destParent === srcParent || destParent === path.parse(destParent).root) return + + let destStat + try { + destStat = await fs.stat(destParent, { bigint: true }) + } catch (err) { + if (err.code === 'ENOENT') return + throw err + } + + if (areIdentical(srcStat, destStat)) { + throw new Error(errMsg(src, dest, funcName)) + } + + return checkParentPathsAsync(src, srcStat, destParent, funcName) +} + function checkParentPathsSync (src, srcStat, dest, funcName) { const srcParent = path.resolve(path.dirname(src)) const destParent = path.resolve(path.dirname(dest)) @@ -145,10 +195,15 @@ function errMsg (src, dest, funcName) { } module.exports = { + // checkPaths checkPaths, checkPathsSync, + checkPathsAsync, + // checkParent checkParentPaths, checkParentPathsSync, + checkParentPathsAsync, + // Misc isSrcSubdir, areIdentical } diff --git a/lib/util/utimes.js b/lib/util/utimes.js index 75395deff..2cea485d8 100644 --- a/lib/util/utimes.js +++ b/lib/util/utimes.js @@ -1,7 +1,13 @@ 'use strict' const fs = require('graceful-fs') +const { promisify } = require('util') +const open = promisify(fs.open) +const futimes = promisify(fs.futimes) +const close = promisify(fs.close) + +// TODO: remove `utimesMillis` once all internal usage has switched to the Promise-based `utimesMillisAsync` API function utimesMillis (path, atime, mtime, callback) { // if (!HAS_MILLIS_RES) return fs.utimes(path, atime, mtime, callback) fs.open(path, 'r+', (err, fd) => { @@ -14,6 +20,33 @@ function utimesMillis (path, atime, mtime, callback) { }) } +async function utimesMillisAsync (path, atime, mtime) { + // if (!HAS_MILLIS_RES) return fs.utimes(path, atime, mtime, callback) + const fd = await open(path, 'r+') + + let futimesErr = null + try { + await futimes(fd, atime, mtime) + } catch (e) { + futimesErr = e + } + + let closeErr = null + + try { + await close(fd) + } catch (e) { + closeErr = e + } + + if (futimesErr) { + throw futimesErr + } + if (closeErr) { + throw closeErr + } +} + function utimesMillisSync (path, atime, mtime) { const fd = fs.openSync(path, 'r+') fs.futimesSync(fd, atime, mtime) @@ -22,5 +55,6 @@ function utimesMillisSync (path, atime, mtime) { module.exports = { utimesMillis, + utimesMillisAsync, utimesMillisSync } From f04093f998fa5d39ef553a20eb13c0706f853c21 Mon Sep 17 00:00:00 2001 From: SukkaW Date: Thu, 19 Oct 2023 23:45:13 +0800 Subject: [PATCH 02/15] Update test to import `copy` from main exports --- lib/copy/__tests__/copy-broken-symlink.test.js | 2 +- lib/copy/__tests__/ncp/broken-symlink.test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/copy/__tests__/copy-broken-symlink.test.js b/lib/copy/__tests__/copy-broken-symlink.test.js index 81e46dea5..68d989d59 100644 --- a/lib/copy/__tests__/copy-broken-symlink.test.js +++ b/lib/copy/__tests__/copy-broken-symlink.test.js @@ -5,7 +5,7 @@ const os = require('os') const fse = require('../..') const path = require('path') const assert = require('assert') -const copy = require('../copy') +const { copy } = require('../') /* global afterEach, beforeEach, describe, it */ diff --git a/lib/copy/__tests__/ncp/broken-symlink.test.js b/lib/copy/__tests__/ncp/broken-symlink.test.js index 956daa862..7a8db699d 100644 --- a/lib/copy/__tests__/ncp/broken-symlink.test.js +++ b/lib/copy/__tests__/ncp/broken-symlink.test.js @@ -3,7 +3,7 @@ const fs = require('fs') const os = require('os') const fse = require('../../..') -const ncp = require('../../copy') +const { copy: ncp } = require('../../') const path = require('path') const assert = require('assert') From 47db7356a88e588621d7ff14192761e275809f1e Mon Sep 17 00:00:00 2001 From: SukkaW Date: Fri, 20 Oct 2023 10:33:50 +0800 Subject: [PATCH 03/15] Use promisified fs API from `lib/fs` --- lib/copy/copy.js | 41 ++++++++++++++--------------------------- 1 file changed, 14 insertions(+), 27 deletions(-) diff --git a/lib/copy/copy.js b/lib/copy/copy.js index 7dca983a4..3f0967593 100644 --- a/lib/copy/copy.js +++ b/lib/copy/copy.js @@ -1,25 +1,12 @@ 'use strict' -const fs = require('graceful-fs') +const fs = require('../fs') const path = require('path') const { mkdirs } = require('../mkdirs') const { pathExists } = require('../path-exists') const { utimesMillisAsync } = require('../util/utimes') const stat = require('../util/stat') -const { promisify } = require('util') - -// TODO: re-exports promisified version of graceful-fs functions from lib/fs/promise.js -const fs$stat = promisify(fs.stat) -const fs$lstat = promisify(fs.lstat) -const fs$unlink = promisify(fs.unlink) -const fs$copyFile = promisify(fs.copyFile) -const fs$mkdir = promisify(fs.mkdir) -const fs$chmod = promisify(fs.chmod) -const fs$readdir = promisify(fs.readdir) -const fs$readlink = promisify(fs.readlink) -const fs$symlink = promisify(fs.symlink) - async function copy (src, dest, opts) { if (typeof opts === 'function') { opts = { filter: opts } @@ -71,7 +58,7 @@ async function runFilter (src, dest, opts) { } async function getStats (destStat, src, dest, opts) { - const statFn = opts.dereference ? fs$stat : fs$lstat + const statFn = opts.dereference ? fs.stat : fs.lstat const srcStat = await statFn(src) if (srcStat.isDirectory()) return onDir(srcStat, destStat, src, dest, opts) @@ -95,7 +82,7 @@ function onFile (srcStat, destStat, src, dest, opts) { async function mayCopyFile (srcStat, src, dest, opts) { if (opts.overwrite) { - await fs$unlink(dest) + await fs.unlink(dest) return copyFile(srcStat, src, dest, opts) } if (opts.errorOnExist) { @@ -104,7 +91,7 @@ async function mayCopyFile (srcStat, src, dest, opts) { } async function copyFile (srcStat, src, dest, opts) { - await fs$copyFile(src, dest) + await fs.copyFile(src, dest) if (opts.preserveTimestamps) { return handleTimestampsAndMode(srcStat.mode, src, dest) } @@ -136,14 +123,14 @@ async function setDestTimestampsAndMode (srcMode, src, dest) { } function setDestMode (dest, srcMode) { - return fs$chmod(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) - const updatedSrcStat = await fs$stat(src) + const updatedSrcStat = await fs.stat(src) return utimesMillisAsync(dest, updatedSrcStat.atime, updatedSrcStat.mtime) } @@ -154,14 +141,14 @@ function onDir (srcStat, destStat, src, dest, opts) { } async function mkDirAndCopy (srcMode, src, dest, opts) { - await fs$mkdir(dest) + await fs.mkdir(dest) await copyDir(src, dest, opts) return setDestMode(dest, srcMode) } async function copyDir (src, dest, opts) { - const items = await fs$readdir(src) + const items = await fs.readdir(src) return copyDirItems(items, src, dest, opts) } @@ -186,19 +173,19 @@ async function copyDirItem (items, item, src, dest, opts) { } async function onLink (destStat, src, dest, opts) { - let resolvedSrc = await fs$readlink(src) + let resolvedSrc = await fs.readlink(src) if (opts.dereference) { resolvedSrc = path.resolve(process.cwd(), resolvedSrc) } if (!destStat) { - return fs$symlink(resolvedSrc, dest) + return fs.symlink(resolvedSrc, dest) } let resolvedDest = null try { - resolvedDest = await fs$readlink(dest) + resolvedDest = await fs.readlink(dest) } catch (e) { - if (e.code === 'EINVAL' || e.code === 'UNKNOWN') return fs$symlink(resolvedSrc, dest) + if (e.code === 'EINVAL' || e.code === 'UNKNOWN') return fs.symlink(resolvedSrc, dest) throw e } if (opts.dereference) { @@ -219,8 +206,8 @@ async function onLink (destStat, src, dest, opts) { } async function copyLink (resolvedSrc, dest) { - await fs$unlink(dest) - return fs$symlink(resolvedSrc, dest) + await fs.unlink(dest) + return fs.symlink(resolvedSrc, dest) } module.exports = copy From e125aa200ab54e37fe8bada6ffa89162ee2d7769 Mon Sep 17 00:00:00 2001 From: SukkaW Date: Fri, 20 Oct 2023 10:36:32 +0800 Subject: [PATCH 04/15] Update test to import `ncp` from re-exports --- lib/copy/__tests__/copy-preserve-timestamp.test.js | 2 +- lib/copy/__tests__/ncp/ncp-error-perm.test.js | 2 +- lib/copy/__tests__/ncp/ncp.test.js | 2 +- lib/copy/__tests__/ncp/symlink.test.js | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/copy/__tests__/copy-preserve-timestamp.test.js b/lib/copy/__tests__/copy-preserve-timestamp.test.js index 6f1336f42..64ee5a3d5 100644 --- a/lib/copy/__tests__/copy-preserve-timestamp.test.js +++ b/lib/copy/__tests__/copy-preserve-timestamp.test.js @@ -3,7 +3,7 @@ const fs = require('../../') const os = require('os') const path = require('path') -const copy = require('../copy') +const { copy } = require('../') const utimesSync = require('../../util/utimes').utimesMillisSync const assert = require('assert') diff --git a/lib/copy/__tests__/ncp/ncp-error-perm.test.js b/lib/copy/__tests__/ncp/ncp-error-perm.test.js index 34b6af1a6..563e0abaa 100644 --- a/lib/copy/__tests__/ncp/ncp-error-perm.test.js +++ b/lib/copy/__tests__/ncp/ncp-error-perm.test.js @@ -5,7 +5,7 @@ const fs = require('fs') const os = require('os') const fse = require('../../..') -const ncp = require('../../copy') +const { copy: ncp } = require('../../') const path = require('path') const assert = require('assert') diff --git a/lib/copy/__tests__/ncp/ncp.test.js b/lib/copy/__tests__/ncp/ncp.test.js index 2b85437a2..5de57c099 100644 --- a/lib/copy/__tests__/ncp/ncp.test.js +++ b/lib/copy/__tests__/ncp/ncp.test.js @@ -1,7 +1,7 @@ 'use strict' const fs = require('fs') -const ncp = require('../../copy') +const { copy: ncp } = require('../../') const path = require('path') const rimraf = require('rimraf') const assert = require('assert') diff --git a/lib/copy/__tests__/ncp/symlink.test.js b/lib/copy/__tests__/ncp/symlink.test.js index 5c188b6fe..6845ce383 100644 --- a/lib/copy/__tests__/ncp/symlink.test.js +++ b/lib/copy/__tests__/ncp/symlink.test.js @@ -3,7 +3,7 @@ const fs = require('fs') const os = require('os') const fse = require('../../..') -const ncp = require('../../copy') +const { copy: ncp } = require('../../') const path = require('path') const assert = require('assert') From d9f8ef94cdf1bc5bc4e6a74f12b11b61d99bcfdd Mon Sep 17 00:00:00 2001 From: SukkaW Date: Fri, 20 Oct 2023 10:43:30 +0800 Subject: [PATCH 05/15] Use promisfied fs API from `lib/fs` --- lib/util/__tests__/utimes.test.js | 2 +- lib/util/utimes.js | 13 ++++--------- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/lib/util/__tests__/utimes.test.js b/lib/util/__tests__/utimes.test.js index d28ece197..e5aceb4e9 100644 --- a/lib/util/__tests__/utimes.test.js +++ b/lib/util/__tests__/utimes.test.js @@ -33,7 +33,7 @@ describe('utimes', () => { fse.emptyDir(TEST_DIR, done) // reset stubs gracefulFsStub = {} - utimes = proxyquire('../utimes', { 'graceful-fs': gracefulFsStub }) + utimes = proxyquire('../utimes', { '../fs': gracefulFsStub }) }) describe('utimesMillis()', () => { diff --git a/lib/util/utimes.js b/lib/util/utimes.js index 2cea485d8..a6b5c7bf7 100644 --- a/lib/util/utimes.js +++ b/lib/util/utimes.js @@ -1,11 +1,6 @@ 'use strict' -const fs = require('graceful-fs') -const { promisify } = require('util') - -const open = promisify(fs.open) -const futimes = promisify(fs.futimes) -const close = promisify(fs.close) +const fs = require('../fs') // TODO: remove `utimesMillis` once all internal usage has switched to the Promise-based `utimesMillisAsync` API function utimesMillis (path, atime, mtime, callback) { @@ -22,11 +17,11 @@ function utimesMillis (path, atime, mtime, callback) { async function utimesMillisAsync (path, atime, mtime) { // if (!HAS_MILLIS_RES) return fs.utimes(path, atime, mtime, callback) - const fd = await open(path, 'r+') + const fd = await fs.open(path, 'r+') let futimesErr = null try { - await futimes(fd, atime, mtime) + await fs.futimes(fd, atime, mtime) } catch (e) { futimesErr = e } @@ -34,7 +29,7 @@ async function utimesMillisAsync (path, atime, mtime) { let closeErr = null try { - await close(fd) + await fs.close(fd) } catch (e) { closeErr = e } From bc8ee0bf340f04fad0fed408bbdd0cc3747e9d0c Mon Sep 17 00:00:00 2001 From: SukkaW Date: Sat, 21 Oct 2023 23:48:49 +0800 Subject: [PATCH 06/15] 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 3f0967593..621efd5ce 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) } From cf3aed7a1d413e329fa7edede323f2479f3901a0 Mon Sep 17 00:00:00 2001 From: SukkaW Date: Sun, 22 Oct 2023 00:10:32 +0800 Subject: [PATCH 07/15] Refactor copy recursive logic --- lib/copy/copy.js | 71 +++++++++++++++++++++--------------------------- 1 file changed, 31 insertions(+), 40 deletions(-) diff --git a/lib/copy/copy.js b/lib/copy/copy.js index 621efd5ce..ecd21bd0f 100644 --- a/lib/copy/copy.js +++ b/lib/copy/copy.js @@ -36,19 +36,14 @@ async function copy (src, dest, opts = {}) { if (!include) return - return checkParentDir(destStat, src, dest, opts) -} - -async function checkParentDir (destStat, src, dest, opts) { + // check if the parent of dest exists, and create it if it doesn't exist const destParent = path.dirname(dest) - const dirExists = await pathExists(destParent) - if (!dirExists) { await mkdirs(destParent) } - return getStats(destStat, src, dest, opts) + await getStatsAndPerformCopy(destStat, src, dest, opts) } async function runFilter (src, dest, opts) { @@ -56,7 +51,7 @@ async function runFilter (src, dest, opts) { return opts.filter(src, dest) } -async function getStats (destStat, src, dest, opts) { +async function getStatsAndPerformCopy (destStat, src, dest, opts) { const statFn = opts.dereference ? fs.stat : fs.lstat const srcStat = await statFn(src) @@ -101,19 +96,10 @@ async function handleTimestampsAndMode (srcMode, src, dest) { if (fileIsNotWritable(srcMode)) { await makeFileWritable(dest, srcMode) } - return setDestTimestampsAndMode(srcMode, src, dest) -} -function fileIsNotWritable (srcMode) { - return (srcMode & 0o200) === 0 -} + // Set timestamps and mode correspondingly -function makeFileWritable (dest, srcMode) { - return setDestMode(dest, srcMode | 0o200) -} - -async function setDestTimestampsAndMode (srcMode, src, dest) { - // The initial srcStat.atime cannot be trusted + // Note that 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 = await fs.stat(src) @@ -122,43 +108,47 @@ async function setDestTimestampsAndMode (srcMode, src, dest) { return setDestMode(dest, srcMode) } +function fileIsNotWritable (srcMode) { + return (srcMode & 0o200) === 0 +} + +function makeFileWritable (dest, srcMode) { + return setDestMode(dest, srcMode | 0o200) +} + function setDestMode (dest, srcMode) { return fs.chmod(dest, srcMode) } async function onDir (srcStat, destStat, src, dest, opts) { + // the dest direcotry might not exist, create it if (!destStat) { await fs.mkdir(dest) } - await copyDir(src, dest, opts) - if (!destStat) { - await setDestMode(dest, srcStat.mode) - } -} -async function copyDir (src, dest, opts) { const items = await fs.readdir(src) - return 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) -} + // loop through the files in the current directory to copy everything + for (let i = 0, len = items.length; i < len; i++) { + const item = items[i] -async function copyDirItem (items, item, src, dest, opts) { - const srcItem = path.join(src, item) - const destItem = path.join(dest, item) + const srcItem = path.join(src, item) + const destItem = path.join(dest, item) - const include = await runFilter(srcItem, destItem, opts) - if (!include) return copyDirItems(items, src, dest, opts) + // skip the item if it is matches by the filter function + const include = await runFilter(srcItem, destItem, opts) + if (!include) continue - const { destStat } = await stat.checkPathsAsync(srcItem, destItem, 'copy', opts) + const { destStat } = await stat.checkPathsAsync(srcItem, destItem, 'copy', opts) - await getStats(destStat, srcItem, destItem, opts) + // If the item is a copyable file, `getStatsAndPerformCopy` will copy it + // If the item is a directory, `getStatsAndPerformCopy` will call `onDir` recursively + await getStatsAndPerformCopy(destStat, srcItem, destItem, opts) + } - return copyDirItems(items, src, dest, opts) + if (!destStat) { + await setDestMode(dest, srcStat.mode) + } } async function onLink (destStat, src, dest, opts) { @@ -194,6 +184,7 @@ async function onLink (destStat, src, dest, opts) { throw new Error(`Cannot overwrite '${resolvedDest}' with '${resolvedSrc}'.`) } + // copy the link await fs.unlink(dest) return fs.symlink(resolvedSrc, dest) } From a3e2c213587b7456f861dafd7a890093ae3d0d7e Mon Sep 17 00:00:00 2001 From: SukkaW Date: Sun, 22 Oct 2023 00:22:15 +0800 Subject: [PATCH 08/15] Universalify stat utility --- lib/copy/copy.js | 10 ++++---- lib/util/stat.js | 63 +++++----------------------------------------- lib/util/utimes.js | 19 +++----------- 3 files changed, 14 insertions(+), 78 deletions(-) diff --git a/lib/copy/copy.js b/lib/copy/copy.js index ecd21bd0f..512b14bc3 100644 --- a/lib/copy/copy.js +++ b/lib/copy/copy.js @@ -4,7 +4,7 @@ const fs = require('../fs') const path = require('path') const { mkdirs } = require('../mkdirs') const { pathExists } = require('../path-exists') -const { utimesMillisAsync } = require('../util/utimes') +const { utimesMillis } = require('../util/utimes') const stat = require('../util/stat') async function copy (src, dest, opts = {}) { @@ -28,9 +28,9 @@ async function copy (src, dest, opts = {}) { ) } - const { srcStat, destStat } = await stat.checkPathsAsync(src, dest, 'copy', opts) + const { srcStat, destStat } = await stat.checkPaths(src, dest, 'copy', opts) - await stat.checkParentPathsAsync(src, srcStat, dest, 'copy') + await stat.checkParentPaths(src, srcStat, dest, 'copy') const include = await runFilter(src, dest, opts) @@ -103,7 +103,7 @@ async function handleTimestampsAndMode (srcMode, src, dest) { // because it is modified by the read(2) system call // (See https://nodejs.org/api/fs.html#fs_stat_time_values) const updatedSrcStat = await fs.stat(src) - await utimesMillisAsync(dest, updatedSrcStat.atime, updatedSrcStat.mtime) + await utimesMillis(dest, updatedSrcStat.atime, updatedSrcStat.mtime) return setDestMode(dest, srcMode) } @@ -139,7 +139,7 @@ async function onDir (srcStat, destStat, src, dest, opts) { const include = await runFilter(srcItem, destItem, opts) if (!include) continue - const { destStat } = await stat.checkPathsAsync(srcItem, destItem, 'copy', opts) + const { destStat } = await stat.checkPaths(srcItem, destItem, 'copy', opts) // If the item is a copyable file, `getStatsAndPerformCopy` will copy it // If the item is a directory, `getStatsAndPerformCopy` will call `onDir` recursively diff --git a/lib/util/stat.js b/lib/util/stat.js index e375bfff5..dfd37d9c7 100644 --- a/lib/util/stat.js +++ b/lib/util/stat.js @@ -2,7 +2,7 @@ const fs = require('../fs') const path = require('path') -const util = require('util') +const u = require('universalify').fromPromise function getStats (src, dest, opts) { const statFunc = opts.dereference @@ -32,39 +32,7 @@ function getStatsSync (src, dest, opts) { return { srcStat, destStat } } -// TODO: remove callback-based internal utility `checkPaths` once all API has migrated to Promise-based `checkPathsAsync` -function checkPaths (src, dest, funcName, opts, cb) { - util.callbackify(getStats)(src, dest, opts, (err, stats) => { - if (err) return cb(err) - const { srcStat, destStat } = stats - - if (destStat) { - if (areIdentical(srcStat, destStat)) { - const srcBaseName = path.basename(src) - const destBaseName = path.basename(dest) - if (funcName === 'move' && - srcBaseName !== destBaseName && - srcBaseName.toLowerCase() === destBaseName.toLowerCase()) { - return cb(null, { srcStat, destStat, isChangingCase: true }) - } - return cb(new Error('Source and destination must not be the same.')) - } - if (srcStat.isDirectory() && !destStat.isDirectory()) { - return cb(new Error(`Cannot overwrite non-directory '${dest}' with directory '${src}'.`)) - } - if (!srcStat.isDirectory() && destStat.isDirectory()) { - return cb(new Error(`Cannot overwrite directory '${dest}' with non-directory '${src}'.`)) - } - } - - if (srcStat.isDirectory() && isSrcSubdir(src, dest)) { - return cb(new Error(errMsg(src, dest, funcName))) - } - return cb(null, { srcStat, destStat }) - }) -} - -async function checkPathsAsync (src, dest, funcName, opts) { +async function checkPaths (src, dest, funcName, opts) { const { srcStat, destStat } = await getStats(src, dest, opts) if (destStat) { if (areIdentical(srcStat, destStat)) { @@ -124,24 +92,7 @@ function checkPathsSync (src, dest, funcName, opts) { // It works for all file types including symlinks since it // checks the src and dest inodes. It starts from the deepest // parent and stops once it reaches the src parent or the root path. -// TODO: remove callback-based internal utility `checkPaths` once all API has migrated to Promise-based `checkPathsAsync` -function checkParentPaths (src, srcStat, dest, funcName, cb) { - const srcParent = path.resolve(path.dirname(src)) - const destParent = path.resolve(path.dirname(dest)) - if (destParent === srcParent || destParent === path.parse(destParent).root) return cb() - fs.stat(destParent, { bigint: true }, (err, destStat) => { - if (err) { - if (err.code === 'ENOENT') return cb() - return cb(err) - } - if (areIdentical(srcStat, destStat)) { - return cb(new Error(errMsg(src, dest, funcName))) - } - return checkParentPaths(src, srcStat, destParent, funcName, cb) - }) -} - -async function checkParentPathsAsync (src, srcStat, dest, funcName) { +async function checkParentPaths (src, srcStat, dest, funcName) { const srcParent = path.resolve(path.dirname(src)) const destParent = path.resolve(path.dirname(dest)) if (destParent === srcParent || destParent === path.parse(destParent).root) return @@ -158,7 +109,7 @@ async function checkParentPathsAsync (src, srcStat, dest, funcName) { throw new Error(errMsg(src, dest, funcName)) } - return checkParentPathsAsync(src, srcStat, destParent, funcName) + return checkParentPaths(src, srcStat, destParent, funcName) } function checkParentPathsSync (src, srcStat, dest, funcName) { @@ -196,13 +147,11 @@ function errMsg (src, dest, funcName) { module.exports = { // checkPaths - checkPaths, + checkPaths: u(checkPaths), checkPathsSync, - checkPathsAsync, // checkParent - checkParentPaths, + checkParentPaths: u(checkParentPaths), checkParentPathsSync, - checkParentPathsAsync, // Misc isSrcSubdir, areIdentical diff --git a/lib/util/utimes.js b/lib/util/utimes.js index a6b5c7bf7..8f97263f3 100644 --- a/lib/util/utimes.js +++ b/lib/util/utimes.js @@ -1,21 +1,9 @@ 'use strict' const fs = require('../fs') +const u = require('universalify').fromPromise -// TODO: remove `utimesMillis` once all internal usage has switched to the Promise-based `utimesMillisAsync` API -function utimesMillis (path, atime, mtime, callback) { - // if (!HAS_MILLIS_RES) return fs.utimes(path, atime, mtime, callback) - fs.open(path, 'r+', (err, fd) => { - if (err) return callback(err) - fs.futimes(fd, atime, mtime, futimesErr => { - fs.close(fd, closeErr => { - if (callback) callback(futimesErr || closeErr) - }) - }) - }) -} - -async function utimesMillisAsync (path, atime, mtime) { +async function utimesMillis (path, atime, mtime) { // if (!HAS_MILLIS_RES) return fs.utimes(path, atime, mtime, callback) const fd = await fs.open(path, 'r+') @@ -49,7 +37,6 @@ function utimesMillisSync (path, atime, mtime) { } module.exports = { - utimesMillis, - utimesMillisAsync, + utimesMillis: u(utimesMillis), utimesMillisSync } From dbfdb3182faafc42ea4b7763d5e6bda3ef93d252 Mon Sep 17 00:00:00 2001 From: SukkaW Date: Sun, 22 Oct 2023 00:31:25 +0800 Subject: [PATCH 09/15] Universalify the `gracefulFsStub` inside the unit test --- lib/util/__tests__/utimes.test.js | 14 ++++++++------ lib/util/utimes.js | 2 +- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/lib/util/__tests__/utimes.test.js b/lib/util/__tests__/utimes.test.js index e5aceb4e9..02c65d1fa 100644 --- a/lib/util/__tests__/utimes.test.js +++ b/lib/util/__tests__/utimes.test.js @@ -6,6 +6,8 @@ const fse = require('../..') const path = require('path') const assert = require('assert') const proxyquire = require('proxyquire') +const u = require('universalify').fromCallback + let gracefulFsStub let utimes @@ -68,25 +70,25 @@ describe('utimes', () => { it('should close open file desciptors after encountering an error', done => { const fakeFd = Math.random() - gracefulFsStub.open = (pathIgnored, flagsIgnored, modeIgnored, callback) => { + gracefulFsStub.open = u((pathIgnored, flagsIgnored, modeIgnored, callback) => { if (typeof modeIgnored === 'function') callback = modeIgnored process.nextTick(() => callback(null, fakeFd)) - } + }) let closeCalled = false - gracefulFsStub.close = (fd, callback) => { + gracefulFsStub.close = u((fd, callback) => { assert.strictEqual(fd, fakeFd) closeCalled = true if (callback) process.nextTick(callback) - } + }) let testError - gracefulFsStub.futimes = (fd, atimeIgnored, mtimeIgnored, callback) => { + gracefulFsStub.futimes = u((fd, atimeIgnored, mtimeIgnored, callback) => { process.nextTick(() => { testError = new Error('A test error') callback(testError) }) - } + }) utimes.utimesMillis('ignored', 'ignored', 'ignored', err => { assert.strictEqual(err, testError) diff --git a/lib/util/utimes.js b/lib/util/utimes.js index 8f97263f3..89a2f1e03 100644 --- a/lib/util/utimes.js +++ b/lib/util/utimes.js @@ -3,7 +3,7 @@ const fs = require('../fs') const u = require('universalify').fromPromise -async function utimesMillis (path, atime, mtime) { +async function utimesMillis (path, atime, mtime, cb) { // if (!HAS_MILLIS_RES) return fs.utimes(path, atime, mtime, callback) const fd = await fs.open(path, 'r+') From b27cdfa5fac5871259df94d7c7fdd9792d7b5223 Mon Sep 17 00:00:00 2001 From: SukkaW Date: Sun, 22 Oct 2023 01:15:38 +0800 Subject: [PATCH 10/15] Inline preserve timestamp function --- lib/copy/copy.js | 53 ++++++++++++++++++++++-------------------------- 1 file changed, 24 insertions(+), 29 deletions(-) diff --git a/lib/copy/copy.js b/lib/copy/copy.js index 512b14bc3..aa6c576d8 100644 --- a/lib/copy/copy.js +++ b/lib/copy/copy.js @@ -7,7 +7,7 @@ const { pathExists } = require('../path-exists') const { utimesMillis } = 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 } } @@ -46,12 +46,12 @@ async function copy (src, dest, opts = {}) { await getStatsAndPerformCopy(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 getStatsAndPerformCopy (destStat, src, dest, opts) { +async function getStatsAndPerformCopy(destStat, src, dest, opts) { const statFn = opts.dereference ? fs.stat : fs.lstat const srcStat = await statFn(src) @@ -69,7 +69,7 @@ async function getStatsAndPerformCopy (destStat, src, dest, opts) { throw new Error(`Unknown file: ${src}`) } -async function onFile (srcStat, destStat, src, dest, opts) { +async function onFile(srcStat, destStat, src, dest, opts) { if (!destStat) return copyFile(srcStat, src, dest, opts) if (opts.overwrite) { @@ -81,46 +81,41 @@ async function onFile (srcStat, destStat, 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) -} + // 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, srcStat.mode) + } -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) - } + // Set timestamps and mode correspondingly - // Set timestamps and mode correspondingly - - // Note that 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 = await fs.stat(src) - await utimesMillis(dest, updatedSrcStat.atime, updatedSrcStat.mtime) + // Note that 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 = await fs.stat(src) + await utimesMillis(dest, updatedSrcStat.atime, updatedSrcStat.mtime) + } - return setDestMode(dest, srcMode) + return setDestMode(dest, srcStat.mode) } -function fileIsNotWritable (srcMode) { +function fileIsNotWritable(srcMode) { return (srcMode & 0o200) === 0 } -function makeFileWritable (dest, srcMode) { +function makeFileWritable(dest, srcMode) { return setDestMode(dest, srcMode | 0o200) } -function setDestMode (dest, srcMode) { +function setDestMode(dest, srcMode) { return fs.chmod(dest, srcMode) } -async function onDir (srcStat, destStat, src, dest, opts) { +async function onDir(srcStat, destStat, src, dest, opts) { // the dest direcotry might not exist, create it if (!destStat) { await fs.mkdir(dest) @@ -151,7 +146,7 @@ async function onDir (srcStat, destStat, 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) From 20ac969302630a31e7674f9529ab00deaf34c910 Mon Sep 17 00:00:00 2001 From: SukkaW Date: Sun, 22 Oct 2023 01:50:07 +0800 Subject: [PATCH 11/15] Make `standard` happy --- lib/copy/copy.js | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/copy/copy.js b/lib/copy/copy.js index aa6c576d8..bf9f12617 100644 --- a/lib/copy/copy.js +++ b/lib/copy/copy.js @@ -7,7 +7,7 @@ const { pathExists } = require('../path-exists') const { utimesMillis } = 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 } } @@ -46,12 +46,12 @@ async function copy(src, dest, opts = {}) { await getStatsAndPerformCopy(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 getStatsAndPerformCopy(destStat, src, dest, opts) { +async function getStatsAndPerformCopy (destStat, src, dest, opts) { const statFn = opts.dereference ? fs.stat : fs.lstat const srcStat = await statFn(src) @@ -69,7 +69,7 @@ async function getStatsAndPerformCopy(destStat, src, dest, opts) { throw new Error(`Unknown file: ${src}`) } -async function onFile(srcStat, destStat, src, dest, opts) { +async function onFile (srcStat, destStat, src, dest, opts) { if (!destStat) return copyFile(srcStat, src, dest, opts) if (opts.overwrite) { @@ -81,13 +81,13 @@ async function onFile(srcStat, destStat, 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) { // 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)) { + if (fileIsNotWritable(srcStat.mode)) { await makeFileWritable(dest, srcStat.mode) } @@ -103,19 +103,19 @@ async function copyFile(srcStat, src, dest, opts) { return setDestMode(dest, srcStat.mode) } -function fileIsNotWritable(srcMode) { +function fileIsNotWritable (srcMode) { return (srcMode & 0o200) === 0 } -function makeFileWritable(dest, srcMode) { +function makeFileWritable (dest, srcMode) { return setDestMode(dest, srcMode | 0o200) } -function setDestMode(dest, srcMode) { +function setDestMode (dest, srcMode) { return fs.chmod(dest, srcMode) } -async function onDir(srcStat, destStat, src, dest, opts) { +async function onDir (srcStat, destStat, src, dest, opts) { // the dest direcotry might not exist, create it if (!destStat) { await fs.mkdir(dest) @@ -146,7 +146,7 @@ async function onDir(srcStat, destStat, 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) From eebb61d61eecacb94fe21c6d157f0975cda6658a Mon Sep 17 00:00:00 2001 From: SukkaW Date: Sun, 22 Oct 2023 01:53:27 +0800 Subject: [PATCH 12/15] Inline `setDestMode` --- lib/copy/copy.js | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/lib/copy/copy.js b/lib/copy/copy.js index bf9f12617..e71a1ce38 100644 --- a/lib/copy/copy.js +++ b/lib/copy/copy.js @@ -100,7 +100,7 @@ async function copyFile (srcStat, src, dest, opts) { await utimesMillis(dest, updatedSrcStat.atime, updatedSrcStat.mtime) } - return setDestMode(dest, srcStat.mode) + return fs.chmod(dest, srcStat.mode) } function fileIsNotWritable (srcMode) { @@ -108,11 +108,7 @@ function fileIsNotWritable (srcMode) { } function makeFileWritable (dest, srcMode) { - return setDestMode(dest, srcMode | 0o200) -} - -function setDestMode (dest, srcMode) { - return fs.chmod(dest, srcMode) + return fs.chmod(dest, srcMode | 0o200) } async function onDir (srcStat, destStat, src, dest, opts) { @@ -142,7 +138,7 @@ async function onDir (srcStat, destStat, src, dest, opts) { } if (!destStat) { - await setDestMode(dest, srcStat.mode) + await fs.chmod(dest, srcStat.mode) } } From ce7cf649c8b67bdd268a2b07d2101a1c4e3fee3f Mon Sep 17 00:00:00 2001 From: SukkaW Date: Sun, 22 Oct 2023 01:54:16 +0800 Subject: [PATCH 13/15] Remove duplicated `opts` handling --- lib/copy/copy.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/lib/copy/copy.js b/lib/copy/copy.js index e71a1ce38..b9223e340 100644 --- a/lib/copy/copy.js +++ b/lib/copy/copy.js @@ -12,10 +12,6 @@ async function copy (src, dest, opts = {}) { opts = { filter: 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 From 1ce5d348c426ea20c5006389477e163211898b22 Mon Sep 17 00:00:00 2001 From: SukkaW Date: Tue, 24 Oct 2023 14:01:54 +0800 Subject: [PATCH 14/15] Apply code review suggestions from @RyanZim --- lib/copy/copy.js | 7 ++----- lib/util/utimes.js | 22 ++++++++-------------- 2 files changed, 10 insertions(+), 19 deletions(-) diff --git a/lib/copy/copy.js b/lib/copy/copy.js index b9223e340..86556e416 100644 --- a/lib/copy/copy.js +++ b/lib/copy/copy.js @@ -108,16 +108,13 @@ function makeFileWritable (dest, srcMode) { } async function onDir (srcStat, destStat, src, dest, opts) { - // the dest direcotry might not exist, create it + // the dest directory might not exist, create it if (!destStat) { await fs.mkdir(dest) } - const items = await fs.readdir(src) - // loop through the files in the current directory to copy everything - for (let i = 0, len = items.length; i < len; i++) { - const item = items[i] + for (const item of await fs.readdir(src)) { const srcItem = path.join(src, item) const destItem = path.join(dest, item) diff --git a/lib/util/utimes.js b/lib/util/utimes.js index 89a2f1e03..87f4588c1 100644 --- a/lib/util/utimes.js +++ b/lib/util/utimes.js @@ -3,28 +3,22 @@ const fs = require('../fs') const u = require('universalify').fromPromise -async function utimesMillis (path, atime, mtime, cb) { +async function utimesMillis (path, atime, mtime) { // if (!HAS_MILLIS_RES) return fs.utimes(path, atime, mtime, callback) const fd = await fs.open(path, 'r+') - let futimesErr = null - try { - await fs.futimes(fd, atime, mtime) - } catch (e) { - futimesErr = e - } - let closeErr = null try { - await fs.close(fd) - } catch (e) { - closeErr = e + await fs.futimes(fd, atime, mtime) + } finally { + try { + await fs.close(fd) + } catch (e) { + closeErr = e + } } - if (futimesErr) { - throw futimesErr - } if (closeErr) { throw closeErr } From c2d187109bb278284b32abe2bca1632cc91faf02 Mon Sep 17 00:00:00 2001 From: SukkaW Date: Tue, 24 Oct 2023 14:05:08 +0800 Subject: [PATCH 15/15] Make `standard` happy --- lib/copy/copy.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/copy/copy.js b/lib/copy/copy.js index 86556e416..565602105 100644 --- a/lib/copy/copy.js +++ b/lib/copy/copy.js @@ -115,7 +115,6 @@ async function onDir (srcStat, destStat, src, dest, opts) { // loop through the files in the current directory to copy everything for (const item of await fs.readdir(src)) { - const srcItem = path.join(src, item) const destItem = path.join(dest, item)