move: support changing case in case-insensitive systems #801
Conversation
|
@manidlou Did you verify this actually changes the case displayed in the file manager or on the command line, for Mac or Windows? |
|
@RyanZim that's definitely required and I wanted to do that but I don't have neither of those systems. I appreciate if anyone could verify that on Mac and Windows! Feel free to let me know if the changes don't work and I will work on it then! I also might be able to have access to my friend's macbook tomorrow, so I'll let you know then. |
|
I verified that on Mac and it successfully changed the case. |
This fixes cross-VM type issues in Jest. Backport of isaacs/rimraf@aa50e02
No breaking changes for us, just performance improvements
lib/util/stat.js
Outdated
| @@ -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' | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
But wouldn't you have to do that for every directory you access?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
So wouldn't it be easier to just rename the file and pass any errors to the user instead?
lib/util/stat.js
Outdated
| const srcBaseName = path.basename(src) | ||
| const destBaseName = path.basename(dest) | ||
| if (funcName === 'move' && | ||
| isCaseInsensitiveSystem && |
There was a problem hiding this comment.
perhaps better to simply not test for this. If the stat() result is the same, it's the same file.
There was a problem hiding this comment.
... And in that case, just call rename(). I can't think of a case where it would fail
There was a problem hiding this comment.
That makes sense! I am ok with not checking for case sensitivity of the system as this is not comprehensive enough!
|
@manidlou ping! |
|
Sorry for the long pause! Been extremely busy! I will have a look at this again. |
|
Since this will merge into v10, and I am a bit worried if we wait for long time, we might end up having some merge conflicts! So I merged current master into v10 and rebased this one so that we will have a clean merge! |
|
@manidlou Honestly, I suspect I'll end up manually cherry-picking everything related to v10, since it's taken so long. Just leave this PR as-is for now. |
|
Landed on master in c8bd383. |
Resolves #759.