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 posix.relative returns different results #13738

Closed
wants to merge 5 commits into from
Closed

path: fix posix.relative returns different results #13738

Show file tree
Hide file tree
Changes from all commits
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
66 changes: 66 additions & 0 deletions doc/api/path.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,39 @@ added: v0.11.15
The `path.posix` property provides access to POSIX specific implementations
of the `path` methods.

*Note*: Be careful when using the following functions directly on Windows.
Copy link
Member

Choose a reason for hiding this comment

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

This note should be moved into the bodies of each of the API sections. Sitting above it, it is far more likely that the Note will become disconnected...


### path.posix.resolve([...path])
If after processing all given `path` segments an absolute path has not yet
been generated, `path.posix.resolve` will throw [`UNSUPPORTED_PLATFORM`] error.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps:

*Note*: Care should be taken when using `path.posix.resolve()` on Windows due to ...

Then include a short snippet about why...

Same with the path.posix.relative() below.

Copy link
Contributor

@refack refack Jun 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

suggestion:

*Note*: Care should be taken when using `path.posix.resolve()` on Windows due to the fact that resolve will use `process.cwd()` verbatim, and so the output will be a concatenation of Windows and POSIX style paths.

### path.posix.relative(from, to)
If `from` and `to` are both relative path, `path.posix.relative` will use the
Copy link
Member

Choose a reason for hiding this comment

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

Please include the () when referencing functions... e.g. path.posix.relative()

path `/fakepath/` as the current working path to generate the result.

For example

```
path.posix.relative('a/b/c', '../../x');

// 'from' will be '/fakepath/a/b/c'
// 'to' will be '/fakepath/../../x', then will be normalized to '/x'
// Returns: '../../../../x'
```

If one of `from` and `to` is relative path, `path.posix.relative` will use the
other absolute path as the current working path to generate the result.

For example

```
path.posix.relative('/a/b/c', '../../x');

// 'from' will be '/a/b/c'
// 'to' will be '/a/b/c/../../x', then will be normalized to '/a/x'
// Returns: '../../x'
```

## path.relative(from, to)
<!-- YAML
added: v0.5.0
Expand Down Expand Up @@ -551,6 +584,39 @@ added: v0.11.15
The `path.win32` property provides access to Windows-specific implementations
of the `path` methods.

*Note*: Be careful when using the following functions directly on POSIX.

### path.win32.resolve([...path])
If after processing all given `path` segments an absolute path has not yet
been generated, `path.win32.resolve` will throw [`UNSUPPORTED_PLATFORM`] error.

### path.win32.relative(from, to)
If `from` and `to` are both relative path, `path.win32.relative` will use the
path `c:\\fakepath\\` as the current working path to generate the result.

For example

```
path.win32.relative('a\\b\\c', '..\\..\\x');

// 'from' will be 'c:\\fakepath\\a\\b\\c'
// 'to' will be 'c:\\fakepath\\..\\..\\x', then will be normalized to 'c:\\x'
// Returns: '..\\..\\..\\..\\x'
```

If one of `from` and `to` is relative path, `path.win32.relative` will use the
other absolute path as the current working path to generate the result.

For example

```
path.win32.relative('c:\\a\\b\\c', '..\\..\\x');

// 'from' will be 'c:\\a\\b\\c'
// 'to' will be 'c:\\a\\b\\c\\..\\..\\x', then will be normalized to 'c:\\a\\x'
// Returns: '..\\..\\x'
```

[`TypeError`]: errors.html#errors_class_typeerror
[`path.parse()`]: #path_path_parse_path
[`path.posix`]: #path_path_posix
Expand Down
1 change: 1 addition & 0 deletions lib/internal/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ E('ERR_UNKNOWN_BUILTIN_MODULE', (id) => `No such built-in module: ${id}`);
E('ERR_UNKNOWN_SIGNAL', (signal) => `Unknown signal: ${signal}`);
E('ERR_UNKNOWN_STDIN_TYPE', 'Unknown stdin file type');
E('ERR_UNKNOWN_STREAM_TYPE', 'Unknown stream file type');
E('ERR_UNSUPPORTED_PLATFORM', 'Unsupported platform');
E('ERR_SOCKET_ALREADY_BOUND', 'Socket is already bound');
E('ERR_SOCKET_BAD_TYPE',
'Bad socket type specified. Valid types are: udp4, udp6');
Expand Down
90 changes: 86 additions & 4 deletions lib/path.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,18 @@ function assertPath(path) {
}
}

function assertWindowsPlatform(platform) {
// throw an error if direct use the function on *nix platform
if (platform !== 'win32')
throw new errors.Error('ERR_UNSUPPORTED_PLATFORM');
}

function assertPosixPlatform(platform) {
// throw an error if direct use the function on windows platform
if (platform === 'win32')
throw new errors.Error('ERR_UNSUPPORTED_PLATFORM');
}

// Resolves . and .. elements in a path with directory names
function normalizeStringWin32(path, allowAboveRoot) {
var res = '';
Expand Down Expand Up @@ -187,6 +199,10 @@ const win32 = {
path = arguments[i];
} else if (!resolvedDevice) {
path = process.cwd();

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

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 @@ -201,6 +217,10 @@ const win32 = {
path.slice(0, 3).toLowerCase() !==
resolvedDevice.toLowerCase() + '\\') {
path = resolvedDevice + '\\';
} else {
// If you use the current working directory,
Copy link
Member

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);
}
}

Expand Down Expand Up @@ -562,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
Contributor

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
Contributor Author

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
Contributor

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
Contributor

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
Contributor Author

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 @@ -1174,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 @@ -1249,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
Contributor

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