Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

move: do not create parent directory if it is root #897

Merged
merged 6 commits into from May 3, 2021
Merged

move: do not create parent directory if it is root #897

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
56be88f
move: do not create parent directory if it is root
manidlou May 1, 2021
58f4021
fix move-sync test
manidlou May 1, 2021
78d2bfe
move-sync test: remove dest after test is done
manidlou May 1, 2021
3050210
remove debug log
manidlou May 1, 2021
d42f350
remove dest in afterEach
manidlou May 1, 2021
060cd87
call done() after test is done
manidlou May 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 20 additions & 1 deletion lib/move-sync/__tests__/move-sync.test.js
View file
Open in desktop
Expand Up @@ -10,6 +10,8 @@ const assert = require('assert')

/* global afterEach, beforeEach, describe, it */

const describeIfWindows = process.platform === 'win32' ? describe : describe.skip

function createSyncErrFn (errCode) {
const fn = function () {
const err = new Error()
Expand Down Expand Up @@ -120,7 +122,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.ok(contents.match(expected))
})

it('should overwrite the destination directory if overwrite = true', () => {
Expand Down Expand Up @@ -268,6 +270,23 @@ describe('moveSync()', () => {
})
})

describeIfWindows('> when dest parent is root', () => {
it('should not create parent directory', () => {
const src = path.join(TEST_DIR, 'a-file')
const dest = path.join(path.parse(TEST_DIR).root, 'another-file')

fse.moveSync(src, dest)

const contents = fs.readFileSync(dest, 'utf8')
const expected = /^sonic the hedgehog\r?\n$/
assert(contents.match(expected))
// this test is a bit special since
// we're moving to root, so we need to
// manually remove dest after we're done.
fse.removeSync(dest)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be in an afterEach in this describeIfWindows block.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense! I will update it then.

})
})

describe('> when actually trying to move a folder across devices', () => {
const differentDevice = '/mnt'
let __skipTests = false
Expand Down
8 changes: 7 additions & 1 deletion lib/move-sync/move-sync.js
View file
Open in desktop
Expand Up @@ -13,10 +13,16 @@ function moveSync (src, dest, opts) {

const { srcStat, isChangingCase = false } = stat.checkPathsSync(src, dest, 'move', opts)
stat.checkParentPathsSync(src, srcStat, dest, 'move')
mkdirpSync(path.dirname(dest))
if (!isParentRoot(dest)) mkdirpSync(path.dirname(dest))
return doRename(src, dest, overwrite, isChangingCase)
}

function isParentRoot (dest) {
const parent = path.dirname(dest)
const parsedPath = path.parse(parent)
return parsedPath.root === parent
}

function doRename (src, dest, overwrite, isChangingCase) {
if (isChangingCase) return rename(src, dest, overwrite)
if (overwrite) {
Expand Down
38 changes: 30 additions & 8 deletions lib/move/__tests__/move.test.js
View file
Open in desktop
Expand Up @@ -8,6 +8,8 @@ const assert = require('assert')

/* global afterEach, beforeEach, describe, it */

const describeIfWindows = process.platform === 'win32' ? describe : describe.skip

function createAsyncErrFn (errCode) {
const fn = function (...args) {
fn.callCount++
Expand Down Expand Up @@ -63,7 +65,7 @@ describe('+ move()', () => {
fs.readFile(dest, 'utf8', (err, contents) => {
const expected = /^sonic the hedgehog\r?\n$/
assert.ifError(err)
assert.ok(contents.match(expected), `${contents} match ${expected}`)
assert.ok(contents.match(expected))
done()
})
})
Expand Down Expand Up @@ -114,7 +116,7 @@ describe('+ move()', () => {
fs.readFile(path.join(dest, 'another-folder', 'file3'), 'utf8', (err, contents) => {
const expected = /^knuckles\r?\n$/
assert.ifError(err)
assert.ok(contents.match(expected), `${contents} match ${expected}`)
assert.ok(contents.match(expected))
tearDownMockFs()
done()
})
Expand All @@ -132,7 +134,7 @@ describe('+ move()', () => {
fs.readFile(dest, 'utf8', (err, contents) => {
const expected = /^sonic the hedgehog\r?\n$/
assert.ifError(err)
assert.ok(contents.match(expected), `${contents} match ${expected}`)
assert.ok(contents.match(expected))
done()
})
})
Expand Down Expand Up @@ -206,7 +208,7 @@ describe('+ move()', () => {
fs.readFile(dest, 'utf8', (err, contents) => {
const expected = /^sonic the hedgehog\r?\n$/
assert.ifError(err)
assert.ok(contents.match(expected), `${contents} match ${expected}`)
assert.ok(contents.match(expected))
done()
})
})
Expand All @@ -225,7 +227,7 @@ describe('+ move()', () => {
fs.readFile(dest, 'utf8', (err, contents) => {
const expected = /^sonic the hedgehog\r?\n$/
assert.ifError(err)
assert.ok(contents.match(expected), `${contents} match ${expected}`)
assert.ok(contents.match(expected))
tearDownMockFs()
done()
})
Expand All @@ -244,7 +246,7 @@ describe('+ move()', () => {
fs.readFile(path.join(dest, 'another-file'), 'utf8', (err, contents) => {
const expected = /^tails\r?\n$/
assert.ifError(err)
assert.ok(contents.match(expected), `${contents} match ${expected}`)
assert.ok(contents.match(expected))
done()
})
})
Expand All @@ -263,14 +265,34 @@ describe('+ move()', () => {
fs.readFile(path.join(dest, 'another-folder', 'file3'), 'utf8', (err, contents) => {
const expected = /^knuckles\r?\n$/
assert.ifError(err)
assert.ok(contents.match(expected), `${contents} match ${expected}`)
assert.ok(contents.match(expected))
tearDownMockFs()
done()
})
})
})
})

describeIfWindows('> when dest parent is root', () => {
it('should not create parent directory', done => {
const src = path.join(TEST_DIR, 'a-file')
const dest = path.join(path.parse(TEST_DIR).root, 'another-file')

fse.move(src, dest, err => {
assert.ifError(err)
fs.readFile(dest, 'utf8', (err, contents) => {
const expected = /^sonic the hedgehog\r?\n$/
assert.ifError(err)
assert.ok(contents.match(expected))
// this test is a bit special since
// we're moving to root, so we need to
// manually remove dest after we're done.
fse.remove(dest, done)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

})
})
})
})

describe('> clobber', () => {
it('should be an alias for overwrite', done => {
const src = path.join(TEST_DIR, 'a-file')
Expand All @@ -284,7 +306,7 @@ describe('+ move()', () => {
fs.readFile(dest, 'utf8', (err, contents) => {
const expected = /^sonic the hedgehog\r?\n$/
assert.ifError(err)
assert.ok(contents.match(expected), `${contents} match ${expected}`)
assert.ok(contents.match(expected))
done()
})
})
Expand Down
7 changes: 7 additions & 0 deletions lib/move/move.js
View file
Open in desktop
Expand Up @@ -21,6 +21,7 @@ function move (src, dest, opts, cb) {
const { srcStat, isChangingCase = false } = stats
stat.checkParentPaths(src, srcStat, dest, 'move', err => {
if (err) return cb(err)
if (isParentRoot(dest)) return doRename(src, dest, overwrite, isChangingCase, cb)
mkdirp(path.dirname(dest), err => {
if (err) return cb(err)
return doRename(src, dest, overwrite, isChangingCase, cb)
Expand All @@ -29,6 +30,12 @@ function move (src, dest, opts, cb) {
})
}

function isParentRoot (dest) {
const parent = path.dirname(dest)
const parsedPath = path.parse(parent)
return parsedPath.root === parent
}

function doRename (src, dest, overwrite, isChangingCase, cb) {
if (isChangingCase) return rename(src, dest, overwrite, cb)
if (overwrite) {
Expand Down