Inconsistent behavior of path.basename(path, ext)

[ChALkeR]

I believe this was introduced somewhere in https://github.com/nodejs/node/pull/5123, which changed the behavior of the ext argument.

This is observed on all supported branches and was even recently backported to 4.x.

Documentation: https://nodejs.org/api/path.html#path_path_basename_path_ext

Observe the input and try to predict the output:

> ['a', 'a/', 'a//'].map(x => path.posix.basename(x))
[ 'a', 'a', 'a' ]
> ['a', 'a/', 'a//'].map(x => path.posix.basename(x,'b'))
[ 'a', 'a', 'a' ]
> ['a', 'a/', 'a//'].map(x => path.posix.basename(x,'a'))
[ '', 'a', 'a' ]
> ['a', 'a/', 'a//'].map(x => path.posix.basename(x,'a/'))
[ 'a', '', 'a' ]
> ['a', 'a/', 'a//'].map(x => path.posix.basename(x,'a//'))
[ 'a', 'a', '' ]
> ['a', 'a/', 'a//'].map(x => path.posix.basename(x,'aa'))
[ 'a', 'a/', 'a//' ]
> ['a', 'a/', 'a//'].map(x => path.posix.basename(x,'bb'))
[ 'a', 'a', 'a' ]
> ['a', 'a/', 'a//'].map(x => path.posix.basename(x,'aaa'))
[ 'a', 'a', 'a//' ]
> ['a', 'a/', 'a//'].map(x => path.posix.basename(x,'aaaa'))
[ 'a', 'a', 'a' ]
> ['dd', '/dd', 'd/dd', 'd/dd/'].map(x => path.posix.basename(x))
[ 'dd', 'dd', 'dd', 'dd' ]
> ['dd', '/dd', 'd/dd', 'd/dd/'].map(x => path.posix.basename(x, 'd'))
[ 'd', 'd', 'd', 'd' ]
> ['dd', '/dd', 'd/dd', 'd/dd/'].map(x => path.posix.basename(x, 'dd'))
[ '', 'dd', 'dd', 'dd' ]
> ['dd', '/dd', 'd/dd', 'd/dd/'].map(x => path.posix.basename(x, 'ddd'))
[ 'dd', 'dd', 'dd', 'dd/' ]

There are more, but all the inconsistencies with the previous behavior involve at least one of those:

1. Either the path ends with /,
2. Or ext includes /,
3. Or ext equals to the actual resolved basename (i.e. path.endsWith('/' + ext)).

More specifically, the following check covers all the cases inconsistent behavior to my knowledge: path.endsWith('/') || ext.includes('/') || path.endsWith('/' + ext) (note that it also includes cases of consistent behavior).

---

Reminder: before #5123, this was how ext behave:

if (ext && f.substr(-1 * ext.length) === ext) {
    f = f.substr(0, f.length - ext.length);
}

I.e. it just sliced off the suffix (after doing everything else).

[ChALkeR]

Note: the above don't come from some actual usecase that is broken by this, I used a bruteforce script that compares the old impl and the new impl outputs to find this.

[Trott]

My inclination (without thinking about it too much and possibly being ignorant of some history here) would be to compare the results to that of the basename CLI tool and consider any differences a bug. (Of course, that assumes that there aren't significant differences in basename implementation on different POSIX platforms and whatnot.)

[ChALkeR]

@Trott In #7519, it was decided that path.basename shouldn't have the exact same behavior as basename(1).

Also, the cases metioned here are unlikely to be met in the «good» code path and use-case, and I don't remember any reports about cases of actual breakage due to #5123 which (as I believe) initially introduced the inconsistencies metioned here.

Converting to basename(1), on the other hand, to my estimation would be a significant breaking change as it might affect widely used cases. E.g. basename('/') output differs from what basename / gives.

/cc @bnoordhuis

[ChALkeR]

/cc @mscdex @MylesBorins

[ChALkeR]

/cc @nodejs/collaborators, thoughts?

[ryzokuken]

@ChALkeR any updates?

[ChALkeR]

@ryzokuken I didn't get any other comments to this, I think. I'm still waiting a reply whether there is some rationale behind this change.