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 parse wrong in windows #5485

Closed
nuintun opened this issue Feb 29, 2016 · 7 comments
Closed

path.relative parse wrong in windows #5485

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

Comments

@nuintun
Copy link

nuintun commented Feb 29, 2016

Node Version: 5.7.0

var path = require('path');
var a = 'E:/WORKSPACE/GitHub/file-send/test/fixtures/name.d/';
var b = 'E:/WORKSPACE/GitHub/file-send/test/fixtures/name.dir/name.txt';

console.log(path.relative(a, b));

result:
qq 20160229181842

@silverwind silverwind added the path Issues and PRs related to the path subsystem. label Feb 29, 2016
@silverwind
Copy link
Contributor

cc: @mscdex

@silverwind
Copy link
Contributor

> path.win32.relative('E:/name.d/', 'E:/name.dir/file.txt')
'ir\\file.txt'

@silverwind silverwind added the confirmed-bug Issues with confirmed bugs. label Feb 29, 2016
@silverwind silverwind added this to the 5.7.1 milestone Feb 29, 2016
@omsmith
Copy link
Contributor

omsmith commented Feb 29, 2016

Affects both posix and win32. Only occurs at the device root

EDIT: win32 occurs outside of the device root in v5.7.0, only occurs at the device root in master (likely due to my fixes)

> path.win32.relative('E:/name.d', 'E:/name.dir/file.txt')
'ir\\file.txt'
> path.win32.relative('E:/foo/name.d', 'E:/foo/name.dir/file.txt')
'..\\name.dir\\file.txt'
> path.posix.relative('/name.d', '/name.dir/file.txt')
'ir/file.txt'
> path.posix.relative('/foo/name.d', '/foo/name.dir/file.txt')
'../name.dir/file.txt'

@omsmith
Copy link
Contributor

omsmith commented Feb 29, 2016

So, pretty sure this is #5447, but we didn't resolve it at the device root. The other direction of "prefixness" problems also exist at the device root which we didn't catch either. Clearly need to add more test cases off of the root, which has ended up being a bit of an edge case:

> path.win32.relative('C:/baz-quux', 'C:/baz')
'..baz'
> path.win32.relative('C:/baz', 'C:/baz-quux')
'-quux'
> path.posix.relative('/baz-quux', '/baz')
'..'
> path.posix.relative('/baz', '/baz-quux')
'-quux'

@omsmith
Copy link
Contributor

omsmith commented Feb 29, 2016

I have a fix for win32, looking at posix

@silverwind
Copy link
Contributor

It seems the issue is observed only in root for posix, and everywhere for win32:

> path.posix.relative('/name.d', '/name.dir/file.txt')
'ir/file.txt'
> path.posix.relative('/a/name.d', '/a/name.dir/file.txt')
'../name.dir/file.txt' // correct
> path.win32.relative('E:/name.d', 'E:/name.dir/file.txt')
'ir\\file.txt'
> path.win32.relative('E:/a/name.d', 'E:/a/name.dir/file.txt')
'ir\\file.txt'

@omsmith
Copy link
Contributor

omsmith commented Feb 29, 2016

Yep, that second win32 case should be working in master/v5.7.1-proposal

omsmith added a commit to omsmith/node that referenced this issue Feb 29, 2016
Fishrock123 pushed a commit that referenced this issue Mar 2, 2016
Fixes #5485

PR-URL: #5490
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Brian White <mscdex@mscdex.net>
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.

3 participants