GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — path: fix path.relative() for prefixes at device root by omsmith · Pull Request #5490 · nodejs/node

Conversation

@omsmith

@omsmith omsmith commented Feb 29, 2016

Copy link
Copy Markdown
Contributor

Pull Request check-list

Please make sure to review and check all of these items:

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Affected core subsystem(s)

Please provide affected core subsystem(s) (like buffer, cluster, crypto, etc)

  • path

Description of change

Please provide a description of the change here.

Fixes #5485

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@omsmith

omsmith commented Feb 29, 2016

Copy link
Copy Markdown
Contributor Author

CC @mscdex @silverwind

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@omsmith
path: fix path.relative() for prefixes at device root
d1e13c2
Fixes nodejs#5485
@omsmith
omsmith force-pushed the path-fix-relative-at-root branch from 032545d to d1e13c2 Compare February 29, 2016 16:15
omsmith
omsmith reviewed Feb 29, 2016
View reviewed changes
Comment thread lib/path.js
return to.slice(toStart + i + 1);
} else if (i === 0) {
// We get here if `from` is the root
// For example: from='/'; to='/foo'

@omsmith omsmith Feb 29, 2016

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This case for could be an if check before the loop, probably?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@silverwind silverwind Feb 29, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Possibly, yeah. Maybe run the benchmarks to see if it helps performance!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@mscdex mscdex added windows Issues and PRs related to the Windows platform. path Issues and PRs related to the path subsystem. and removed windows Issues and PRs related to the Windows platform. labels Feb 29, 2016
@silverwind silverwind added this to the 5.7.1 milestone Feb 29, 2016
@silverwind

silverwind commented Feb 29, 2016

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@silverwind silverwind mentioned this pull request Feb 29, 2016
Release proposal: v5.7.1 #5464
Merged
5 tasks done
@silverwind

silverwind commented Feb 29, 2016

Copy link
Copy Markdown
Contributor

CI is green, LGTM. We can possibly optimize further in the future, but otherwise, I'd land this as is.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@silverwind

silverwind commented Feb 29, 2016

Copy link
Copy Markdown
Contributor

@mscdex wanna sign this off too?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mscdex

mscdex commented Feb 29, 2016

Copy link
Copy Markdown
Contributor

LGTM

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@omsmith

omsmith commented Feb 29, 2016

Copy link
Copy Markdown
Contributor Author

Tweaking later sounds good to me 👍

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@silverwind

silverwind commented Feb 29, 2016

Copy link
Copy Markdown
Contributor

Thanks again! Landed in f296a7f.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@silverwind silverwind closed this Feb 29, 2016
silverwind pushed a commit that referenced this pull request Feb 29, 2016
@omsmith @silverwind
path: fix path.relative() for prefixes at root
f296a7f
Fixes #5485

PR-URL: #5490
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Brian White <[email protected]>
@jasnell

jasnell commented Mar 2, 2016

Copy link
Copy Markdown
Member

@mscdex ... is this relevant for v4?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Fishrock123 pushed a commit that referenced this pull request Mar 2, 2016
@omsmith @Fishrock123
path: fix path.relative() for prefixes at root
91d218d
Fixes #5485

PR-URL: #5490
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Brian White <[email protected]>
@mscdex

mscdex commented Mar 2, 2016

Copy link
Copy Markdown
Contributor

@jasnell It's only relevant if my original path perf PR is landed.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Fishrock123 added a commit that referenced this pull request Mar 2, 2016
@Fishrock123
2016-03-02, Version 5.7.1 (Stable)
805f054
Notable changes:

* governance: The Core Technical Committee (CTC) added four new members
to help guide Node.js core development: Evan Lucas, Rich Trott, Ali
Ijaz Sheikh and Сковорода Никита Андреевич (Nikita Skovoroda).

* openssl: Upgrade from 1.0.2f to 1.0.2g (Ben Noordhuis)
#5507
  - Fix a double-free defect in parsing malformed DSA keys that may
potentially be used for DoS or memory corruption attacks. It is likely
to be very difficult to use this defect for a practical attack and is
therefore considered low severity for Node.js users. More info is
available at https://www.openssl.org/news/vulnerabilities.html#2016-0705
  - Fix a defect that can cause memory corruption in certain very rare
cases relating to the internal `BN_hex2bn()` and `BN_dec2bn()`
functions. It is believed that Node.js is not invoking the code paths
that use these functions so practical attacks via Node.js using this
defect are _unlikely_ to be possible. More info is available at
https://www.openssl.org/news/vulnerabilities.html#2016-0797
  - Fix a defect that makes the CacheBleed Attack
(https://ssrg.nicta.com.au/projects/TS/cachebleed/) possible. This
defect enables attackers to execute side-channel attacks leading to the
potential recovery of entire RSA private keys. It only affects the
Intel Sandy Bridge (and possibly older) microarchitecture when using
hyper-threading. Newer microarchitectures, including Haswell, are
unaffected. More info is available at
https://www.openssl.org/news/vulnerabilities.html#2016-0702

* Fixed several regressions that appeared in v5.7.0:
  - path.relative():
    - Output is no longer unnecessarily verbose (Brian White)
#5389
    - Resolving UNC paths on Windows now works correctly (Owen Smith)
#5456
    - Resolving paths with prefixes now works correctly from the root
directory (Owen Smith) #5490
  - url: Fixed an off-by-one error with `parse()` (Brian White)
#5394
  - dgram: Now correctly handles a default address case when offset and
length are specified (Matteo Collina)
#5407

PR-URL: #5464
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Mar 2, 2016
@Fishrock123
2016-03-02, Version 5.7.1 (Stable)
9091ccd
Notable changes:

* governance: The Core Technical Committee (CTC) added four new members
to help guide Node.js core development: Evan Lucas, Rich Trott, Ali
Ijaz Sheikh and Сковорода Никита Андреевич (Nikita Skovoroda).

* openssl: Upgrade from 1.0.2f to 1.0.2g (Ben Noordhuis)
nodejs#5507
  - Fix a double-free defect in parsing malformed DSA keys that may
potentially be used for DoS or memory corruption attacks. It is likely
to be very difficult to use this defect for a practical attack and is
therefore considered low severity for Node.js users. More info is
available at https://www.openssl.org/news/vulnerabilities.html#2016-0705
  - Fix a defect that can cause memory corruption in certain very rare
cases relating to the internal `BN_hex2bn()` and `BN_dec2bn()`
functions. It is believed that Node.js is not invoking the code paths
that use these functions so practical attacks via Node.js using this
defect are _unlikely_ to be possible. More info is available at
https://www.openssl.org/news/vulnerabilities.html#2016-0797
  - Fix a defect that makes the CacheBleed Attack
(https://ssrg.nicta.com.au/projects/TS/cachebleed/) possible. This
defect enables attackers to execute side-channel attacks leading to the
potential recovery of entire RSA private keys. It only affects the
Intel Sandy Bridge (and possibly older) microarchitecture when using
hyper-threading. Newer microarchitectures, including Haswell, are
unaffected. More info is available at
https://www.openssl.org/news/vulnerabilities.html#2016-0702

* Fixed several regressions that appeared in v5.7.0:
  - path.relative():
    - Output is no longer unnecessarily verbose (Brian White)
nodejs#5389
    - Resolving UNC paths on Windows now works correctly (Owen Smith)
nodejs#5456
    - Resolving paths with prefixes now works correctly from the root
directory (Owen Smith) nodejs#5490
  - url: Fixed an off-by-one error with `parse()` (Brian White)
nodejs#5394
  - dgram: Now correctly handles a default address case when offset and
length are specified (Matteo Collina)
nodejs#5407

PR-URL: nodejs#5464
@omsmith
omsmith deleted the path-fix-relative-at-root branch March 4, 2016 14:20
@MylesBorins MylesBorins mentioned this pull request Mar 10, 2016
Audit commits not found on v4.x #5647
Closed
@MylesBorins MylesBorins added the dont-land-on-v4.x label Mar 10, 2016
@MylesBorins

MylesBorins commented Mar 10, 2016

Copy link
Copy Markdown
Contributor

adding don't land. If we decide to revisit the original Path changes then we can change this

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@MylesBorins MylesBorins added lts-watch-v4.x and removed dont-land-on-v4.x labels Mar 10, 2016
@jasnell

jasnell commented Mar 11, 2016

Copy link
Copy Markdown
Member

hmm... @thealphanerd ... why did you pull the don't land back off and add the lts-watch back on?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@MylesBorins

MylesBorins commented Mar 11, 2016

Copy link
Copy Markdown
Contributor

@rvagg wanted to keep the path changes under LTS watch. There were regressions but now that those have primarily been found and fixed the idea was that we could eventually land them

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jasnell jasnell added the lts-agenda label Mar 11, 2016
@jasnell

jasnell commented Mar 11, 2016

Copy link
Copy Markdown
Member

Added the lts-agenda label. I'd like @nodejs/lts to weight in on the path changes.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@MylesBorins MylesBorins mentioned this pull request Mar 11, 2016
Track backporting Path optimizations nodejs/Release#87
Closed
@rvagg

rvagg commented Mar 14, 2016

Copy link
Copy Markdown
Member

Actually, I just wanted to make sure that if they got backported that they all stayed together since they are sprawling now. I'm -1 on backporting these at all though, just being prepared in case they end up going through.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@MylesBorins MylesBorins removed the lts-agenda label Mar 30, 2016
@MylesBorins MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels May 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

path Issues and PRs related to the path subsystem.

Projects

None yet

Milestone

5.7.1

Development

Successfully merging this pull request may close these issues.

path.relative parse wrong in windows

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@omsmith @silverwind @mscdex @jasnell @MylesBorins @rvagg
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.