From 00063917f71f5d1f5cf55b55e664d8cb89fb99eb Mon Sep 17 00:00:00 2001 From: Mani Maghsoudlou Date: Mon, 6 Mar 2017 23:02:55 -0800 Subject: [PATCH 1/3] Add moveSync and its tests --- lib/index.js | 1 + lib/move-sync/__tests__/move-sync.test.js | 411 ++++++++++++++++++++++ lib/move-sync/index.js | 116 ++++++ 3 files changed, 528 insertions(+) create mode 100644 lib/move-sync/__tests__/move-sync.test.js create mode 100644 lib/move-sync/index.js diff --git a/lib/index.js b/lib/index.js index e840a9dc..d25fd544 100644 --- a/lib/index.js +++ b/lib/index.js @@ -18,6 +18,7 @@ assign(fs, require('./mkdirs')) assign(fs, require('./remove')) assign(fs, require('./json')) assign(fs, require('./move')) +assign(fs, require('./move-sync')) assign(fs, require('./empty')) assign(fs, require('./ensure')) assign(fs, require('./output')) diff --git a/lib/move-sync/__tests__/move-sync.test.js b/lib/move-sync/__tests__/move-sync.test.js new file mode 100644 index 00000000..87e84f87 --- /dev/null +++ b/lib/move-sync/__tests__/move-sync.test.js @@ -0,0 +1,411 @@ +'use strict' + +const fs = require('graceful-fs') +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 */ + +function createSyncErrFn (errCode) { + const fn = function () { + const err = new Error() + err.code = errCode + throw err + } + return fn +} + +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') + }) + + afterEach(done => rimraf(TEST_DIR, done)) + + it('should not move if src and dest are the same', () => { + const src = `${TEST_DIR}/a-file` + const dest = `${TEST_DIR}/a-file` + + try { + fse.moveSync(src, dest) + } catch (err) { + assert.ifError(err) + } + + // 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}`) + }) + + it('should rename a file on the same device', () => { + const src = `${TEST_DIR}/a-file` + const dest = `${TEST_DIR}/a-file-dest` + + try { + fse.moveSync(src, dest) + } catch (err) { + assert.ifError(err) + } + + const contents = fs.readFileSync(dest, 'utf8') + const expected = /^sonic the hedgehog\r?\n$/ + assert.ok(contents.match(expected), `${contents} match ${expected}`) + }) + + it('should not overwrite the destination by default', () => { + const src = `${TEST_DIR}/a-file` + const dest = `${TEST_DIR}/a-folder/another-file` + + // verify file exists already + assert(fs.existsSync(dest)) + + try { + fse.moveSync(src, dest) + } catch (err) { + assert.ok(err && err.code === 'EEXIST', 'throw EEXIST') + } + }) + + it('should not overwrite if overwrite = false', () => { + const src = `${TEST_DIR}/a-file` + const dest = `${TEST_DIR}/a-folder/another-file` + + // verify file exists already + assert(fs.existsSync(dest)) + + try { + fse.moveSync(src, dest, {overwrite: false}) + } catch (err) { + assert.ok(err && err.code === 'EEXIST', 'throw EEXIST') + } + }) + + it('should overwrite file if overwrite = true', () => { + const src = `${TEST_DIR}/a-file` + const dest = `${TEST_DIR}/a-folder/another-file` + + // verify file exists already + assert(fs.existsSync(dest)) + + try { + fse.moveSync(src, dest, {overwrite: true}) + } catch (err) { + assert.ifError(err) + } + + const contents = fs.readFileSync(dest, 'utf8') + const expected = /^sonic the hedgehog\r?\n$/ + assert.ok(contents.match(expected), `${contents} match ${expected}`) + }) + + it('should overwrite the destination directory if overwrite = true', function (done) { + // Tests fail on appveyor/Windows due to + // https://github.com/isaacs/node-graceful-fs/issues/98. + // Workaround by increasing the timeout by a minute (because + // graceful times out after a minute). + this.timeout(90000) + + // Create src + const src = path.join(TEST_DIR, 'src') + fse.ensureDirSync(src) + fse.mkdirsSync(path.join(src, 'some-folder')) + fs.writeFileSync(path.join(src, 'some-file'), 'hi') + + const dest = path.join(TEST_DIR, 'a-folder') + + // verify dest has stuff in it + const pathsBefore = fs.readdirSync(dest) + assert(pathsBefore.indexOf('another-file') >= 0) + assert(pathsBefore.indexOf('another-folder') >= 0) + + try { + fse.moveSync(src, dest, {overwrite: true}) + } catch (err) { + assert.ifError(err) + } + + // verify dest does not have old stuff + const pathsAfter = fs.readdirSync(dest) + assert.strictEqual(pathsAfter.indexOf('another-file'), -1) + assert.strictEqual(pathsAfter.indexOf('another-folder'), -1) + + // verify dest has new stuff + assert(pathsAfter.indexOf('some-file') >= 0) + assert(pathsAfter.indexOf('some-folder') >= 0) + done() + }) + + /* + it('should not create directory structure if mkdirp is false', done => { + const src = `${TEST_DIR}/a-file` + const dest = `${TEST_DIR}/does/not/exist/a-file-dest` + + // verify dest directory does not exist + assert(!fs.existsSync(path.dirname(dest))) + + fse.move(src, dest, {mkdirp: false}, err => { + assert.strictEqual(err.code, 'ENOENT') + done() + }) + }) + */ + + it('should create directory structure by default', () => { + const src = `${TEST_DIR}/a-file` + const dest = `${TEST_DIR}/does/not/exist/a-file-dest` + + // verify dest directory does not exist + assert(!fs.existsSync(path.dirname(dest))) + + try { + fse.moveSync(src, dest) + } catch (err) { + assert.ifError(err) + } + + const contents = fs.readFileSync(dest, 'utf8') + const expected = /^sonic the hedgehog\r?\n$/ + assert.ok(contents.match(expected), `${contents} match ${expected}`) + }) + + it('should work across devices', () => { + const src = `${TEST_DIR}/a-file` + const dest = `${TEST_DIR}/a-file-dest` + + setUpMockFs('EXDEV') + + try { + fse.moveSync(src, dest) + } catch (err) { + assert.ifError(err) + } + + const contents = fs.readFileSync(dest, 'utf8') + const expected = /^sonic the hedgehog\r?\n$/ + assert.ok(contents.match(expected), `${contents} match ${expected}`) + tearDownMockFs('EXDEV') + }) + + it('should move folders', () => { + const src = `${TEST_DIR}/a-folder` + const dest = `${TEST_DIR}/a-folder-dest` + + // verify it doesn't exist + assert(!fs.existsSync(dest)) + + try { + fse.moveSync(src, dest) + } catch (err) { + assert.ifError(err) + } + + 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') + + try { + fse.moveSync(src, dest) + } catch (err) { + assert.ifError(err) + } + + const contents = fs.readFileSync(dest + '/another-folder/file3', 'utf8') + const expected = /^knuckles\r?\n$/ + assert.ok(contents.match(expected), `${contents} match ${expected}`) + tearDownMockFs('EISDIR') + }) + + it('should overwrite folders across devices', () => { + const src = `${TEST_DIR}/a-folder` + const dest = `${TEST_DIR}/a-folder-dest` + + fs.mkdirSync(dest) + + setUpMockFs('EXDEV') + + try { + fse.moveSync(src, dest, {overwrite: true}) + } catch (err) { + assert.ifError(err) + } + + const contents = fs.readFileSync(dest + '/another-folder/file3', 'utf8') + const expected = /^knuckles\r?\n$/ + assert.ok(contents.match(expected), `${contents} match ${expected}`) + tearDownMockFs('EXDEV') + }) + + it('should move folders across devices with EXDEV error', () => { + const src = `${TEST_DIR}/a-folder` + const dest = `${TEST_DIR}/a-folder-dest` + + setUpMockFs('EXDEV') + + try { + fse.moveSync(src, dest) + } catch (err) { + assert.ifError(err) + } + + const contents = fs.readFileSync(dest + '/another-folder/file3', 'utf8') + const expected = /^knuckles\r?\n$/ + assert.ok(contents.match(expected), `${contents} match ${expected}`) + tearDownMockFs('EXDEV') + }) + + it('should move folders across devices with EPERM error', () => { + const src = `${TEST_DIR}/a-folder` + const dest = `${TEST_DIR}/a-folder-dest` + + setUpMockFs('EPERM') + + try { + fse.moveSync(src, dest) + } catch (err) { + assert.ifError(err) + } + + const contents = fs.readFileSync(dest + '/another-folder/file3', 'utf8') + const expected = /^knuckles\r?\n$/ + assert.ok(contents.match(expected), `${contents} match ${expected}`) + tearDownMockFs('EPERM') + }) + + it('should move folders across devices with ENOTSUP error', () => { + const src = `${TEST_DIR}/a-folder` + const dest = `${TEST_DIR}/a-folder-dest` + + setUpMockFs('ENOTSUP') + + try { + fse.moveSync(src, dest) + } catch (err) { + assert.ifError(err) + } + + const contents = fs.readFileSync(dest + '/another-folder/file3', 'utf8') + const expected = /^knuckles\r?\n$/ + assert.ok(contents.match(expected), `${contents} match ${expected}`) + tearDownMockFs('ENOTSUP') + }) + + describe('clobber', () => { + it('should be an alias for overwrite', () => { + const src = `${TEST_DIR}/a-file` + const dest = `${TEST_DIR}/a-folder/another-file` + + // verify file exists already + assert(fs.existsSync(dest)) + + try { + fse.moveSync(src, dest, {clobber: true}) + } catch (err) { + assert.ifError(err) + } + + const contents = fs.readFileSync(dest, 'utf8') + const expected = /^sonic the hedgehog\r?\n$/ + assert.ok(contents.match(expected), `${contents} match ${expected}`) + }) + }) + + describe.skip('> when trying to a move a folder into itself', () => { + it('should produce an error', () => { + const SRC_DIR = path.join(TEST_DIR, 'test') + const DEST_DIR = path.join(TEST_DIR, 'test', 'test') + + assert(!fs.existsSync(SRC_DIR)) + fs.mkdirSync(SRC_DIR) + assert(fs.existsSync(SRC_DIR)) + + try { + fse.moveSync(SRC_DIR, DEST_DIR) + } catch (err) { + assert(err) + assert(fs.existsSync(SRC_DIR)) + } + }) + }) + + // tested on Linux ubuntu 3.13.0-32-generic #57-Ubuntu SMP i686 i686 GNU/Linux + // this won't trigger a bug on Mac OS X Yosimite with a USB drive (/Volumes) + // see issue #108 + describe('> when actually trying to a move a folder across devices', () => { + const differentDevice = '/mnt' + let __skipTests = false + + // must set this up, if not, exit silently + if (!fs.existsSync(differentDevice)) { + console.log('Skipping cross-device move test') + __skipTests = true + } + + // make sure we have permission on device + try { + fs.writeFileSync(path.join(differentDevice, 'file'), 'hi') + } catch (err) { + console.log("Can't write to device. Skipping moveSync test.") + __skipTests = true + } + + const _it = __skipTests ? it.skip : it + + describe('> just the folder', () => { + _it('should move the folder', () => { + const src = '/mnt/some/weird/dir-really-weird' + const dest = path.join(TEST_DIR, 'device-weird') + + if (!fs.existsSync(src)) { + fse.mkdirpSync(src) + } + + assert(!fs.existsSync(dest)) + + assert(fs.lstatSync(src).isDirectory()) + + try { + fse.moveSync(src, dest) + } catch (err) { + assert.ifError(err) + } + assert(fs.existsSync(dest)) + assert(fs.lstatSync(dest).isDirectory()) + }) + }) + }) +}) diff --git a/lib/move-sync/index.js b/lib/move-sync/index.js new file mode 100644 index 00000000..43bd0e0d --- /dev/null +++ b/lib/move-sync/index.js @@ -0,0 +1,116 @@ +'use strict' + +// most of this code was written by Andrew Kelley +// licensed under the BSD license: see +// https://github.com/andrewrk/node-mv/blob/master/package.json +// This is the sync version that somehow follows the same pattern. + +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 + +function moveSync (src, dest, options) { + options = options || {} + const overwrite = options.overwrite || options.clobber || false + + if (path.resolve(src) === path.resolve(dest)) return + + 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 + } + } + } +} + +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 moveFileSyncAcrossDevice (src, dest, overwrite) { + const BUF_LENGTH = 64 * 1024 + const _buff = Buffer.alloc(BUF_LENGTH) + + const flags = overwrite ? 'w' : 'wx' + + try { + const fdr = fs.openSync(src, 'r') + const stat = fs.fstatSync(fdr) + const fdw = fs.openSync(dest, flags, stat.mode) + let bytesRead = 1 + let pos = 0 + + while (bytesRead > 0) { + bytesRead = fs.readSync(fdr, _buff, 0, BUF_LENGTH, pos) + fs.writeSync(fdw, _buff, 0, bytesRead) + pos += bytesRead + } + + fs.closeSync(fdr) + fs.closeSync(fdw) + return fs.unlinkSync(src) + } catch (err) { + // may want to create a directory but `out` line above + // creates an empty file for us: See #108 + // don't care about error here + fs.unlinkSync(dest) + // note: `err` here is from the fdr error + if (err.code === 'EISDIR' || err.code === 'EPERM') { + return moveDirSyncAcrossDevice(src, dest, overwrite) + } + throw err + } +} + +function moveDirSyncAcrossDevice (src, dest, overwrite) { + const options = { + overwrite: false + } + + if (overwrite) { + removeSync(dest) + tryCopySync() + } else { + tryCopySync() + } + + function tryCopySync () { + copySync(src, dest, options) + return removeSync(src) + } +} + +module.exports = { + moveSync +} From 08d7d3aaac4075a8f8f10f8fc2f8c15869b10289 Mon Sep 17 00:00:00 2001 From: Mani Maghsoudlou Date: Wed, 8 Mar 2017 00:57:37 -0800 Subject: [PATCH 2/3] Add prevent moving into itself and its tests --- ...ve-sync-prevent-moving-into-itself.test.js | 166 ++++++++++++++++++ lib/move-sync/__tests__/move-sync.test.js | 139 +++++---------- lib/move-sync/index.js | 63 +++---- package.json | 1 + 4 files changed, 239 insertions(+), 130 deletions(-) create mode 100644 lib/move-sync/__tests__/move-sync-prevent-moving-into-itself.test.js 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 new file mode 100644 index 00000000..2470b84f --- /dev/null +++ b/lib/move-sync/__tests__/move-sync-prevent-moving-into-itself.test.js @@ -0,0 +1,166 @@ +'use strict' + +const assert = require('assert') +const os = require('os') +const path = require('path') +const fs = require(process.cwd()) +const klawSync = require('klaw-sync') + +/* global beforeEach, afterEach, describe, it */ + +const FILES = [ + 'file0.txt', + path.join('dir1', 'file1.txt'), + path.join('dir1', 'dir2', 'file2.txt'), + path.join('dir1', 'dir2', 'dir3', 'file3.txt') +] + +const dat0 = 'file0' +const dat1 = 'file1' +const dat2 = 'file2' +const dat3 = 'file3' + +describe('+ moveSync() - prevent moving into itself', () => { + let TEST_DIR, src, dest + + beforeEach(() => { + TEST_DIR = path.join(os.tmpdir(), 'fs-extra', 'move-sync-prevent-moving-into-itself') + src = path.join(TEST_DIR, 'src') + fs.mkdirsSync(src) + + fs.outputFileSync(path.join(src, FILES[0]), dat0) + fs.outputFileSync(path.join(src, FILES[1]), dat1) + fs.outputFileSync(path.join(src, FILES[2]), dat2) + fs.outputFileSync(path.join(src, FILES[3]), dat3) + }) + + afterEach(() => fs.removeSync(TEST_DIR)) + + describe('> when source is a file', () => { + it(`should move the file successfully even when dest is a subdir of src`, () => { + const srcFile = path.join(TEST_DIR, 'src', 'srcfile.txt') + const destFile = path.join(TEST_DIR, 'src', 'dest', 'destfile.txt') + fs.writeFileSync(srcFile, dat0) + + fs.moveSync(srcFile, destFile) + + const out = fs.readFileSync(destFile, 'utf8') + assert.strictEqual(out, dat0, 'file contents matched') + assert(!fs.existsSync(srcFile)) + }) + }) + + describe('> when source is a directory', () => { + it(`should move the directory successfully when dest is 'src_dest'`, () => { + dest = path.join(TEST_DIR, 'src_dest') + return testSuccess(src, dest) + }) + + it(`should move the directory successfully when dest is 'src-dest'`, () => { + dest = path.join(TEST_DIR, 'src-dest') + return testSuccess(src, dest) + }) + + it(`should move the directory successfully when dest is 'dest_src'`, () => { + dest = path.join(TEST_DIR, 'dest_src') + return testSuccess(src, dest) + }) + + it(`should move the directory successfully when dest is 'src_dest/src'`, () => { + dest = path.join(TEST_DIR, 'src_dest', 'src') + return testSuccess(src, dest) + }) + + it(`should move the directory successfully when dest is 'src-dest/src'`, () => { + dest = path.join(TEST_DIR, 'src-dest', 'src') + return testSuccess(src, dest) + }) + + it(`should move the directory successfully when dest is 'dest_src/src'`, () => { + dest = path.join(TEST_DIR, 'dest_src', 'src') + return testSuccess(src, dest) + }) + + it(`should move the directory successfully when dest is 'src_src/dest'`, () => { + dest = path.join(TEST_DIR, 'src_src', 'dest') + return testSuccess(src, dest) + }) + + it(`should move the directory successfully when dest is 'src-src/dest'`, () => { + dest = path.join(TEST_DIR, 'src-src', 'dest') + return testSuccess(src, dest) + }) + + it(`should move the directory successfully when dest is 'srcsrc/dest'`, () => { + dest = path.join(TEST_DIR, 'srcsrc', 'dest') + return testSuccess(src, dest) + }) + + it(`should move the directory successfully when dest is 'dest/src'`, () => { + dest = path.join(TEST_DIR, 'dest', 'src') + return testSuccess(src, dest) + }) + + it('should move the directory successfully when dest is very nested that all its parents need to be created', () => { + dest = path.join(TEST_DIR, 'dest', 'src', 'foo', 'bar', 'baz', 'qux', 'quux', 'waldo', + 'grault', 'garply', 'fred', 'plugh', 'thud', 'some', 'highly', 'deeply', + 'badly', 'nasty', 'crazy', 'mad', 'nested', 'dest') + assert(!fs.existsSync(dest)) + return testSuccess(src, dest) + }) + + it(`should throw error when dest is 'src/dest'`, () => { + dest = path.join(TEST_DIR, 'src', 'dest') + return testError(src, dest) + }) + + it(`should throw error when dest is 'src/src_dest'`, () => { + dest = path.join(TEST_DIR, 'src', 'src_dest') + return testError(src, dest) + }) + + it(`should throw error when dest is 'src/dest_src'`, () => { + dest = path.join(TEST_DIR, 'src', 'dest_src') + return testError(src, dest) + }) + + it(`should throw error when dest is 'src/dest/src'`, () => { + dest = path.join(TEST_DIR, 'src', 'dest', 'src') + return testError(src, dest) + }) + }) +}) + +function testSuccess (src, dest) { + const srclen = klawSync(src).length + // assert src has contents + assert(srclen > 2) + + fs.moveSync(src, dest) + + const destlen = klawSync(dest).length + + // assert src and dest length are the same + assert.strictEqual(destlen, srclen, 'src and dest length should be equal') + + const o0 = fs.readFileSync(path.join(dest, FILES[0]), 'utf8') + const o1 = fs.readFileSync(path.join(dest, FILES[1]), 'utf8') + const o2 = fs.readFileSync(path.join(dest, FILES[2]), 'utf8') + const o3 = fs.readFileSync(path.join(dest, FILES[3]), 'utf8') + + assert.strictEqual(o0, dat0, 'file contents matched') + assert.strictEqual(o1, dat1, 'file contents matched') + assert.strictEqual(o2, dat2, 'file contents matched') + assert.strictEqual(o3, dat3, 'file contents matched') + assert(!fs.existsSync(src)) +} + +function testError (src, dest) { + try { + fs.moveSync(src, dest) + } catch (err) { + assert.strictEqual(err.message, `Cannot move '${src}' into 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 87e84f87..0872dfcc 100644 --- a/lib/move-sync/__tests__/move-sync.test.js +++ b/lib/move-sync/__tests__/move-sync.test.js @@ -53,11 +53,7 @@ describe('moveSync()', () => { const src = `${TEST_DIR}/a-file` const dest = `${TEST_DIR}/a-file` - try { - fse.moveSync(src, dest) - } catch (err) { - assert.ifError(err) - } + fse.moveSync(src, dest) // assert src not affected const contents = fs.readFileSync(src, 'utf8') @@ -69,11 +65,7 @@ describe('moveSync()', () => { const src = `${TEST_DIR}/a-file` const dest = `${TEST_DIR}/a-file-dest` - try { - fse.moveSync(src, dest) - } catch (err) { - assert.ifError(err) - } + fse.moveSync(src, dest) const contents = fs.readFileSync(dest, 'utf8') const expected = /^sonic the hedgehog\r?\n$/ @@ -115,11 +107,7 @@ describe('moveSync()', () => { // verify file exists already assert(fs.existsSync(dest)) - try { - fse.moveSync(src, dest, {overwrite: true}) - } catch (err) { - assert.ifError(err) - } + fse.moveSync(src, dest, {overwrite: true}) const contents = fs.readFileSync(dest, 'utf8') const expected = /^sonic the hedgehog\r?\n$/ @@ -146,11 +134,7 @@ describe('moveSync()', () => { assert(pathsBefore.indexOf('another-file') >= 0) assert(pathsBefore.indexOf('another-folder') >= 0) - try { - fse.moveSync(src, dest, {overwrite: true}) - } catch (err) { - assert.ifError(err) - } + fse.moveSync(src, dest, {overwrite: true}) // verify dest does not have old stuff const pathsAfter = fs.readdirSync(dest) @@ -163,21 +147,6 @@ describe('moveSync()', () => { done() }) - /* - it('should not create directory structure if mkdirp is false', done => { - const src = `${TEST_DIR}/a-file` - const dest = `${TEST_DIR}/does/not/exist/a-file-dest` - - // verify dest directory does not exist - assert(!fs.existsSync(path.dirname(dest))) - - fse.move(src, dest, {mkdirp: false}, err => { - assert.strictEqual(err.code, 'ENOENT') - done() - }) - }) - */ - it('should create directory structure by default', () => { const src = `${TEST_DIR}/a-file` const dest = `${TEST_DIR}/does/not/exist/a-file-dest` @@ -185,11 +154,7 @@ describe('moveSync()', () => { // verify dest directory does not exist assert(!fs.existsSync(path.dirname(dest))) - try { - fse.moveSync(src, dest) - } catch (err) { - assert.ifError(err) - } + fse.moveSync(src, dest) const contents = fs.readFileSync(dest, 'utf8') const expected = /^sonic the hedgehog\r?\n$/ @@ -202,16 +167,12 @@ describe('moveSync()', () => { setUpMockFs('EXDEV') - try { - fse.moveSync(src, dest) - } catch (err) { - assert.ifError(err) - } + fse.moveSync(src, dest) const contents = fs.readFileSync(dest, 'utf8') const expected = /^sonic the hedgehog\r?\n$/ assert.ok(contents.match(expected), `${contents} match ${expected}`) - tearDownMockFs('EXDEV') + tearDownMockFs() }) it('should move folders', () => { @@ -221,11 +182,7 @@ describe('moveSync()', () => { // verify it doesn't exist assert(!fs.existsSync(dest)) - try { - fse.moveSync(src, dest) - } catch (err) { - assert.ifError(err) - } + fse.moveSync(src, dest) const contents = fs.readFileSync(dest + '/another-file', 'utf8') const expected = /^tails\r?\n$/ @@ -238,16 +195,12 @@ describe('moveSync()', () => { setUpMockFs('EISDIR') - try { - fse.moveSync(src, dest) - } catch (err) { - assert.ifError(err) - } + 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('EISDIR') + tearDownMockFs() }) it('should overwrite folders across devices', () => { @@ -258,16 +211,12 @@ describe('moveSync()', () => { setUpMockFs('EXDEV') - try { - fse.moveSync(src, dest, {overwrite: true}) - } catch (err) { - assert.ifError(err) - } + fse.moveSync(src, dest, {overwrite: true}) const contents = fs.readFileSync(dest + '/another-folder/file3', 'utf8') const expected = /^knuckles\r?\n$/ assert.ok(contents.match(expected), `${contents} match ${expected}`) - tearDownMockFs('EXDEV') + tearDownMockFs() }) it('should move folders across devices with EXDEV error', () => { @@ -276,16 +225,12 @@ describe('moveSync()', () => { setUpMockFs('EXDEV') - try { - fse.moveSync(src, dest) - } catch (err) { - assert.ifError(err) - } + 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('EXDEV') + tearDownMockFs() }) it('should move folders across devices with EPERM error', () => { @@ -294,16 +239,12 @@ describe('moveSync()', () => { setUpMockFs('EPERM') - try { - fse.moveSync(src, dest) - } catch (err) { - assert.ifError(err) - } + 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('EPERM') + tearDownMockFs() }) it('should move folders across devices with ENOTSUP error', () => { @@ -312,16 +253,12 @@ describe('moveSync()', () => { setUpMockFs('ENOTSUP') - try { - fse.moveSync(src, dest) - } catch (err) { - assert.ifError(err) - } + 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('ENOTSUP') + tearDownMockFs() }) describe('clobber', () => { @@ -332,11 +269,7 @@ describe('moveSync()', () => { // verify file exists already assert(fs.existsSync(dest)) - try { - fse.moveSync(src, dest, {clobber: true}) - } catch (err) { - assert.ifError(err) - } + fse.moveSync(src, dest, {clobber: true}) const contents = fs.readFileSync(dest, 'utf8') const expected = /^sonic the hedgehog\r?\n$/ @@ -344,10 +277,10 @@ describe('moveSync()', () => { }) }) - describe.skip('> when trying to a move a folder into itself', () => { + describe('> when trying to a move a folder into itself', () => { it('should produce an error', () => { - const SRC_DIR = path.join(TEST_DIR, 'test') - const DEST_DIR = path.join(TEST_DIR, 'test', 'test') + const SRC_DIR = path.join(TEST_DIR, 'src') + const DEST_DIR = path.join(TEST_DIR, 'src', 'dest') assert(!fs.existsSync(SRC_DIR)) fs.mkdirSync(SRC_DIR) @@ -356,22 +289,33 @@ describe('moveSync()', () => { try { fse.moveSync(SRC_DIR, DEST_DIR) } catch (err) { - assert(err) + assert(err.message, `Cannot move ${SRC_DIR} into itself ${DEST_DIR}.`) assert(fs.existsSync(SRC_DIR)) + assert(!fs.existsSync(DEST_DIR)) } }) }) - // tested on Linux ubuntu 3.13.0-32-generic #57-Ubuntu SMP i686 i686 GNU/Linux - // this won't trigger a bug on Mac OS X Yosimite with a USB drive (/Volumes) - // see issue #108 + describe('> when trying to a move a file into its parent subdirectory', () => { + it('should move successfully', () => { + const src = `${TEST_DIR}/a-file` + const dest = `${TEST_DIR}/dest/a-file-dest` + + fse.moveSync(src, dest) + + const contents = fs.readFileSync(dest, 'utf8') + const expected = /^sonic the hedgehog\r?\n$/ + assert.ok(contents.match(expected), `${contents} match ${expected}`) + }) + }) + describe('> when actually trying to a move a folder across devices', () => { const differentDevice = '/mnt' let __skipTests = false // must set this up, if not, exit silently if (!fs.existsSync(differentDevice)) { - console.log('Skipping cross-device move test') + console.log('Skipping cross-device moveSync test') __skipTests = true } @@ -398,11 +342,8 @@ describe('moveSync()', () => { assert(fs.lstatSync(src).isDirectory()) - try { - fse.moveSync(src, dest) - } catch (err) { - assert.ifError(err) - } + fse.moveSync(src, dest) + assert(fs.existsSync(dest)) assert(fs.lstatSync(dest).isDirectory()) }) diff --git a/lib/move-sync/index.js b/lib/move-sync/index.js index 43bd0e0d..435fbe15 100644 --- a/lib/move-sync/index.js +++ b/lib/move-sync/index.js @@ -1,10 +1,5 @@ 'use strict' -// most of this code was written by Andrew Kelley -// licensed under the BSD license: see -// https://github.com/andrewrk/node-mv/blob/master/package.json -// This is the sync version that somehow follows the same pattern. - const fs = require('graceful-fs') const path = require('path') const copySync = require('../copy-sync').copySync @@ -15,7 +10,12 @@ function moveSync (src, dest, options) { options = options || {} const overwrite = options.overwrite || options.clobber || false - if (path.resolve(src) === path.resolve(dest)) return + src = path.resolve(src) + dest = path.resolve(dest) + + if (src === dest) return + + if (isSrcSubdir(src, dest)) throw new Error(`Cannot move '${src}' into itself '${dest}'.`) mkdirpSync(path.dirname(dest)) tryRenameSync() @@ -64,33 +64,21 @@ function moveFileSyncAcrossDevice (src, dest, overwrite) { const flags = overwrite ? 'w' : 'wx' - try { - const fdr = fs.openSync(src, 'r') - const stat = fs.fstatSync(fdr) - const fdw = fs.openSync(dest, flags, stat.mode) - let bytesRead = 1 - let pos = 0 - - while (bytesRead > 0) { - bytesRead = fs.readSync(fdr, _buff, 0, BUF_LENGTH, pos) - fs.writeSync(fdw, _buff, 0, bytesRead) - pos += bytesRead - } + const fdr = fs.openSync(src, 'r') + const stat = fs.fstatSync(fdr) + const fdw = fs.openSync(dest, flags, stat.mode) + let bytesRead = 1 + let pos = 0 - fs.closeSync(fdr) - fs.closeSync(fdw) - return fs.unlinkSync(src) - } catch (err) { - // may want to create a directory but `out` line above - // creates an empty file for us: See #108 - // don't care about error here - fs.unlinkSync(dest) - // note: `err` here is from the fdr error - if (err.code === 'EISDIR' || err.code === 'EPERM') { - return moveDirSyncAcrossDevice(src, dest, overwrite) - } - throw err + while (bytesRead > 0) { + bytesRead = fs.readSync(fdr, _buff, 0, BUF_LENGTH, pos) + fs.writeSync(fdw, _buff, 0, bytesRead) + pos += bytesRead } + + fs.closeSync(fdr) + fs.closeSync(fdw) + return fs.unlinkSync(src) } function moveDirSyncAcrossDevice (src, dest, overwrite) { @@ -111,6 +99,19 @@ function moveDirSyncAcrossDevice (src, dest, overwrite) { } } +// 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 + } +} + module.exports = { moveSync } diff --git a/package.json b/package.json index 3b545188..bdd2d41c 100644 --- a/package.json +++ b/package.json @@ -40,6 +40,7 @@ "coveralls": "^2.11.2", "istanbul": "^0.4.5", "klaw": "^1.0.0", + "klaw-sync": "^1.1.2", "minimist": "^1.1.1", "mocha": "^3.1.2", "proxyquire": "^1.7.10", From 535fe8d393f7d43c60883383251574596eb4b35f Mon Sep 17 00:00:00 2001 From: Mani Maghsoudlou Date: Wed, 8 Mar 2017 04:05:46 -0800 Subject: [PATCH 3/3] Add more tests for moveSync when moving files into src subdir --- ...ve-sync-prevent-moving-into-itself.test.js | 61 +++++++++++++------ 1 file changed, 42 insertions(+), 19 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 2470b84f..3685faaf 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 @@ -37,68 +37,81 @@ describe('+ moveSync() - prevent moving into itself', () => { afterEach(() => fs.removeSync(TEST_DIR)) describe('> when source is a file', () => { - it(`should move the file successfully even when dest is a subdir of src`, () => { - const srcFile = path.join(TEST_DIR, 'src', 'srcfile.txt') + it(`should move the file successfully even when dest parent is 'src/dest'`, () => { const destFile = path.join(TEST_DIR, 'src', 'dest', 'destfile.txt') - fs.writeFileSync(srcFile, dat0) + return testSuccessFile(src, destFile) + }) + + it(`should move the file successfully when dest parent is 'src/src_dest'`, () => { + const destFile = path.join(TEST_DIR, 'src', 'src_dest', 'destfile.txt') + return testSuccessFile(src, destFile) + }) - fs.moveSync(srcFile, destFile) + it(`should move the file successfully when dest parent is 'src/dest_src'`, () => { + const destFile = path.join(TEST_DIR, 'src', 'dest_src', 'destfile.txt') + return testSuccessFile(src, destFile) + }) + + it(`should move the file successfully when dest parent is 'src/dest/src'`, () => { + const destFile = path.join(TEST_DIR, 'src', 'dest', 'src', 'destfile.txt') + return testSuccessFile(src, destFile) + }) - const out = fs.readFileSync(destFile, 'utf8') - assert.strictEqual(out, dat0, 'file contents matched') - assert(!fs.existsSync(srcFile)) + it(`should move the file successfully when dest parent is 'srcsrc/dest'`, () => { + const destFile = path.join(TEST_DIR, 'srcsrc', 'dest', 'destfile.txt') + return testSuccessFile(src, destFile) }) }) describe('> when source is a directory', () => { it(`should move the directory successfully when dest is 'src_dest'`, () => { dest = path.join(TEST_DIR, 'src_dest') - return testSuccess(src, dest) + return testSuccessDir(src, dest) }) it(`should move the directory successfully when dest is 'src-dest'`, () => { dest = path.join(TEST_DIR, 'src-dest') - return testSuccess(src, dest) + return testSuccessDir(src, dest) }) it(`should move the directory successfully when dest is 'dest_src'`, () => { dest = path.join(TEST_DIR, 'dest_src') - return testSuccess(src, dest) + return testSuccessDir(src, dest) }) it(`should move the directory successfully when dest is 'src_dest/src'`, () => { dest = path.join(TEST_DIR, 'src_dest', 'src') - return testSuccess(src, dest) + return testSuccessDir(src, dest) }) it(`should move the directory successfully when dest is 'src-dest/src'`, () => { dest = path.join(TEST_DIR, 'src-dest', 'src') - return testSuccess(src, dest) + return testSuccessDir(src, dest) }) it(`should move the directory successfully when dest is 'dest_src/src'`, () => { dest = path.join(TEST_DIR, 'dest_src', 'src') - return testSuccess(src, dest) + return testSuccessDir(src, dest) }) it(`should move the directory successfully when dest is 'src_src/dest'`, () => { dest = path.join(TEST_DIR, 'src_src', 'dest') - return testSuccess(src, dest) + return testSuccessDir(src, dest) }) it(`should move the directory successfully when dest is 'src-src/dest'`, () => { dest = path.join(TEST_DIR, 'src-src', 'dest') - return testSuccess(src, dest) + return testSuccessDir(src, dest) }) it(`should move the directory successfully when dest is 'srcsrc/dest'`, () => { dest = path.join(TEST_DIR, 'srcsrc', 'dest') - return testSuccess(src, dest) + return testSuccessDir(src, dest) }) it(`should move the directory successfully when dest is 'dest/src'`, () => { dest = path.join(TEST_DIR, 'dest', 'src') - return testSuccess(src, dest) + return testSuccessDir(src, dest) }) it('should move the directory successfully when dest is very nested that all its parents need to be created', () => { @@ -106,7 +119,7 @@ describe('+ moveSync() - prevent moving into itself', () => { 'grault', 'garply', 'fred', 'plugh', 'thud', 'some', 'highly', 'deeply', 'badly', 'nasty', 'crazy', 'mad', 'nested', 'dest') assert(!fs.existsSync(dest)) - return testSuccess(src, dest) + return testSuccessDir(src, dest) }) it(`should throw error when dest is 'src/dest'`, () => { @@ -131,7 +144,17 @@ describe('+ moveSync() - prevent moving into itself', () => { }) }) -function testSuccess (src, dest) { +function testSuccessFile (src, destFile) { + const srcFile = path.join(src, FILES[0]) + + fs.moveSync(srcFile, destFile) + + const o0 = fs.readFileSync(destFile, 'utf8') + assert.strictEqual(o0, dat0, 'file contents matched') + assert(!fs.existsSync(srcFile)) +} + +function testSuccessDir (src, dest) { const srclen = klawSync(src).length // assert src has contents assert(srclen > 2)