Conversation
mscdex
added
windows
|
Here are some relevant unit tests for checking if a path is absolute in win32 We should port some/all of them |
| } | ||
| } | ||
| } else if (code === 47/*/*/ || code === 92/*\*/) { | ||
| if (code === 47/*/*/ || code === 92/*\*/) { |
There was a problem hiding this comment.
This code is much simpler than the old code - any idea why it was so long and what purpose the checks served?
There was a problem hiding this comment.
I don't recall offhand, probably just a misinterpretation of the old regexp.
There was a problem hiding this comment.
Ok, the new version is correct on its own - the old one is just weird.
@benjamingr We already cover those more or less, especially so with the test case additions provided by this PR. |
|
@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. |
|
@mscdex ... can you add a bit more explanation to the commit log? Otherwise LGTM |
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
|
@jasnell Updated |
|
Thanks! LGTM! |
|
Landed in 3072546 |
|
@thealphanerd AFAIK backporting the tests should be fine |
|
+1 to backporting the tests |
Adds test cases to ensure win32.isAbsolute is consistent. This is a backport from 3072546 ref: nodejs#6028
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>
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>
4 participants
Pull Request check-list
make -j8 test(UNIX) orvcbuild test nosign(Windows) pass withthis change (including linting)?
test (or a benchmark) included?
Affected core subsystem(s)
Description of change
Fixes: #6027