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

Fix path.relative UNC path result #8523

Closed
wants to merge 1 commit into from
Closed

Fix path.relative UNC path result #8523

wants to merge 1 commit into from

Conversation

jasongin
Copy link
Member

@jasongin jasongin commented Sep 13, 2016 •
edited
Loading

Checklist
  • vcbuild test nosign (Windows) passes
    (Some tests failed, but they're unrelated: the failures repro without this change.)
  • tests are included
  • commit message follows commit guidelines
Affected core subsystem(s)

path

Description of change

When the result of a path.relative() is an absolute UNC path, it should
include the leading backslashes.

Fixes: #8444

@nodejs-github-bot nodejs-github-bot added the path Issues and PRs related to the path subsystem. label Sep 13, 2016
@jasongin
Fix path.relative UNC path result
5712f33 
When the result of a path.relative() is an absolute UNC path, it should
include the leading backslashes.

Fixes: nodejs#8444
@jasongin jasongin changed the title Fix path.resolve UNC path result Fix path.relative UNC path result Sep 13, 2016
@jasongin
Copy link
Member Author

@mscdex please review.

@mscdex mscdex added the windows Issues and PRs related to the Windows platform. label Sep 13, 2016
@mscdex
Copy link
Contributor

mscdex commented Sep 13, 2016

CI: https://ci.nodejs.org/job/node-test-pull-request/4036/

@mscdex
Copy link
Contributor

mscdex commented Sep 13, 2016

LGTM, CI failure on ARM is unrelated.

jasnell
jasnell approved these changes Sep 15, 2016
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM

bzoz
bzoz approved these changes Sep 16, 2016
View reviewed changes
Copy link
Contributor

@bzoz bzoz left a comment

Choose a reason for hiding this comment

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

LGTM

jasnell pushed a commit that referenced this pull request Sep 20, 2016
@jasongin @jasnell
path: fix path.relative UNC path result
c1e47ed 
When the result of a path.relative() is an absolute UNC path, it should
include the leading backslashes.

Fixes: #8444
PR-URL: #8523
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
@jasnell
Copy link
Member

jasnell commented Sep 20, 2016

Landed in c1e47ed! Thank you!

@jasnell jasnell closed this Sep 20, 2016
@MylesBorins
Copy link
Contributor

hey all... does this seem like a good candidate for a backport?

@addaleax
Copy link
Member

addaleax commented Oct 6, 2016

Has this been in a Current release yet? But yeah, I would say so.

@MylesBorins
Copy link
Contributor

@addaleax has not been, but was mostly auditing for labeling

Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
@jasongin @Fishrock123
path: fix path.relative UNC path result
d72a7b3 
When the result of a path.relative() is an absolute UNC path, it should
include the leading backslashes.

Fixes: #8444
PR-URL: #8523
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
@MylesBorins
Copy link
Contributor

@addaleax turns out this actually has been in v6 for a minute... unfortunately not landing cleanly

would someone be willing to do a manual backport?

/cc @jasongin

@addaleax
Copy link
Member

That would probably best be @jasongin or @mscdex?

@mscdex
Copy link
Contributor

mscdex commented Nov 18, 2016

@thealphanerd @addaleax This PR isn't relevant for v4.x since the path module rewrite was not included there.

@addaleax
Copy link
Member

@mscdex thanks for clarifying!

@jasongin jasongin deleted the resolveunc branch November 21, 2016 17:55
@joaocgreis joaocgreis mentioned this pull request Dec 30, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bzoz bzoz bzoz approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
path Issues and PRs related to the path subsystem. windows Issues and PRs related to the Windows platform.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jasongin @mscdex @jasnell @MylesBorins @addaleax @bzoz @nodejs-github-bot