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.relative() wrong result in v5.7.0 Windows #5447

Closed
frickt opened this issue Feb 26, 2016 · 6 comments
Closed

path.relative() wrong result in v5.7.0 Windows #5447

frickt opened this issue Feb 26, 2016 · 6 comments
Labels
confirmed-bug Issues with confirmed bugs. path Issues and PRs related to the path subsystem.
Milestone

Comments

@frickt
Copy link

frickt commented Feb 26, 2016

Version: <= 5.6.0:

$ node
> process.version
'v5.6.0'
> path.relative('C:\\git\\foo\\bootstrap-loader','C:\\git\\foo\\bootstrap')
'..\\bootstrap'

Version: 5.7.0


$ node
> process.version
process.version
'v5.7.0'
> path.relative('C:\\git\\foo\\bootstrap-loader','C:\\git\\foo\\bootstrap')
'..'

'..' needs to be '..\bootstrap'

@frickt frickt changed the title path.relative() wrong result in v5.7.0 Windows 10 path.relative() wrong result in v5.7.0 Windows Feb 26, 2016
@rvagg
Copy link
Member

rvagg commented Feb 26, 2016

dupe of #5383 fixed in #5389, this is a regression that'll be fixed in a 5.7.1 which is due out shortly, thanks for reporting

@rvagg rvagg closed this as completed Feb 26, 2016
@targos
Copy link
Member

targos commented Feb 26, 2016

It's not a dupe of #5383. The result is really wrong here and I'm quite sure #5389 did not fix this.

cc @mscdex

@targos targos reopened this Feb 26, 2016
@targos targos added path Issues and PRs related to the path subsystem. confirmed-bug Issues with confirmed bugs. labels Feb 26, 2016
@rvagg
Copy link
Member

rvagg commented Feb 26, 2016

right, sorry for my hastiness, confirmed that it's still borked with #5389

@rvagg rvagg mentioned this issue Feb 26, 2016
3 tasks
@targos targos added this to the 5.7.1 milestone Feb 26, 2016
@omsmith
Copy link
Contributor

omsmith commented Feb 26, 2016

Played with it a bit, the following diff passes, but really not sure about it.

diff --git a/lib/path.js b/lib/path.js
index d01e9b6..e012441 100644
--- a/lib/path.js
+++ b/lib/path.js
@@ -603,14 +603,28 @@ const win32 = {
     var i = 0;
     for (; i <= length; ++i) {
       if (i === length) {
-        if (lastCommonSep > 2 && // ignore separator match following device root
-            toLen > length &&
-            to.charCodeAt(i) === 92/*\*/) {
-          // We get here if `from` is the exact base path for `to`.
-          // For example: from='C:\\foo\\bar'; to='C:\\foo\\bar\\baz'
-          return toOrig.slice(i + 1);
+        if (toLen > length) {
+          if (to.charCodeAt(toStart + i) === 92/*\*/) {
+            // We get here if `from` is the exact base path for `to`.
+            // For example: from='C:\\foo\\bar'; to='C:\\foo\\bar\\baz'
+            return toOrig.slice(toStart + i + 1);
+          } else if (lastCommonSep === 2) {
+            // We get here if `from` is the device root.
+            // For example: from='C:\\'; to='C:\\foo'
+            return toOrig.slice(toStart + i);
+          }
+        }
+        if (fromLen > length) {
+          if (from.charCodeAt(fromStart + i) === 92/*\*/) {
+            // We get here if `to` is the exact base path for `from`.
+            // For example: from:='C:\\foo\\bar'; to='C:\\foo'
+            lastCommonSep = i;
+          } else if (lastCommonSep === 2) {
+            // We get here if `to` is the device root.
+            // For example: from='C:\\foo\\bar'; to='C:\\'
+            lastCommonSep = 3;
+          }
         }
-        lastCommonSep = i;
         break;
       }
       var fromCode = from.charCodeAt(fromStart + i);
diff --git a/test/parallel/test-path.js b/test/parallel/test-path.js
index aeb6ec0..02b280e 100644
--- a/test/parallel/test-path.js
+++ b/test/parallel/test-path.js
@@ -470,7 +470,9 @@ const relativeTests = [
      ['c:/AaAa/bbbb', 'c:/aaaa/bbbb', ''],
      ['c:/aaaaa/', 'c:/aaaa/cccc', '..\\aaaa\\cccc'],
      ['C:\\foo\\bar\\baz\\quux', 'C:\\', '..\\..\\..\\..'],
-     ['C:\\foo\\test', 'C:\\foo\\test\\bar\\package.json', 'bar\\package.json']
+     ['C:\\foo\\test', 'C:\\foo\\test\\bar\\package.json', 'bar\\package.json'],
+     ['C:\\foo\\bar\\baz-quux', 'C:\\foo\\bar\\baz', '..\\baz'],
+     ['C:\\foo\\bar', 'C:\\baz\\quux', '..\\..\\baz\\quux']
     ]
   ],
   [ path.posix.relative,

@mscdex
Copy link
Contributor

mscdex commented Feb 26, 2016

@omsmith That LGTM, mind making a PR?

@omsmith
Copy link
Contributor

omsmith commented Feb 26, 2016

@mscdex sure thing

omsmith added a commit to omsmith/node that referenced this issue Feb 27, 2016
when the basename of "to" was a prefix of the basename of "from" win32
relative() would miss including it in the result

Fixes nodejs#5447
omsmith added a commit to omsmith/node that referenced this issue Feb 27, 2016
adds posix test cases for paths similar to those that caused nodejs#5447
silverwind pushed a commit that referenced this issue Feb 27, 2016
adds posix test cases for paths similar to those that caused #5447

PR-URL: #5456
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
rvagg pushed a commit that referenced this issue Feb 28, 2016
when the basename of "to" was a prefix of the basename of "from" win32
relative() would miss including it in the result

Fixes: #5447
PR-URL: #5456
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
rvagg pushed a commit that referenced this issue Feb 28, 2016
adds posix test cases for paths similar to those that caused #5447

PR-URL: #5456
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. path Issues and PRs related to the path subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants