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",