FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

path: fix win32.isAbsolute() by mscdex · Pull Request #6028 · nodejs/node · GitHub

/ node Public

path: fix win32.isAbsolute() - #6028

Closed
mscdex wants to merge 1 commit into
nodejs:masterfrom
mscdex:path-fix-win32-isabsolute
ConversationCommits (1)ChecksFiles changed
Closed

path: fix win32.isAbsolute()#6028
mscdex wants to merge 1 commit into
nodejs:masterfrom
mscdex:path-fix-win32-isabsolute

Conversation

mscdex commented Apr 3, 2016

Copy link
Copy Markdown
Contributor

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 added windows Issues and PRs related to the Windows platform. path Issues and PRs related to the path subsystem. labels Apr 3, 2016

mscdex commented Apr 3, 2016

Copy link
Copy Markdown
Contributor Author

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

benjamingr commented Apr 3, 2016

Copy link
Copy Markdown
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 reviewed Apr 3, 2016
View reviewed changes
Comment thread lib/path.js
}
}
} else if (code === 47/*/*/ || code === 92/*\*/) {
if (code === 47/*/*/ || code === 92/*\*/) {

benjamingr Apr 3, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mscdex Apr 3, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

benjamingr Apr 3, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mscdex commented Apr 3, 2016

Copy link
Copy Markdown
Contributor Author

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 commented Apr 3, 2016

Copy link
Copy Markdown
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 commented Apr 3, 2016

Copy link
Copy Markdown
Contributor Author

@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 commented Apr 4, 2016

Copy link
Copy Markdown
Member

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

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 force-pushed the path-fix-win32-isabsolute branch from d35449c to 45a960e Compare April 4, 2016 01:41

mscdex commented Apr 4, 2016

Copy link
Copy Markdown
Contributor Author

@jasnell Updated

jasnell commented Apr 4, 2016

Copy link
Copy Markdown
Member

Thanks! LGTM!

jasnell pushed a commit that referenced this pull request Apr 4, 2016
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 commented Apr 4, 2016

Copy link
Copy Markdown
Member

Landed in 3072546

jasnell closed this Apr 4, 2016
MylesBorins mentioned this pull request Apr 4, 2016
Closed

MylesBorins commented Apr 4, 2016

Copy link
Copy Markdown
Contributor

@mscdex @jasnell should we back port the test?

edit: it currently passes fwiw

mscdex commented Apr 4, 2016

Copy link
Copy Markdown
Contributor Author

@thealphanerd AFAIK backporting the tests should be fine

jasnell commented Apr 4, 2016

Copy link
Copy Markdown
Member

+1 to backporting the tests

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
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 mentioned this pull request Apr 4, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Apr 5, 2016
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
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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

Back | FazBrowse Home | New Git URL