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.

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

path: fix win32 volume-relative paths #14440

Closed
wants to merge 1 commit into from

Conversation

TimothyGu
Copy link
Member

path.resolve() and path.join() are left alone in this commit due to
a lack of clear semantics.

Fixes: #14405

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

path

@TimothyGu TimothyGu added path Issues and PRs related to the path subsystem. windows Issues and PRs related to the Windows platform. labels Jul 24, 2017
@nodejs-github-bot nodejs-github-bot added the path Issues and PRs related to the path subsystem. label Jul 24, 2017
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

['C:\\path\\dir\\index.html', 'C:\\'],
['C:\\another_path\\DIR\\1\\2\\33\\\\index', 'C:\\'],
['another_path\\DIR with spaces\\1\\2\\33\\index', ''],
['\\foo\\C:', '\\'],
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is a valid Windows path, is it?

Copy link
Contributor

Choose a reason for hiding this comment

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

path does not do validation, but it's not:
image

Copy link
Member

Choose a reason for hiding this comment

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

I don't mean we should validate it, I just don't think we should test against invalid paths.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see any harm in having it here. In fact we should keep it just for compatibility until some future fix that needs to remove it.

['C:abc', 'C:'],
['C:\\', 'C:\\'],
['C:\\abc', 'C:\\' ],
['', ''],
Copy link
Member

Choose a reason for hiding this comment

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

Could you add tests for NTFS alterate streams? e.g. ['cd:foo', '']. Not just to test root, but also the other properties used in checkParseFormat.

assert.strictEqual(path.win32.basename('C:basename.ext'), 'basename.ext');
assert.strictEqual(path.win32.basename('C:basename.ext\\'), 'basename.ext');
assert.strictEqual(path.win32.basename('C:basename.ext\\\\'), 'basename.ext');
assert.strictEqual(path.win32.basename('C:foo'), 'foo');
Copy link
Member

Choose a reason for hiding this comment

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

Again, a test for NTFS alternate streams would be nice: basename('cd:foo') === 'cd:foo'

['C:\\path\\dir\\index.html', 'C:\\'],
['C:\\another_path\\DIR\\1\\2\\33\\\\index', 'C:\\'],
['another_path\\DIR with spaces\\1\\2\\33\\index', ''],
['\\foo\\C:', '\\'],
Copy link
Contributor

Choose a reason for hiding this comment

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

path does not do validation, but it's not:
image

@@ -406,6 +422,11 @@ assert.strictEqual(path.win32.normalize('a//b//.'), 'a\\b');
assert.strictEqual(path.win32.normalize('//server/share/dir/file.ext'),
'\\\\server\\share\\dir\\file.ext');
assert.strictEqual(path.win32.normalize('/a/b/c/../../../x/y/z'), '\\x\\y\\z');
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional:

Could you parameterize this part like the others are:

[[a,b]].forEach(([tested, expected]) => ...

(not the scope of this PR but still would be nice)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather leave that to a dedicated PR, as there are many other tests that do not use this pattern in the file.

const output = path.parse(element);
assert.strictEqual(typeof output.root, 'string');
assert.strictEqual(typeof output.dir, 'string');
assert.strictEqual(typeof output.base, 'string');
assert.strictEqual(typeof output.ext, 'string');
assert.strictEqual(typeof output.name, 'string');
assert.strictEqual(path.format(output), element);
assert.strictEqual(output.root, root);
assert.strictEqual(output.dir.indexOf(output.root), 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

could you use startsWith, so it won't pop up when grepping for indexOf

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't quite know the reason for grepping indexOf, but changed nevertheless.

@refack
Copy link
Contributor

refack commented Jul 24, 2017

@TimothyGu could you run performance comparison, just for reference?
(minimal set could be with --filter parse path i.e. node benchmark/compare.js --old .\nodefwithout.exe --new Release\nodewith.exe --filter parse path > path.csv)

@refack
Copy link
Contributor

refack commented Jul 24, 2017

P.S. thanks for doing this 👍! Playing with path is not exactly a fun time...

@TimothyGu
Copy link
Member Author

@refack For good measure:

$ node benchmark/compare.js --old ./node-master --new ./node-14440 --runs 10 --filter parse-win32 path > path-win32.csv
$ Rscript benchmark/compare.R < path-win32.csv 
                                                                             improvement confidence      p.value
 path/parse-win32.js n=1000000 path=""                                           -1.44 %            1.527455e-01
 path/parse-win32.js n=1000000 path="\\\\foo"                                    29.10 %        *** 4.505598e-07
 path/parse-win32.js n=1000000 path="\\\\foo\\\\bar\\\\baz\\\\asdf\\\\.quux"      3.27 %        *** 7.782213e-11
 path/parse-win32.js n=1000000 path="C:\\\\"                                      0.36 %            2.234293e-01
 path/parse-win32.js n=1000000 path="C:\\\\foo"                                  33.79 %        *** 5.225296e-12
 path/parse-win32.js n=1000000 path="E:\\\\foo\\\\bar.baz"                        0.09 %            7.339049e-01
 path/parse-win32.js n=1000000 path="foo\\\\.bar.baz"                             1.26 %        *** 3.623360e-06
 path/parse-win32.js n=1000000 path="foo\\\\bar"                                 -0.14 %            6.443696e-01

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Thank you

@TimothyGu
Copy link
Member Author

TimothyGu commented Jul 25, 2017

However:

                                                                                                improvement confidence      p.value
 path/extname-win32.js n=1000000 path=""                                                           -74.64 %        *** 1.674087e-26
 path/extname-win32.js n=1000000 path="\\\\"                                                        -4.91 %        *** 1.755024e-07
 path/extname-win32.js n=1000000 path="\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux.foobarbazasdfquux"     -7.25 %        *** 1.823382e-05
 path/extname-win32.js n=1000000 path="C:\\\\foo"                                                  -21.36 %        *** 5.547522e-18
 path/extname-win32.js n=1000000 path="D:\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux"                    -23.40 %        *** 3.036449e-06
 path/extname-win32.js n=1000000 path="foo\\\\.bar.baz"                                             -7.85 %        *** 1.491586e-14
 path/extname-win32.js n=1000000 path="foo\\\\bar\\\\...baz.quux"                                  -12.20 %        *** 4.673165e-05
 path/extname-win32.js n=1000000 path="foo\\\\bar\\\\..baz.quux"                                   -11.68 %        *** 1.828409e-13
 path/extname-win32.js n=1000000 path="index.html"                                                  -8.17 %        *** 1.836566e-09
 path/extname-win32.js n=1000000 path="index"                                                       -7.79 %          * 2.396233e-02

There isn't anything I can do to improve the worst case (path=""). On the other hand, two factors alleviate the drastic change:

  • The same piece of code is in basename() already.
  • V8 5.9 used in master is known to not handle charCodeAt() well, and Node.js canary which has V8 6.1 restores the original performance for that benchmark while making all other benchmarks faster as well.

@refack
Copy link
Contributor

refack commented Jul 25, 2017

There isn't anything I can do to improve the worst case (path=""). On the other hand, two factors alleviate the drastic change:

Since this is a bug fix IMHO we can absorb the performance degradation for this case. Especially since the most common case doesn't take a bit hit (and the absolute numbers are probably tiny anyway):

 path/extname-win32.js n=1000000 path="index.html"              -8.17 %        *** 1.836566e-09

@addaleax
Copy link
Member

@TimothyGu This would need to be rebased ;)

@tniessen
Copy link
Member

tniessen commented Jul 26, 2017

@TimothyGu My bad, sorry, I landed f7f590c this morning. You can safely drop my changes to winPaths.

`path.resolve()` and `path.join()` are left alone in this commit due to
the lack of clear semantics.

Fixes: nodejs#14405
@TimothyGu
Copy link
Member Author

@addaleax @tniessen Rebased. I compared and cherry-picked some changes in f7f590c not already in test-path.js, so this should benefit other path.* functions as well.

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

LGTM, no opinion on performance. Thanks for fixing this :)

@TimothyGu
Copy link
Member Author

Landed in 2791761.

@TimothyGu TimothyGu closed this Jul 30, 2017
@TimothyGu TimothyGu deleted the path-win32 branch July 30, 2017 11:27
TimothyGu added a commit that referenced this pull request Jul 30, 2017
`path.resolve()` and `path.join()` are left alone in this commit due to
the lack of clear semantics.

PR-URL: #14440
Fixes: #14405
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
addaleax pushed a commit that referenced this pull request Aug 1, 2017
`path.resolve()` and `path.join()` are left alone in this commit due to
the lack of clear semantics.

PR-URL: #14440
Fixes: #14405
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@addaleax addaleax mentioned this pull request Aug 2, 2017
@MylesBorins
Copy link
Contributor

This does not land cleanly in LTS. Please feel free to manually backport by following the guide. Please also feel free to replace do-not-land if it is being backported

@TimothyGu
Copy link
Member Author

@MylesBorins Done: #14787

MylesBorins pushed a commit that referenced this pull request Aug 14, 2017
`path.resolve()` and `path.join()` are left alone in this commit due to
the lack of clear semantics.

Backport-PR-URL: #14787
PR-URL: #14440
Fixes: #14405
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
`path.resolve()` and `path.join()` are left alone in this commit due to
the lack of clear semantics.

Backport-PR-URL: #14787
PR-URL: #14440
Fixes: #14405
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
MylesBorins added a commit that referenced this pull request Sep 5, 2017
This LTS release comes with 152 commits. This includes 75 which are
test related, 25 which are doc related, 21 which are build / tool
related and 3 commits which are updates to dependencies.

Notable Changes:

* build:
 - Codesigning is fixed on macOS (Evan Lucas)
   #14179
* deps:
 - Snapshots are turned back on!!! (Yang Guo)
   #14385
* path:
 - win32 volume-relative paths are working again! (Timothy Gu)
   #14440
* tools:
 - v6.x can now build with ICU 59 (Steven R. Loomis)
   #12078

PR-URL: #14852
MylesBorins added a commit that referenced this pull request Sep 5, 2017
This LTS release comes with 152 commits. This includes 75 which are
test related, 25 which are doc related, 21 which are build / tool
related and 3 commits which are updates to dependencies.

Notable Changes:

* build:
 - Codesigning is fixed on macOS (Evan Lucas)
   #14179
* deps:
 - Snapshots are turned back on!!! (Yang Guo)
   #14385
* path:
 - win32 volume-relative paths are working again! (Timothy Gu)
   #14440
* tools:
 - v6.x can now build with ICU 59 (Steven R. Loomis)
   #12078

PR-URL: #14852
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017
This LTS release comes with 152 commits. This includes 75 which are
test related, 25 which are doc related, 21 which are build / tool
related and 3 commits which are updates to dependencies.

Notable Changes:

* build:
 - Codesigning is fixed on macOS (Evan Lucas)
   nodejs#14179
* deps:
 - Snapshots are turned back on!!! (Yang Guo)
   nodejs#14385
* path:
 - win32 volume-relative paths are working again! (Timothy Gu)
   nodejs#14440
* tools:
 - v6.x can now build with ICU 59 (Steven R. Loomis)
   nodejs#12078

PR-URL: nodejs#14852
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
`path.resolve()` and `path.join()` are left alone in this commit due to
the lack of clear semantics.

PR-URL: nodejs/node#14440
Fixes: nodejs/node#14405
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
path Issues and PRs related to the path subsystem. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

path.join('C:') = 'C:.' only in windows node LTS version
6 participants