Skip to content

path: fix posix.relative returns different results #13738

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

Closed
DuanPengfei wants to merge 5 commits into from
Closed

path: fix posix.relative returns different results #13738

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
466faeb
path: fix posix.relative returns different results
DuanPengfei Jun 17, 2017
0864e8e
fix: typo
DuanPengfei Jun 19, 2017
acfdb47
fix: rm unnecessary test
DuanPengfei Jun 19, 2017
8d604e1
feat: `resolve` only throw error when rely on cwd
DuanPengfei Jun 22, 2017
c8bbf22
doc: update `path.posix` and `path.win32` doc
DuanPengfei Jun 22, 2017
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
Prev Previous commit
Next Next commit
feat: resolve only throw error when rely on cwd
  • Loading branch information
@DuanPengfei
DuanPengfei committed Jun 22, 2017
commit 8d604e19a84c02c570cd70bb67aac8cd3b28f4cd
80 changes: 74 additions & 6 deletions lib/path.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,10 @@ const win32 = {
path = arguments[i];
} else if (!resolvedDevice) {
path = process.cwd();

// If you use the current working directory,
Copy link
Copy Markdown
Member

@jasnell jasnell Jun 26, 2017

Choose a reason for hiding this comment

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

// If the current working directory is used ...

// it is necessary to check whether the current platform support.
assertWindowsPlatform(process.platform);
} else {
// Windows has the concept of drive-specific current working
// directories. If we've resolved a drive letter but not yet an
Expand All @@ -213,11 +217,14 @@ const win32 = {
path.slice(0, 3).toLowerCase() !==
resolvedDevice.toLowerCase() + '\\') {
path = resolvedDevice + '\\';
} else {
// If you use the current working directory,
Copy link
Copy Markdown
Member

@jasnell jasnell Jun 26, 2017

Choose a reason for hiding this comment

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

// The the current working directory is used...

// it is necessary to check whether the current platform support.
assertWindowsPlatform(process.platform);
}
}

assertPath(path);
assertWindowsPlatform(process.platform);

// Skip empty entries
if (path.length === 0) {
Expand Down Expand Up @@ -575,8 +582,38 @@ const win32 = {
if (from === to)
return '';

var fromOrig = win32.resolve(from);
var toOrig = win32.resolve(to);
var fromOrig;
var toOrig;
var isFromAbsolute = true;
var isToAbsolute = true;

try {
fromOrig = win32.resolve(from);
} catch (err) {
if (err.code === 'ERR_UNSUPPORTED_PLATFORM')
isFromAbsolute = false;
}
try {
toOrig = win32.resolve(to);
} catch (err) {
if (err.code === 'ERR_UNSUPPORTED_PLATFORM')
isToAbsolute = false;
}

if (process.platform !== 'win32') {
if (!isFromAbsolute && !isToAbsolute) {
Copy link
Copy Markdown
Contributor

@refack refack Jun 22, 2017

Choose a reason for hiding this comment

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

looks like you can do this separately for to and from

if (isFromAbsolute) ...
else ...
if (isToAbsolute) ...
else ...

Or maybe I'm wrong?

Copy link
Copy Markdown
Contributor Author

@DuanPengfei DuanPengfei Jun 27, 2017

Choose a reason for hiding this comment

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

Because need to know both type of from and to, so I think I can't do this separately.

Copy link
Copy Markdown
Contributor

@refack refack Jun 27, 2017

Choose a reason for hiding this comment

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

Ok 👍

from = 'c:\\fakepath\\' + from;
Copy link
Copy Markdown
Contributor

@refack refack Jun 22, 2017

Choose a reason for hiding this comment

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

store the surrogate in a const (or even a function if it works)

function absoluteWithSurrogate(p) {
  return win32.resolve(win32.join(`c:\\fakepath`, p));
}

Copy link
Copy Markdown
Contributor Author

@DuanPengfei DuanPengfei Jun 23, 2017

Choose a reason for hiding this comment

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

👌

to = 'c:\\fakepath\\' + to;
fromOrig = win32.resolve(from);
toOrig = win32.resolve(to);
} else if (isFromAbsolute && !isToAbsolute) {
to = from + '\\' + to;
toOrig = win32.resolve(to);
} else if (!isFromAbsolute && isToAbsolute) {
from = to + '\\' + from;
fromOrig = win32.resolve(from);
}
}

if (fromOrig === toOrig)
return '';
Expand Down Expand Up @@ -1177,7 +1214,6 @@ const posix = {
}

assertPath(path);
assertPosixPlatform(process.platform);

// Skip empty entries
if (path.length === 0) {
Expand All @@ -1188,6 +1224,11 @@ const posix = {
resolvedAbsolute = path.charCodeAt(0) === 47/*/*/;
}

// If you use the current working directory,
// it is necessary to check whether the current platform support.
if (cwd)
assertPosixPlatform(process.platform);

// At this point the path should be resolved to a full absolute path, but
// handle relative paths to be safe (might happen when process.cwd() fails)

Expand Down Expand Up @@ -1263,8 +1304,35 @@ const posix = {
if (from === to)
return '';

from = posix.resolve(from);
to = posix.resolve(to);
var isFromAbsolute = true;
var isToAbsolute = true;
try {
from = posix.resolve(from);
} catch (err) {
if (err.code === 'ERR_UNSUPPORTED_PLATFORM')
isFromAbsolute = false;
}
try {
to = posix.resolve(to);
} catch (err) {
if (err.code === 'ERR_UNSUPPORTED_PLATFORM')
isToAbsolute = false;
}

if (process.platform === 'win32') {
if (!isFromAbsolute && !isToAbsolute) {
from = '/fakepath/' + from;
Copy link
Copy Markdown
Contributor

@refack refack Jun 22, 2017

Choose a reason for hiding this comment

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

wrap (same as above)

to = '/fakepath/' + to;
from = posix.resolve(from);
to = posix.resolve(to);
} else if (isFromAbsolute && !isToAbsolute) {
to = from + '/' + to;
to = posix.resolve(to);
} else if (!isFromAbsolute && isToAbsolute) {
from = to + '/' + from;
from = posix.resolve(from);
}
}

if (from === to)
return '';
Expand Down
Loading