Repository navigation
path.basename matches ext parameter case-sensitively on case-insensitive filesystems #13727
Description
- Version: v8.1.2
- Platform: 64-bit Windows 10
- Subsystem: path
On systems with case-insensitive filesystems, path.basename nevertheless matches its ext parameter case-sensitively, which enables subtle bugs like istanbuljs/spawn-wrap#56 and is inconsistent with the behavior of other APIs like fs.existsSync that observe the case-sensitivity of the filesystem:
> process.execPath
'c:\\Program Files\\nodejs\\node.exe'
> process.execPath.toUpperCase()
'C:\\PROGRAM FILES\\NODEJS\\NODE.EXE'
> fs.existsSync(process.execPath)
true
> fs.existsSync(process.execPath.toUpperCase())
true
> path.basename(process.execPath, '.exe')
'node'
> path.basename(process.execPath.toUpperCase(), '.exe')
'NODE.EXE'
path.basename's behavior is, however, consistent with the basename program on the Unix-like systems I've tested, which also matches an extension case-sensitively, even on a case-insensitive filesystem (like my macOS system). Nevertheless, it feels like a footgun, as some developers will expect the behavior to be consistent across the fs and path APIs.
By comparison:
- Perl's equivalent matches the extension case-insensitively on case-insensitive filesystems: http://perldoc.perl.org/File/Basename.html
- Ruby's equivalent, on the other hand, appears to match case-sensitively on all platforms: https://stackoverflow.com/questions/15494565/is-there-a-file-basename-with-case-insensitive-suffix
- Python's equivalent doesn't even offer the option to strip an extension: https://docs.python.org/2/library/os.path.html#os.path.basename
- The POSIX specification for basename also doesn't offer the option to strip an extension: http://pubs.opengroup.org/onlinepubs/009695399/functions/basename.html
Note that 7c731ec added the comment "// TODO: make this comparison case-insensitive on windows? " to path.basename back in 2011. Then #5123 removed it as part of a broader rewrite (although there's no evidence that the latter change was intended to resolve the TODO one way or the other).
I could argue this either way, so I would understand intentionally not fixing it. But it does seem like a footgun, given the aforementioned bug (for which I've submitted a fix that does the extension stripping in a separate String.replace operation). At the very least, I thought it worth an issue, so even if you decide not to change the behavior, that decision will be documented somewhere.
Activity
addaleax commentedon Jun 16, 2017
One thing worth mentioning: The
pathmodule doesn’t actually do any file system operations (which matters, because that means it also works on non-existent paths), so it won’t know whether the file system is actually case-sensitive or not. That means that thepath.posixfunctions will likely keep their current behaviour, even if you can of course have case-insensitive filesystems on POSIX.But I agree, it would seem reasonable to make this change on Windows.
mscdex commentedon Jun 16, 2017
Also, a case-insensitive check (e.g. the need to call
str.toLowerCase()twice) will cause a considerable decrease in performance.refack commentedon Jun 16, 2017
Micro benchmark:
Output:
But since the code actually does char by char
ext.charCodeAt(extIdx)comparison an ASCII only optimization is possible,mscdex commentedon Jun 16, 2017
@refack You would also need to
.toLowerCase()the extension part ofname, not justext.Additionally, we can't assume ASCII on Windows/MacOS (and I'm not sure checking for and bailing on non-ASCII characters would avoid a regression).
refack commentedon Jun 16, 2017
Yep.
But it's a bug I had to workaround several times (case sensitive ext).
Might be worth the trade off, or add an opt-in third argument
basename(path, ext, caseSensitive)mscdex commentedon Jun 16, 2017
Also, it's technically possible (from what I briefly read) to make Windows case-sensitive (with regard to filesystem operations), although I don't know how common that is (NTFS itself already supports case sensitivity).
refack commentedon Jun 16, 2017
That is true 🤔 (NTFS is case sensitive, it's the kernel that isn't, Ref: https://technet.microsoft.com/en-us/library/cc725747(v=ws.11).aspx)
So an opt-in argument is probably the best solution... Mostly because it will explicitly document this non-intuitive behaviour.
Trott commentedon May 31, 2018
This looks (to me, at least) like something we're not likely to change. Thanks for the issue. You're totally right that this is worth being intentional about. I think this should be closed, but if anyone disagrees, please re-open (or leave a comment if GitHub doesn't let you re-open it).
Trott commentedon May 31, 2018
(By the way, if we do add a third argument, I'd prefer it be an
optionsobject so that we don't have to add a fourth or fifth argument later.)Trott commentedon May 31, 2018
(PR welcome on that third argument, although no guarantee it won't be met with "this should be done in userland". I would be OK with it, though.)