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: support changing case in case-insensitive systems #801

Closed
wants to merge 13 commits into from
Closed

move: support changing case in case-insensitive systems #801

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
50e590e
move: support changing case in case-insensitive systems
manidlou May 16, 2020
f86c099
Do not check errors with `instanceof` (#805)
federicobozzini Jun 3, 2020
7b12b05
9.0.1
RyanZim Jun 3, 2020
96facaa
Docs: add fse-cli to the README (#815)
atao60 Jun 17, 2020
cdfe6e8
check changing case before checking overwrite
manidlou Jun 21, 2020
6bffcd8
Upgrade universalify (#825)
RyanZim Jul 29, 2020
d409cf8
Add promise support for fs.rm() (#860)
RyanZim Jan 19, 2021
76d38fc
tests: fix birthtime not available on tmpfs on Linux (#861)
ChALkeR Jan 19, 2021
1625838
9.1.0
RyanZim Jan 19, 2021
59b2658
do not check for system case sensitivity
manidlou Feb 15, 2021
b37f88b
update test descriptions
manidlou Feb 16, 2021
9a0ef93
Merge branch 'master' into v10
manidlou Feb 17, 2021
e21e8b7
Merge branch 'v10' into mani/move-case-insensitive
manidlou Feb 17, 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
81 changes: 16 additions & 65 deletions lib/move-sync/__tests__/move-sync-case-insensitive-paths.test.js
View file
Open in desktop
Expand Up @@ -4,7 +4,6 @@ const assert = require('assert')
const os = require('os')
const path = require('path')
const fs = require('../../')
const platform = os.platform()

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

Expand All @@ -25,22 +24,10 @@ describe('+ moveSync() - case insensitive paths', () => {
src = path.join(TEST_DIR, 'srcdir')
fs.outputFileSync(path.join(src, 'subdir', 'file.txt'), 'some data')
dest = path.join(TEST_DIR, 'srcDir')
let errThrown = false

try {
fs.moveSync(src, dest)
} catch (err) {
if (platform === 'darwin' || platform === 'win32') {
assert.strictEqual(err.message, 'Source and destination must not be the same.')
errThrown = true
}
}
if (platform === 'darwin' || platform === 'win32') assert(errThrown)
if (platform === 'linux') {
assert(fs.existsSync(dest))
assert.strictEqual(fs.readFileSync(path.join(dest, 'subdir', 'file.txt'), 'utf8'), 'some data')
assert(!errThrown)
}
fs.moveSync(src, dest)
assert(fs.existsSync(dest))
assert.strictEqual(fs.readFileSync(path.join(dest, 'subdir', 'file.txt'), 'utf8'), 'some data')
})
})

Expand All @@ -49,22 +36,10 @@ describe('+ moveSync() - case insensitive paths', () => {
src = path.join(TEST_DIR, 'srcfile')
fs.outputFileSync(src, 'some data')
dest = path.join(TEST_DIR, 'srcFile')
let errThrown = false

try {
fs.moveSync(src, dest)
} catch (err) {
if (platform === 'darwin' || platform === 'win32') {
assert.strictEqual(err.message, 'Source and destination must not be the same.')
errThrown = true
}
}
if (platform === 'darwin' || platform === 'win32') assert(errThrown)
if (platform === 'linux') {
assert(fs.existsSync(dest))
assert.strictEqual(fs.readFileSync(dest, 'utf8'), 'some data')
assert(!errThrown)
}
fs.moveSync(src, dest)
assert(fs.existsSync(dest))
assert.strictEqual(fs.readFileSync(dest, 'utf8'), 'some data')
})
})

Expand All @@ -75,24 +50,12 @@ describe('+ moveSync() - case insensitive paths', () => {
const srcLink = path.join(TEST_DIR, 'src-symlink')
fs.symlinkSync(src, srcLink, 'dir')
dest = path.join(TEST_DIR, 'src-Symlink')
let errThrown = false

try {
fs.moveSync(srcLink, dest)
} catch (err) {
if (platform === 'darwin' || platform === 'win32') {
assert.strictEqual(err.message, 'Source and destination must not be the same.')
errThrown = true
}
}
if (platform === 'darwin' || platform === 'win32') assert(errThrown)
if (platform === 'linux') {
assert(fs.existsSync(dest))
assert.strictEqual(fs.readFileSync(path.join(dest, 'subdir', 'file.txt'), 'utf8'), 'some data')
const destLink = fs.readlinkSync(dest)
assert.strictEqual(destLink, src)
assert(!errThrown)
}
fs.moveSync(srcLink, dest)
assert(fs.existsSync(dest))
assert.strictEqual(fs.readFileSync(path.join(dest, 'subdir', 'file.txt'), 'utf8'), 'some data')
const destLink = fs.readlinkSync(dest)
assert.strictEqual(destLink, src)
})

it('should behave correctly based on the OS, symlink file', () => {
Expand All @@ -101,24 +64,12 @@ describe('+ moveSync() - case insensitive paths', () => {
const srcLink = path.join(TEST_DIR, 'src-symlink')
fs.symlinkSync(src, srcLink, 'file')
dest = path.join(TEST_DIR, 'src-Symlink')
let errThrown = false

try {
fs.moveSync(srcLink, dest)
} catch (err) {
if (platform === 'darwin' || platform === 'win32') {
assert.strictEqual(err.message, 'Source and destination must not be the same.')
errThrown = true
}
}
if (platform === 'darwin' || platform === 'win32') assert(errThrown)
if (platform === 'linux') {
assert(fs.existsSync(dest))
assert.strictEqual(fs.readFileSync(dest, 'utf8'), 'some data')
const destLink = fs.readlinkSync(dest)
assert.strictEqual(destLink, src)
assert(!errThrown)
}
fs.moveSync(srcLink, dest)
assert(fs.existsSync(dest))
assert.strictEqual(fs.readFileSync(dest, 'utf8'), 'some data')
const destLink = fs.readlinkSync(dest)
assert.strictEqual(destLink, src)
})
})
})
7 changes: 4 additions & 3 deletions lib/move-sync/move-sync.js
View file
Open in desktop
Expand Up @@ -11,17 +11,18 @@ function moveSync (src, dest, opts) {
opts = opts || {}
const overwrite = opts.overwrite || opts.clobber || false

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

function doRename (src, dest, overwrite) {
function doRename (src, dest, overwrite, isChangingCase) {
if (overwrite) {
removeSync(dest)
return rename(src, dest, overwrite)
}
if (isChangingCase) return rename(src, dest, overwrite)
if (fs.existsSync(dest)) throw new Error('dest already exists.')
return rename(src, dest, overwrite)
}
Expand Down
53 changes: 16 additions & 37 deletions lib/move/__tests__/move-case-insensitive-paths.test.js
View file
Open in desktop
Expand Up @@ -4,7 +4,6 @@ const assert = require('assert')
const os = require('os')
const path = require('path')
const fs = require('../../')
const platform = os.platform()

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

Expand All @@ -27,14 +26,9 @@ describe('+ move() - case insensitive paths', () => {
dest = path.join(TEST_DIR, 'srcDir')

fs.move(src, dest, err => {
if (platform === 'linux') {
assert.ifError(err)
assert(fs.existsSync(dest))
assert.strictEqual(fs.readFileSync(path.join(dest, 'subdir', 'file.txt'), 'utf8'), 'some data')
}
if (platform === 'darwin' || platform === 'win32') {
assert.strictEqual(err.message, 'Source and destination must not be the same.')
}
assert.ifError(err)
assert(fs.existsSync(dest))
assert.strictEqual(fs.readFileSync(path.join(dest, 'subdir', 'file.txt'), 'utf8'), 'some data')
done()
})
})
Expand All @@ -47,14 +41,9 @@ describe('+ move() - case insensitive paths', () => {
dest = path.join(TEST_DIR, 'srcFile')

fs.move(src, dest, err => {
if (platform === 'linux') {
assert.ifError(err)
assert(fs.existsSync(dest))
assert.strictEqual(fs.readFileSync(dest, 'utf8'), 'some data')
}
if (platform === 'darwin' || platform === 'win32') {
assert.strictEqual(err.message, 'Source and destination must not be the same.')
}
assert.ifError(err)
assert(fs.existsSync(dest))
assert.strictEqual(fs.readFileSync(dest, 'utf8'), 'some data')
done()
})
})
Expand All @@ -69,16 +58,11 @@ describe('+ move() - case insensitive paths', () => {
dest = path.join(TEST_DIR, 'src-Symlink')

fs.move(srcLink, dest, err => {
if (platform === 'linux') {
assert.ifError(err)
assert(fs.existsSync(dest))
assert.strictEqual(fs.readFileSync(path.join(dest, 'subdir', 'file.txt'), 'utf8'), 'some data')
const destLink = fs.readlinkSync(dest)
assert.strictEqual(destLink, src)
}
if (platform === 'darwin' || platform === 'win32') {
assert.strictEqual(err.message, 'Source and destination must not be the same.')
}
assert.ifError(err)
assert(fs.existsSync(dest))
assert.strictEqual(fs.readFileSync(path.join(dest, 'subdir', 'file.txt'), 'utf8'), 'some data')
const destLink = fs.readlinkSync(dest)
assert.strictEqual(destLink, src)
done()
})
})
Expand All @@ -91,16 +75,11 @@ describe('+ move() - case insensitive paths', () => {
dest = path.join(TEST_DIR, 'src-Symlink')

fs.move(srcLink, dest, err => {
if (platform === 'linux') {
assert.ifError(err)
assert(fs.existsSync(dest))
assert.strictEqual(fs.readFileSync(dest, 'utf8'), 'some data')
const destLink = fs.readlinkSync(dest)
assert.strictEqual(destLink, src)
}
if (platform === 'darwin' || platform === 'win32') {
assert.strictEqual(err.message, 'Source and destination must not be the same.')
}
assert.ifError(err)
assert(fs.existsSync(dest))
assert.strictEqual(fs.readFileSync(dest, 'utf8'), 'some data')
const destLink = fs.readlinkSync(dest)
assert.strictEqual(destLink, src)
done()
})
})
Expand Down
7 changes: 4 additions & 3 deletions lib/move/move.js
View file
Open in desktop
Expand Up @@ -18,24 +18,25 @@ function move (src, dest, opts, cb) {

stat.checkPaths(src, dest, 'move', opts, (err, stats) => {
if (err) return cb(err)
const { srcStat } = stats
const { srcStat, isChangingCase = false } = stats
stat.checkParentPaths(src, srcStat, dest, 'move', err => {
if (err) return cb(err)
mkdirp(path.dirname(dest), err => {
if (err) return cb(err)
return doRename(src, dest, overwrite, cb)
return doRename(src, dest, overwrite, isChangingCase, cb)
})
})
})
}

function doRename (src, dest, overwrite, cb) {
function doRename (src, dest, overwrite, isChangingCase, cb) {
if (overwrite) {
return remove(dest, err => {
manidlou marked this conversation as resolved.
Show resolved Hide resolved
if (err) return cb(err)
return rename(src, dest, overwrite, cb)
})
}
if (isChangingCase) return rename(src, dest, overwrite, cb)
pathExists(dest, (err, destExists) => {
if (err) return cb(err)
if (destExists) return cb(new Error('dest already exists.'))
Expand Down
18 changes: 18 additions & 0 deletions lib/util/stat.js
View file
Open in desktop
Expand Up @@ -11,6 +11,8 @@ const lstat = (file) => nodeSupportsBigInt ? fs.lstat(file, { bigint: true }) :
const statSync = (file) => nodeSupportsBigInt ? fs.statSync(file, { bigint: true }) : fs.statSync(file)
const lstatSync = (file) => nodeSupportsBigInt ? fs.lstatSync(file, { bigint: true }) : fs.lstatSync(file)

const isCaseInsensitiveSystem = process.platform === 'darwin' || process.platform === 'win32'
Copy link

Choose a reason for hiding this comment

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

Linux also has case-insensitive filesystems, and it can network mount CIFS which is also case-insensitive. So the only way to test this is to see if you can access a file on the filesystem you want to work with via another casing that doesn't exist yet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree that it doesn't include all case-insensitive file systems. Basically a better approach would be to actually probe the file system to find out about its case-sensitivity. It might have some performance drawback but ultimately that would be a better approach.

Copy link

Choose a reason for hiding this comment

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

But wouldn't you have to do that for every directory you access?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If directories are mounted on different file systems, yes! Basically, finding case-sensitivity of a file system is a complex thing! If we want to be 100% safe, we need to check where the directories are mounted, and also if there is any subsystem that has its own case comparison table.

Copy link

Choose a reason for hiding this comment

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

So wouldn't it be easier to just rename the file and pass any errors to the user instead?


function getStats (src, dest, opts) {
const statFunc = opts.dereference ? stat : lstat
return Promise.all([
Expand Down Expand Up @@ -42,6 +44,14 @@ function checkPaths (src, dest, funcName, opts, cb) {

if (destStat) {
if (areIdentical(srcStat, destStat)) {
const srcBaseName = path.basename(src)
const destBaseName = path.basename(dest)
if (funcName === 'move' &&
isCaseInsensitiveSystem &&
Copy link

Choose a reason for hiding this comment

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

perhaps better to simply not test for this. If the stat() result is the same, it's the same file.

Copy link

Choose a reason for hiding this comment

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

... And in that case, just call rename(). I can't think of a case where it would fail

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That makes sense! I am ok with not checking for case sensitivity of the system as this is not comprehensive enough!

srcBaseName !== destBaseName &&
srcBaseName.toLowerCase() === destBaseName.toLowerCase()) {
return cb(null, { srcStat, destStat, isChangingCase: true })
}
return cb(new Error('Source and destination must not be the same.'))
}
if (srcStat.isDirectory() && !destStat.isDirectory()) {
Expand All @@ -64,6 +74,14 @@ function checkPathsSync (src, dest, funcName, opts) {

if (destStat) {
if (areIdentical(srcStat, destStat)) {
const srcBaseName = path.basename(src)
const destBaseName = path.basename(dest)
if (funcName === 'move' &&
isCaseInsensitiveSystem &&
srcBaseName !== destBaseName &&
srcBaseName.toLowerCase() === destBaseName.toLowerCase()) {
return { srcStat, destStat, isChangingCase: true }
}
throw new Error('Source and destination must not be the same.')
}
if (srcStat.isDirectory() && !destStat.isDirectory()) {
Expand Down