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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 10 additions & 50 deletions lib/path.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,57 +439,17 @@ const win32 = {
if (len === 0)
return false;
var code = path.charCodeAt(0);
if (len > 1) {
if (code === 47/*/*/ || code === 92/*\*/) {
// Possible UNC root

code = path.charCodeAt(1);
if (code === 47/*/*/ || code === 92/*\*/) {
// Matched double path separator at beginning
var j = 2;
var last = j;
// Match 1 or more non-path separators
for (; j < len; ++j) {
code = path.charCodeAt(j);
if (code === 47/*/*/ || code === 92/*\*/)
break;
}
if (j < len && j !== last) {
// Matched!
last = j;
// Match 1 or more path separators
for (; j < len; ++j) {
code = path.charCodeAt(j);
if (code !== 47/*/*/ && code !== 92/*\*/)
break;
}
if (j < len && j !== last) {
// Matched!
last = j;
// Match 1 or more non-path separators
for (; j < len; ++j) {
code = path.charCodeAt(j);
if (code === 47/*/*/ || code === 92/*\*/)
break;
}
if (j !== last)
return true;
}
}
}
} else if ((code >= 65/*A*/ && code <= 90/*Z*/) ||
(code >= 97/*a*/ && code <= 122/*z*/)) {
// Possible device root

code = path.charCodeAt(1);
if (path.charCodeAt(1) === 58/*:*/ && len > 2) {
code = path.charCodeAt(2);
if (code === 47/*/*/ || code === 92/*\*/)
return true;
}
}
} 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.

return true;
} else if ((code >= 65/*A*/ && code <= 90/*Z*/) ||
Copy link
Member

Choose a reason for hiding this comment

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

Any reason this check is required? It only seems to matter for invalid paths, something like \:\ or weird things. I'm only asking because this deviates from the .net implementation http://referencesource.microsoft.com/#mscorlib/system/io/path.cs,1018

And slightly from the Python one https://github.com/python/cpython/blob/1fe0fd9feb6a4472a9a1b186502eb9c0b2366326/Lib/ntpath.py#L67

Although at this point it's better to keep this way for BC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMHO being more strict is better anyway. I don't see Windows adopting anything other than ASCII alphabetic drive letters any time soon.

Copy link
Member

Choose a reason for hiding this comment

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

Right, and it would be a breaking change anyway. OK, all questions addressed - LGTM.

(code >= 97/*a*/ && code <= 122/*z*/)) {
// Possible device root

if (len > 2 && path.charCodeAt(1) === 58/*:*/) {
code = path.charCodeAt(2);
if (code === 47/*/*/ || code === 92/*\*/)
return true;
}
}
return false;
},
Expand Down
10 changes: 10 additions & 0 deletions test/parallel/test-path.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,8 +442,18 @@ assert.equal(failures.length, 0, failures.join(''));


// path.isAbsolute tests
assert.equal(path.win32.isAbsolute('/'), true);
assert.equal(path.win32.isAbsolute('//'), true);
assert.equal(path.win32.isAbsolute('//server'), true);
assert.equal(path.win32.isAbsolute('//server/file'), true);
assert.equal(path.win32.isAbsolute('\\\\server\\file'), true);
assert.equal(path.win32.isAbsolute('\\\\server'), true);
assert.equal(path.win32.isAbsolute('\\\\'), true);
assert.equal(path.win32.isAbsolute('c'), false);
assert.equal(path.win32.isAbsolute('c:'), false);
assert.equal(path.win32.isAbsolute('c:\\'), true);
assert.equal(path.win32.isAbsolute('c:/'), true);
assert.equal(path.win32.isAbsolute('c://'), true);
assert.equal(path.win32.isAbsolute('C:/Users/'), true);
assert.equal(path.win32.isAbsolute('C:\\Users\\'), true);
assert.equal(path.win32.isAbsolute('C:cwd/another'), false);
Expand Down