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

path: fix win32.isAbsolute() #6028

Closed
wants to merge 1 commit into from
Closed

path: fix win32.isAbsolute() #6028

wants to merge 1 commit into from

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Apr 3, 2016

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?

Affected core subsystem(s)

  • path

Description of change

Fixes: #6027

@mscdex mscdex added windows Issues and PRs related to the Windows platform. path Issues and PRs related to the path subsystem. labels Apr 3, 2016
@mscdex
Copy link
Contributor Author

mscdex commented Apr 3, 2016

CI: https://ci.nodejs.org/job/node-test-pull-request/2141/

@benjamingr
Copy link
Member

Here are some relevant unit tests for checking if a path is absolute in win32

https://github.com/pmfsampaio/NETMF-LPC/blob/master/MicroFrameworkPK_v4_2/Test/Platform/Tests/CLR/System/IO/Path/IsPathRooted.cs

We should port some/all of them

benjamingr
benjamingr reviewed Apr 3, 2016
View reviewed changes
lib/path.js
}
}
} else if (code === 47/*/*/ || code === 92/*\*/) {
if (code === 47/*/*/ || code === 92/*\*/) {
Copy link
Member

Choose a reason for hiding this comment

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

This code is much simpler than the old code - any idea why it was so long and what purpose the checks served?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't recall offhand, probably just a misinterpretation of the old regexp.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, the new version is correct on its own - the old one is just weird.

@mscdex
Copy link
Contributor Author

mscdex commented Apr 3, 2016

Here are some relevant unit tests for checking if a path is absolute in win32

https://github.com/pmfsampaio/NETMF-LPC/blob/master/MicroFrameworkPK_v4_2/Test/Platform/Tests/CLR/System/IO/Path/IsPathRooted.cs

We should port some/all of them

@benjamingr We already cover those more or less, especially so with the test case additions provided by this PR.

@benjamingr
Copy link
Member

Any particular reason the code between resolve and normalize is so duplicated?

Unrelated to this PR, but I want to know if there is a specific reason to it.

LGTM + asked for some clarifications.

@mscdex
Copy link
Contributor Author

mscdex commented Apr 3, 2016

@benjamingr I don't recall if it was performance reasons or what. If someone wants to try factoring that out and checking the performance differences, go for it :-). However I suspect that a helper function containing that factored out portion wouldn't be inlined (if for no reason other than code size), which would cause a performance regression to some degree.

@jasnell
Copy link
Member

jasnell commented Apr 4, 2016

@mscdex ... can you add a bit more explanation to the commit log? Otherwise LGTM

@mscdex
path: fix win32.isAbsolute() inconsistency
45a960e 
This commit fixes an inconsistency in absolute path checking compared
to the absolute path detection used by the other path.win32 functions.

Fixes: nodejs#6027
PR-URL: nodejs#6028
@mscdex mscdex force-pushed the path-fix-win32-isabsolute branch from d35449c to 45a960e Compare April 4, 2016 01:41
@mscdex
Copy link
Contributor Author

mscdex commented Apr 4, 2016

@jasnell Updated

@jasnell
Copy link
Member

jasnell commented Apr 4, 2016

Thanks! LGTM!

jasnell pushed a commit that referenced this pull request Apr 4, 2016
@mscdex @jasnell
path: fix win32.isAbsolute() inconsistency
3072546 
This commit fixes an inconsistency in absolute path checking compared
to the absolute path detection used by the other path.win32 functions.

Fixes: #6027
PR-URL: #6028
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Apr 4, 2016

Landed in 3072546

@jasnell jasnell closed this Apr 4, 2016
@MylesBorins MylesBorins mentioned this pull request Apr 4, 2016
Closed
@MylesBorins
Copy link
Contributor

@mscdex @jasnell should we back port the test?

edit: it currently passes fwiw

@mscdex
Copy link
Contributor Author

mscdex commented Apr 4, 2016

@thealphanerd AFAIK backporting the tests should be fine

@jasnell
Copy link
Member

jasnell commented Apr 4, 2016

+1 to backporting the tests

@mscdex mscdex deleted the path-fix-win32-isabsolute branch April 4, 2016 21:56
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Apr 4, 2016
@mscdex
test: ensure win32.isAbsolute() is consistent
809f930 
Adds test cases to ensure win32.isAbsolute is consistent.

This is a backport from 3072546

ref: nodejs#6028
@MylesBorins MylesBorins mentioned this pull request Apr 4, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Apr 5, 2016
@mscdex
path: fix win32.isAbsolute() inconsistency
1c40079 
This commit fixes an inconsistency in absolute path checking compared
to the absolute path detection used by the other path.win32 functions.

Fixes: #6027
PR-URL: #6028
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 5, 2016
2016-04-05, Version 5.10.1 (Stable) Release
6888706 
Notable changes:

http:
  * Enclose IPv6 Host header in square brackets. This will enable
  proper seperation of the host adress from any port reference
  (Mihai Potra) #5314
path:
  * Make win32.isAbsolute more consistent (Brian White)
  #6028
This was referenced Apr 5, 2016
Closed
Merged
MylesBorins pushed a commit that referenced this pull request Apr 5, 2016
2016-04-05, Version 5.10.1 (Stable) Release
5c4a414 
Notable changes:

http:
  * Enclose IPv6 Host header in square brackets. This will enable
  proper seperation of the host adress from any port reference
  (Mihai Potra) #5314

path:
  * Make win32.isAbsolute more consistent (Brian White)
  #6028

PR-URL: #6060
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 5, 2016
2016-04-05, Version 5.10.1 (Stable) Release
82d57d3 
Notable changes:

http:
  * Enclose IPv6 Host header in square brackets. This will enable
  proper seperation of the host adress from any port reference
  (Mihai Potra) #5314

path:
  * Make win32.isAbsolute more consistent (Brian White)
  #6028

PR-URL: #6060
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
jasnell pushed a commit that referenced this pull request Apr 8, 2016
@mscdex @jasnell
test: ensure win32.isAbsolute() is consistent
ea83c38 
Adds test cases to ensure win32.isAbsolute is consistent.

This is a backport from 3072546

Refs: #6028
PR-URL: #6043
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Roman Reiss <me@silverwind.io>
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. windows Issues and PRs related to the Windows platform.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

path.isAbsolute wrong output
4 participants
@mscdex @benjamingr @jasnell @MylesBorins