Merge 172538a into fb3dda7

jprichardson · Aug 5, 2018 · 76240fc · 76240fc
2 parents fb3dda7 + 172538a
commit 76240fc
Show file tree

Hide file tree

Showing 4 changed files with 49 additions and 167 deletions.
diff --git a/lib/move-sync/__tests__/move-sync-prevent-moving-into-itself.test.js b/lib/move-sync/__tests__/move-sync-prevent-moving-into-itself.test.js
@@ -182,7 +182,7 @@ function testError (src, dest) {
   try {
     fs.moveSync(src, dest)
   } catch (err) {
-    assert.strictEqual(err.message, `Cannot move '${src}' into itself '${dest}'.`)
+    assert.strictEqual(err.message, `Cannot move '${src}' to a subdirectory of itself, '${dest}'.`)
     assert(fs.existsSync(src))
     assert(!fs.existsSync(dest))
   }

diff --git a/lib/move-sync/__tests__/move-sync.test.js b/lib/move-sync/__tests__/move-sync.test.js
@@ -5,7 +5,6 @@ const os = require('os')
 const fse = require(process.cwd())
 const path = require('path')
 const assert = require('assert')
-const rimraf = require('rimraf')
 
 /* global afterEach, beforeEach, describe, it */
 
@@ -19,35 +18,29 @@ function createSyncErrFn (errCode) {
 }
 
 const originalRenameSync = fs.renameSync
-const originalLinkSync = fs.linkSync
 
 function setUpMockFs (errCode) {
   fs.renameSync = createSyncErrFn(errCode)
-  fs.linkSync = createSyncErrFn(errCode)
 }
 
 function tearDownMockFs () {
   fs.renameSync = originalRenameSync
-  fs.linkSync = originalLinkSync
 }
 
 describe('moveSync()', () => {
   let TEST_DIR
 
   beforeEach(() => {
     TEST_DIR = path.join(os.tmpdir(), 'fs-extra', 'move-sync')
-
     fse.emptyDirSync(TEST_DIR)
 
-    // Create fixtures:
-    fs.writeFileSync(path.join(TEST_DIR, 'a-file'), 'sonic the hedgehog\n')
-    fs.mkdirSync(path.join(TEST_DIR, 'a-folder'))
-    fs.writeFileSync(path.join(TEST_DIR, 'a-folder/another-file'), 'tails\n')
-    fs.mkdirSync(path.join(TEST_DIR, 'a-folder/another-folder'))
-    fs.writeFileSync(path.join(TEST_DIR, 'a-folder/another-folder/file3'), 'knuckles\n')
+    // Create fixtures
+    fse.outputFileSync(path.join(TEST_DIR, 'a-file'), 'sonic the hedgehog\n')
+    fse.outputFileSync(path.join(TEST_DIR, 'a-folder/another-file'), 'tails\n')
+    fse.outputFileSync(path.join(TEST_DIR, 'a-folder/another-folder/file3'), 'knuckles\n')
   })
 
-  afterEach(done => rimraf(TEST_DIR, done))
+  afterEach(() => fse.removeSync(TEST_DIR))
 
   it('should not move if src and dest are the same', () => {
     const src = `${TEST_DIR}/a-file`
@@ -58,13 +51,12 @@ describe('moveSync()', () => {
     // assert src not affected
     const contents = fs.readFileSync(src, 'utf8')
     const expected = /^sonic the hedgehog\r?\n$/
-    assert.ok(contents.match(expected), `${contents} match ${expected}`)
+    assert(contents.match(expected))
   })
 
   it('should error if src and dest are the same and src does not exist', () => {
     const src = `${TEST_DIR}/non-existent`
     const dest = src
-
     assert.throws(() => fse.moveSync(src, dest))
   })
 
@@ -76,7 +68,7 @@ describe('moveSync()', () => {
 
     const contents = fs.readFileSync(dest, 'utf8')
     const expected = /^sonic the hedgehog\r?\n$/
-    assert.ok(contents.match(expected), `${contents} match ${expected}`)
+    assert(contents.match(expected))
   })
 
   it('should not overwrite the destination by default', () => {
@@ -89,7 +81,7 @@ describe('moveSync()', () => {
     try {
       fse.moveSync(src, dest)
     } catch (err) {
-      assert.ok(err && err.code === 'EEXIST', 'throw EEXIST')
+      assert.strictEqual(err.message, 'dest already exists.')
     }
   })
 
@@ -103,7 +95,7 @@ describe('moveSync()', () => {
     try {
       fse.moveSync(src, dest, {overwrite: false})
     } catch (err) {
-      assert.ok(err && err.code === 'EEXIST', 'throw EEXIST')
+      assert.strictEqual(err.message, 'dest already exists.')
     }
   })
 
@@ -121,7 +113,7 @@ describe('moveSync()', () => {
     assert.ok(contents.match(expected), `${contents} match ${expected}`)
   })
 
-  it('should overwrite the destination directory if overwrite = true', done => {
+  it('should overwrite the destination directory if overwrite = true', () => {
     // Create src
     const src = path.join(TEST_DIR, 'src')
     fse.ensureDirSync(src)
@@ -145,7 +137,6 @@ describe('moveSync()', () => {
     // verify dest has new stuff
     assert(pathsAfter.indexOf('some-file') >= 0)
     assert(pathsAfter.indexOf('some-folder') >= 0)
-    done()
   })
 
   it('should create directory structure by default', () => {
@@ -159,7 +150,7 @@ describe('moveSync()', () => {
 
     const contents = fs.readFileSync(dest, 'utf8')
     const expected = /^sonic the hedgehog\r?\n$/
-    assert.ok(contents.match(expected), `${contents} match ${expected}`)
+    assert(contents.match(expected))
   })
 
   it('should work across devices', () => {
@@ -172,7 +163,7 @@ describe('moveSync()', () => {
 
     const contents = fs.readFileSync(dest, 'utf8')
     const expected = /^sonic the hedgehog\r?\n$/
-    assert.ok(contents.match(expected), `${contents} match ${expected}`)
+    assert(contents.match(expected))
     tearDownMockFs()
   })
 
@@ -187,27 +178,12 @@ describe('moveSync()', () => {
 
     const contents = fs.readFileSync(dest + '/another-file', 'utf8')
     const expected = /^tails\r?\n$/
-    assert.ok(contents.match(expected), `${contents} match ${expected}`)
-  })
-
-  it('should move folders across devices with EISDIR error', () => {
-    const src = `${TEST_DIR}/a-folder`
-    const dest = `${TEST_DIR}/a-folder-dest`
-
-    setUpMockFs('EISDIR')
-
-    fse.moveSync(src, dest)
-
-    const contents = fs.readFileSync(dest + '/another-folder/file3', 'utf8')
-    const expected = /^knuckles\r?\n$/
-    assert.ok(contents.match(expected), `${contents} match ${expected}`)
-    tearDownMockFs()
+    assert(contents.match(expected))
   })
 
   it('should overwrite folders across devices', () => {
     const src = `${TEST_DIR}/a-folder`
     const dest = `${TEST_DIR}/a-folder-dest`
-
     fs.mkdirSync(dest)
 
     setUpMockFs('EXDEV')
@@ -216,7 +192,7 @@ describe('moveSync()', () => {
 
     const contents = fs.readFileSync(dest + '/another-folder/file3', 'utf8')
     const expected = /^knuckles\r?\n$/
-    assert.ok(contents.match(expected), `${contents} match ${expected}`)
+    assert(contents.match(expected))
     tearDownMockFs()
   })
 
@@ -230,35 +206,7 @@ describe('moveSync()', () => {
 
     const contents = fs.readFileSync(dest + '/another-folder/file3', 'utf8')
     const expected = /^knuckles\r?\n$/
-    assert.ok(contents.match(expected), `${contents} match ${expected}`)
-    tearDownMockFs()
-  })
-
-  it('should move folders across devices with EPERM error', () => {
-    const src = `${TEST_DIR}/a-folder`
-    const dest = `${TEST_DIR}/a-folder-dest`
-
-    setUpMockFs('EPERM')
-
-    fse.moveSync(src, dest)
-
-    const contents = fs.readFileSync(dest + '/another-folder/file3', 'utf8')
-    const expected = /^knuckles\r?\n$/
-    assert.ok(contents.match(expected), `${contents} match ${expected}`)
-    tearDownMockFs()
-  })
-
-  it('should move folders across devices with ENOTSUP error', () => {
-    const src = `${TEST_DIR}/a-folder`
-    const dest = `${TEST_DIR}/a-folder-dest`
-
-    setUpMockFs('ENOTSUP')
-
-    fse.moveSync(src, dest)
-
-    const contents = fs.readFileSync(dest + '/another-folder/file3', 'utf8')
-    const expected = /^knuckles\r?\n$/
-    assert.ok(contents.match(expected), `${contents} match ${expected}`)
+    assert(contents.match(expected))
     tearDownMockFs()
   })
 
@@ -274,7 +222,7 @@ describe('moveSync()', () => {
 
       const contents = fs.readFileSync(dest, 'utf8')
       const expected = /^sonic the hedgehog\r?\n$/
-      assert.ok(contents.match(expected), `${contents} match ${expected}`)
+      assert(contents.match(expected))
     })
   })
 
@@ -306,7 +254,7 @@ describe('moveSync()', () => {
 
       const contents = fs.readFileSync(dest, 'utf8')
       const expected = /^sonic the hedgehog\r?\n$/
-      assert.ok(contents.match(expected), `${contents} match ${expected}`)
+      assert(contents.match(expected))
     })
   })
 
@@ -324,7 +272,7 @@ describe('moveSync()', () => {
     try {
       fs.writeFileSync(path.join(differentDevice, 'file'), 'hi')
     } catch (err) {
-      console.log("Can't write to device. Skipping moveSync test.")
+      console.log(`Can't write to device. Skipping moveSync test.`)
       __skipTests = true
     }
 
@@ -335,12 +283,8 @@ describe('moveSync()', () => {
         const src = '/mnt/some/weird/dir-really-weird'
         const dest = path.join(TEST_DIR, 'device-weird')
 
-        if (!fs.existsSync(src)) {
-          fse.mkdirpSync(src)
-        }
-
+        if (!fs.existsSync(src)) fse.mkdirpSync(src)
         assert(!fs.existsSync(dest))
-
         assert(fs.lstatSync(src).isDirectory())
 
         fse.moveSync(src, dest)

diff --git a/lib/move-sync/index.js b/lib/move-sync/index.js
@@ -4,114 +4,53 @@ const fs = require('graceful-fs')
 const path = require('path')
 const copySync = require('../copy-sync').copySync
 const removeSync = require('../remove').removeSync
-const mkdirpSync = require('../mkdirs').mkdirsSync
-const buffer = require('../util/buffer')
-
-function moveSync (src, dest, options) {
-  options = options || {}
-  const overwrite = options.overwrite || options.clobber || false
+const mkdirpSync = require('../mkdirs').mkdirpSync
 
+function moveSync (src, dest, opts) {
+  opts = opts || {}
+  const overwrite = opts.overwrite || opts.clobber || false
   src = path.resolve(src)
   dest = path.resolve(dest)
-
   if (src === dest) return fs.accessSync(src)
 
-  if (isSrcSubdir(src, dest)) throw new Error(`Cannot move '${src}' into itself '${dest}'.`)
-
+  const st = fs.statSync(src)
+  if (st.isDirectory() && isSrcSubdir(src, dest)) throw new Error(`Cannot move '${src}' to a subdirectory of itself, '${dest}'.`)
   mkdirpSync(path.dirname(dest))
-  tryRenameSync()
-
-  function tryRenameSync () {
-    if (overwrite) {
-      try {
-        return fs.renameSync(src, dest)
-      } catch (err) {
-        if (err.code === 'ENOTEMPTY' || err.code === 'EEXIST' || err.code === 'EPERM') {
-          removeSync(dest)
-          options.overwrite = false // just overwriteed it, no need to do it again
-          return moveSync(src, dest, options)
-        }
-
-        if (err.code !== 'EXDEV') throw err
-        return moveSyncAcrossDevice(src, dest, overwrite)
-      }
-    } else {
-      try {
-        fs.linkSync(src, dest)
-        return fs.unlinkSync(src)
-      } catch (err) {
-        if (err.code === 'EXDEV' || err.code === 'EISDIR' || err.code === 'EPERM' || err.code === 'ENOTSUP') {
-          return moveSyncAcrossDevice(src, dest, overwrite)
-        }
-        throw err
-      }
-    }
-  }
+  return doRename(src, dest, overwrite)
 }
 
-function moveSyncAcrossDevice (src, dest, overwrite) {
-  const stat = fs.statSync(src)
-
-  if (stat.isDirectory()) {
-    return moveDirSyncAcrossDevice(src, dest, overwrite)
-  } else {
-    return moveFileSyncAcrossDevice(src, dest, overwrite)
+function doRename (src, dest, overwrite) {
+  if (overwrite) {
+    removeSync(dest)
+    return rename(src, dest, overwrite)
   }
+  if (fs.existsSync(dest)) throw new Error('dest already exists.')
+  return rename(src, dest, overwrite)
 }
 
-function moveFileSyncAcrossDevice (src, dest, overwrite) {
-  const BUF_LENGTH = 64 * 1024
-  const _buff = buffer(BUF_LENGTH)
-
-  const flags = overwrite ? 'w' : 'wx'
-
-  const fdr = fs.openSync(src, 'r')
-  const stat = fs.fstatSync(fdr)
-  const fdw = fs.openSync(dest, flags, stat.mode)
-  let pos = 0
-
-  while (pos < stat.size) {
-    const bytesRead = fs.readSync(fdr, _buff, 0, BUF_LENGTH, pos)
-    fs.writeSync(fdw, _buff, 0, bytesRead)
-    pos += bytesRead
+function rename (src, dest, overwrite) {
+  try {
+    fs.renameSync(src, dest)
+  } catch (err) {
+    if (err.code !== 'EXDEV') throw err
+    return moveAcrossDevice(src, dest, overwrite)
   }
-
-  fs.closeSync(fdr)
-  fs.closeSync(fdw)
-  return fs.unlinkSync(src)
 }
 
-function moveDirSyncAcrossDevice (src, dest, overwrite) {
-  const options = {
-    overwrite: false
+function moveAcrossDevice (src, dest, overwrite) {
+  const opts = {
+    overwrite,
+    errorOnExist: true
   }
 
-  if (overwrite) {
-    removeSync(dest)
-    tryCopySync()
-  } else {
-    tryCopySync()
-  }
-
-  function tryCopySync () {
-    copySync(src, dest, options)
-    return removeSync(src)
-  }
+  copySync(src, dest, opts)
+  return removeSync(src)
 }
 
-// return true if dest is a subdir of src, otherwise false.
-// extract dest base dir and check if that is the same as src basename
 function isSrcSubdir (src, dest) {
-  try {
-    return fs.statSync(src).isDirectory() &&
-           src !== dest &&
-           dest.indexOf(src) > -1 &&
-           dest.split(path.dirname(src) + path.sep)[1].split(path.sep)[0] === path.basename(src)
-  } catch (e) {
-    return false
-  }
+  const srcArr = src.split(path.sep)
+  const destArr = dest.split(path.sep)
+  return srcArr.reduce((acc, curr, i) => acc && destArr[i] === curr, true)
 }
 
-module.exports = {
-  moveSync
-}
+module.exports = { moveSync }