From 2cc1aebc12dd300f91eaa2edf8cb23298cdeee43 Mon Sep 17 00:00:00 2001 From: Mani Maghsoudlou Date: Sat, 4 Aug 2018 14:08:03 -0700 Subject: [PATCH] moveSync: refactor to use renameSync --- ...ve-sync-prevent-moving-into-itself.test.js | 2 +- lib/move-sync/__tests__/move-sync.test.js | 94 +++----------- lib/move-sync/index.js | 120 +++++------------- package.json | 1 - 4 files changed, 50 insertions(+), 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 index 3685faaf..1bf32c05 100644 --- 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 index d2c226b3..2139b8ce 100644 --- 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,16 +18,13 @@ 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()', () => { @@ -36,18 +32,15 @@ describe('moveSync()', () => { 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 index 6d4f56fa..b2dbc941 100644 --- a/lib/move-sync/index.js +++ b/lib/move-sync/index.js @@ -4,114 +4,54 @@ 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 +const pathExistsSync = require('../path-exists').pathExistsSync +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 (pathExistsSync(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 } diff --git a/package.json b/package.json index 3d4db47a..9c4f33fb 100644 --- a/package.json +++ b/package.json @@ -49,7 +49,6 @@ "mocha": "^5.0.5", "proxyquire": "^2.0.1", "read-dir-files": "^0.1.1", - "rimraf": "^2.2.8", "secure-random": "^1.1.1", "semver": "^5.3.0", "standard": "^11.0.1",