path.basename matches `ext` parameter case-sensitively on case-insensitive filesystems

[mykmelez]

• 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.

[addaleax]

One thing worth mentioning: The path module 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 the path.posix functions 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]

Also, a case-insensitive check (e.g. the need to call str.toLowerCase() twice) will cause a considerable decrease in performance.

[refack]

But I agree, it would seem reasonable to make this change on Windows.

Also, a case-insensitive check (e.g. the need to call str.toLowerCase()) will cause a considerable decrease in performance.

Micro benchmark:

const ext = '.ps1';
const name = 'D:\\code\\node\\vcbuild.ps1';

console.time('without');
for (let i = 0; i < 1E8; ++i) {
  basename(name, ext);
}
console.timeEnd('without');

console.time('with');
for (let i = 0; i < 1E8; ++i) {
  basename(name, ext.toLowerCase());
}
console.timeEnd('with');

Output:

without: 16264.727ms
with: 21017.397ms

But since the code actually does char by char ext.charCodeAt(extIdx) comparison an ASCII only optimization is possible,

[mscdex]

@refack You would also need to .toLowerCase() the extension part of name, not just ext.

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]

@refack You would also need to .toLowerCase() the extension part of name, not just ext.

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).

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]

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]

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).

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]

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]

(By the way, if we do add a third argument, I'd prefer it be an options object so that we don't have to add a fourth or fifth argument later.)

[Trott]

(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.)