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

[v6.x backport] path.win32 related fixes and tests #14787

Closed
wants to merge 3 commits into from
Closed

[v6.x backport] path.win32 related fixes and tests #14787

wants to merge 3 commits into from

Conversation

TimothyGu
Copy link
Member

First commit (#14438):

commit 5180f932eaccfa380f68908b00b098205cb466ba
Author: Tobias Nießen <tniessen@tnie.de>
Date:   Mon Jul 24 16:20:30 2017 +0200

    test: increase coverage for path.parse
    
    PR-URL: https://github.com/nodejs/node/pull/14438
    Reviewed-By: Refael Ackermann <refack@gmail.com>
    Reviewed-By: James M Snell <jasnell@gmail.com>
    Reviewed-By: Anna Henningsen <anna@addaleax.net>
    Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
    Reviewed-By: Timothy Gu <timothygu99@gmail.com>
    Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
    Reviewed-By: Khaidi Chu <i@2333.moe>

Second commit (#14440):

commit 6fa5f3575422a12d374b349e3234331ca5e734c8
Author: Timothy Gu <timothygu99@gmail.com>
Date:   Mon Jul 24 08:39:02 2017 +0800

    path: fix win32 volume-relative paths
    
    `path.resolve()` and `path.join()` are left alone in this commit due to
    the lack of clear semantics.
    
    PR-URL: https://github.com/nodejs/node/pull/14440
    Fixes: https://github.com/nodejs/node/issues/14405
    Reviewed-By: Refael Ackermann <refack@gmail.com>
    Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

path

tniessen and others added 2 commits August 12, 2017 17:35
@tniessen @TimothyGu
test: increase coverage for path.parse
5180f93 
PR-URL: nodejs#14438
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
@TimothyGu
path: fix win32 volume-relative paths
6fa5f35 
`path.resolve()` and `path.join()` are left alone in this commit due to
the lack of clear semantics.

PR-URL: nodejs#14440
Fixes: nodejs#14405
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@TimothyGu TimothyGu requested review from refack and tniessen August 12, 2017 10:30
@nodejs-github-bot nodejs-github-bot added path Issues and PRs related to the path subsystem. v6.x labels Aug 12, 2017
This was referenced Aug 12, 2017
Closed
Closed
@TimothyGu
Copy link
Member Author

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

@tniessen
Copy link
Member

Did you intentionally not backport 2ac0aff (part of #14438)?

@TimothyGu
Copy link
Member Author

@tniessen No it was not intentional. I backported the test commit in #14438 because it caused the merge conflict in #14440 but I'll backport the other one too. Thanks for noticing!

@tniessen
path: remove unnecessary string copies
f5a8586 
As the length of `path` is known at this point, there is no point in
making an exact copy using `slice`.

PR-URL: nodejs#14438
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
@tniessen
Copy link
Member

I pushed the missing commit as discussed on IRC.

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

MylesBorins pushed a commit that referenced this pull request Aug 14, 2017
@tniessen @MylesBorins
test: increase coverage for path.parse
3f2b8a9 
Backport-PR-URL: #14787
PR-URL: #14438
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
MylesBorins pushed a commit that referenced this pull request Aug 14, 2017
@TimothyGu @MylesBorins
path: fix win32 volume-relative paths
ba411f1 
`path.resolve()` and `path.join()` are left alone in this commit due to
the lack of clear semantics.

Backport-PR-URL: #14787
PR-URL: #14440
Fixes: #14405
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this pull request Aug 14, 2017
@tniessen @MylesBorins
path: remove unnecessary string copies
d75363b 
As the length of `path` is known at this point, there is no point in
making an exact copy using `slice`.

Backport-PR-URL: #14787
PR-URL: #14438
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
@MylesBorins
Copy link
Contributor

landed in ee98df8...d75363b

@TimothyGu TimothyGu deleted the v6.x-14440 branch August 15, 2017 06:30
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
@tniessen @MylesBorins
test: increase coverage for path.parse
c2a0a93 
Backport-PR-URL: #14787
PR-URL: #14438
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
@TimothyGu @MylesBorins
path: fix win32 volume-relative paths
eefd322 
`path.resolve()` and `path.join()` are left alone in this commit due to
the lack of clear semantics.

Backport-PR-URL: #14787
PR-URL: #14440
Fixes: #14405
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
@tniessen @MylesBorins
path: remove unnecessary string copies
e82a914 
As the length of `path` is known at this point, there is no point in
making an exact copy using `slice`.

Backport-PR-URL: #14787
PR-URL: #14438
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tniessen tniessen tniessen approved these changes

@refack refack refack approved these changes

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

Successfully merging this pull request may close these issues.
5 participants
@TimothyGu @tniessen @MylesBorins @refack @nodejs-github-bot