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

lib: fix basename comparison on windows #4669

Closed
wants to merge 1 commit into from
Closed

lib: fix basename comparison on windows #4669

wants to merge 1 commit into from

Conversation

tflanagan
Copy link
Contributor

Fixes TODO in lib/path.js ref'd here #4642

 var f = win32SplitPath(path)[2];
  // TODO: make this comparison case-insensitive on windows?
  if (ext && f.substr(-1 * ext.length) === ext) {
    f = f.substr(0, f.length - ext.length);

@mscdex mscdex added the path Issues and PRs related to the path subsystem. label Jan 13, 2016
@mscdex
Copy link
Contributor

mscdex commented Jan 13, 2016

This change assumes that ext is lowercase, which may not necessarily be true.

@mscdex
Copy link
Contributor

mscdex commented Jan 13, 2016

Also, I'm not sure that the isWindows check is necessary since it's already supposed to be a win32.* method.

@tflanagan tflanagan force-pushed the path-win-fix-comparison branch from 9a720a4 to 4febedf Compare January 13, 2016 15:20
@tflanagan
Copy link
Contributor Author

Thanks @mscdex, I've address those comments

mscdex
mscdex reviewed Jan 13, 2016
View reviewed changes
test/parallel/test-path.js Outdated
@@ -406,6 +406,15 @@ assert.equal(path.win32.delimiter, ';');
// posix
assert.equal(path.posix.delimiter, ':');

// ensure ext comparison is case-insensitive on windows
if (common.isWindows) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this conditional can be dropped as well, since path.win32 would be available on all platforms and is the same implementations that would be used on real Windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @mscdex, I've changed this

@tflanagan tflanagan force-pushed the path-win-fix-comparison branch from 4febedf to b62b2b0 Compare January 13, 2016 15:31
cjihrig
cjihrig reviewed Jan 13, 2016
View reviewed changes
lib/path.js
@@ -340,11 +340,20 @@ win32.basename = function(path, ext) {
if (ext !== undefined && typeof ext !== 'string')
throw new TypeError('"ext" argument must be a string');

var f = win32SplitPath(path)[2];
// TODO: make this comparison case-insensitive on windows?
if (ext && f.substr(-1 * ext.length) === ext) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would if (ext && f.substr(-1 * ext.length).toLowerCase() === ext.toLowerCase()) { have been enough to address the original comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would, but you were concerned with readability in a previous comment, that would work, but would kill readability

Copy link
Contributor

Choose a reason for hiding this comment

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

For readability, it could be: if (ext && f.toLowerCase().endsWith(ext.toLowerCase())) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is much more elegant and witty.

I'll push a fix soon, thanks @cjihrig.

@tflanagan tflanagan force-pushed the path-win-fix-comparison branch from b62b2b0 to 1d05b89 Compare January 13, 2016 16:22
@tflanagan
Copy link
Contributor Author

/poke

Would this be a semver-minor change?

@bnoordhuis
Copy link
Member

It's a backwards incompatible change so semver-major, I'll add the tag.

Apropos .toLowerCase(), it has a possible gotcha in that it may not correctly lower-case some Unicode characters.

It's currently implemented using the unibrow library but I know that the V8 team has plans to swap it out for ICU because unibrow lags behind the Unicode spec. To wit, V8's test suite has a number of regression tests for .toLowerCase() that are marked FAIL.

In other words, this may not be an entirely risk-free change.

@bnoordhuis bnoordhuis added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 25, 2016
@tflanagan
Copy link
Contributor Author

/poke

@jasnell
Copy link
Member

jasnell commented Feb 19, 2016

/cc @bnoordhuis @mscdex @orangemocha ... any thoughts on this one?

@orangemocha
Copy link
Contributor

No objections to doing case-insensitive matching, but this function has undergone substantial changes (and the todo was removed) with b212be0. Could you please rework your changes over the current master branch?

@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

@tflanagan .. ping

@estliberitas estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:22
@MylesBorins
Copy link
Contributor

@tflanagan ping

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Aug 9, 2016
@Trott
Copy link
Member

Trott commented Sep 16, 2016

@tflanagan Same question here: Any chance you can do a rebase? If not, I or someone else can try to take it on, but it would be awesome to get this landed and it would be great to see you back in action. (But I totally get it if you're busy with other stuff or your interests have gone elsewhere!)

@rvagg rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:01
@jasnell
Copy link
Member

jasnell commented Feb 28, 2017

Closing given the lack of forward progress on this.

@jasnell jasnell closed this Feb 28, 2017
@gibfahn
Copy link
Member

gibfahn commented Mar 2, 2017

For posterity, I think this was fixed as part of @mscdex's rewrite in #5123

b212be0#diff-c0fae59301ef49fb71b43278db9ad881L344

@gibfahn gibfahn removed the stalled Issues and PRs that are stalled. label Mar 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
path Issues and PRs related to the path subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@tflanagan @mscdex @bnoordhuis @jasnell @orangemocha @MylesBorins @Trott @gibfahn @cjihrig