-
Notifications
You must be signed in to change notification settings - Fork 29k
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
Fix for path.resolve containing relative path to another drive in windows #57
Conversation
d7e65ff
to
185d11c
Compare
I just added upstream |
Seems like other people's commits got included. Is v0.12 the branch to pull from or master? |
ok, you totally borked it out :)
Also I would recommend you to use feature branches next time, to ensure that this things won't happen. And do |
Will do |
Done cherry picked only my changes. |
Can we merge this. I thought this fork was more active than nodejs |
@ankurp This needs a review from a Windows developer, maybe @seishun or @piscisaureus. To expedient matters, can you squash the commits and write a nice commit log? See CONTRIBUTING.md for an example. Thanks. |
@ankurp '../../../../../D:/another/random/path' doesn't look like a valid path to me. Can you name an example where this works as you would expect? |
I am squashing the commits. I found this bug when generating sourcemaps for a web project and the files being imported in sass were on a different Windows drive. If you do a path.relative from one drive to another it will go up using |
@seishun Try this please in io.js |
paths starting with relative path to another drive for example `../../d:/some/dir/` are not resolved correctly. This regex update fixes it. Adding and updating tests along with the fix.
This is a path.relative bug if anything. Relative path across drives doesn't make sense. See for example PathRelativePathTo: "On the other hand, "c:\FolderA\FolderB" and "a:\FolderA\FolderD do not share a common prefix, and the function will fail.". |
@seishun The relative path is correct as in the drives are mounted at the root in Windows. You can follow along the relative path and get to that folder in other drive. I have also squashed the commit and updated the PR. |
There is no single "root" in Windows. My Computer is purely a Windows Explorer concept.
Actually no, |
Then the fix is to throw an error if someone tries to do relative path across drives? |
That sounds sane to me. It's a good translation of returning FALSE from PathRelativePathTo. |
Ok let me change the PR then. |
-1.
|
OK then I will throw an exception if there are ../ in the path to another lettered drive. |
* [GitHub] Fork specific PR template * [GitHub] Check for SemVer and Windows test-taint goal
* [GitHub] Fork specific PR template * [GitHub] Check for SemVer and Windows test-taint goal
* [GitHub] Fork specific PR template * [GitHub] Check for SemVer and Windows test-taint goal
Original commit message: [bigint] Fix possibly-uninitialized leading digit on right shift (cherry picked from commit e82a3b4d47a93ab64f07d8c03e3cd17b6b961c3f) (cherry picked from commit 1162c460dee4218abd798b51b88926aef5c8bd61) No-Try: true No-Presubmit: true No-Tree-Checks: true Fixed: chromium:1151890 Change-Id: I26f5c76494a9ff3f5a141f381e1c9a543e368571 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2561618 Auto-Submit: Jakob Kummerow <jkummerow@chromium.org> Commit-Queue: Georg Neis <neis@chromium.org> Reviewed-by: Georg Neis <neis@chromium.org> Cr-Original-Original-Commit-Position: refs/heads/master@{#71422} Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2565245 Reviewed-by: Jakob Kummerow <jkummerow@chromium.org> Cr-Original-Commit-Position: refs/branch-heads/8.7@{nodejs#57} Cr-Original-Branched-From: 0d81cd72688512abcbe1601015baee390c484a6a-refs/heads/8.7.220@{#1} Cr-Original-Branched-From: 942c2ef85caef00fcf02517d049f05e9a3d4b440-refs/heads/master@{#70196} Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2624611 Commit-Queue: Victor-Gabriel Savu <vsavu@google.com> Cr-Commit-Position: refs/branch-heads/8.6@{nodejs#54} Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1} Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472} Refs: v8/v8@412ac52
Original commit message: [bigint] Fix possibly-uninitialized leading digit on right shift (cherry picked from commit e82a3b4d47a93ab64f07d8c03e3cd17b6b961c3f) (cherry picked from commit 1162c460dee4218abd798b51b88926aef5c8bd61) No-Try: true No-Presubmit: true No-Tree-Checks: true Fixed: chromium:1151890 Change-Id: I26f5c76494a9ff3f5a141f381e1c9a543e368571 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2561618 Auto-Submit: Jakob Kummerow <jkummerow@chromium.org> Commit-Queue: Georg Neis <neis@chromium.org> Reviewed-by: Georg Neis <neis@chromium.org> Cr-Original-Original-Commit-Position: refs/heads/master@{#71422} Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2565245 Reviewed-by: Jakob Kummerow <jkummerow@chromium.org> Cr-Original-Commit-Position: refs/branch-heads/8.7@{nodejs#57} Cr-Original-Branched-From: 0d81cd72688512abcbe1601015baee390c484a6a-refs/heads/8.7.220@{#1} Cr-Original-Branched-From: 942c2ef85caef00fcf02517d049f05e9a3d4b440-refs/heads/master@{#70196} Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2624611 Commit-Queue: Victor-Gabriel Savu <vsavu@google.com> Cr-Commit-Position: refs/branch-heads/8.6@{nodejs#54} Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1} Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472} Refs: v8/v8@412ac52
Original commit message: [bigint] Fix possibly-uninitialized leading digit on right shift (cherry picked from commit e82a3b4d47a93ab64f07d8c03e3cd17b6b961c3f) (cherry picked from commit 1162c460dee4218abd798b51b88926aef5c8bd61) No-Try: true No-Presubmit: true No-Tree-Checks: true Fixed: chromium:1151890 Change-Id: I26f5c76494a9ff3f5a141f381e1c9a543e368571 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2561618 Auto-Submit: Jakob Kummerow <jkummerow@chromium.org> Commit-Queue: Georg Neis <neis@chromium.org> Reviewed-by: Georg Neis <neis@chromium.org> Cr-Original-Original-Commit-Position: refs/heads/master@{#71422} Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2565245 Reviewed-by: Jakob Kummerow <jkummerow@chromium.org> Cr-Original-Commit-Position: refs/branch-heads/8.7@{#57} Cr-Original-Branched-From: 0d81cd72688512abcbe1601015baee390c484a6a-refs/heads/8.7.220@{#1} Cr-Original-Branched-From: 942c2ef85caef00fcf02517d049f05e9a3d4b440-refs/heads/master@{#70196} Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2624611 Commit-Queue: Victor-Gabriel Savu <vsavu@google.com> Cr-Commit-Position: refs/branch-heads/8.6@{#54} Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1} Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472} Refs: v8/v8@412ac52 PR-URL: #38275 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Fixes #50
Issue can be reproduced by running
path.resolve
in Windows as suchpath.resolve('C:/some/random/path', '../../../../../D:/another/random/path');
This outputsc:\D:\another\random\path
whered:
drive is not considered absolute.