Fix Resolve-Path -LiteralPath. by charub · Pull Request #2572 · PowerShell/PowerShell

Charu Bassi (charub) · 2016-10-31T23:04:17Z

Resolving #2570.

Ignore drive paths in the check. If the input is a drive path, then "" is passed as the path to GetDriveQualifiedPath with drive info. Since, the path here starts with "", treatAsRelative is set to false
which prevents further formatting of this path. The check is only valid for PSDrives with root path equals to UNC paths or the paths in unix.

Microsoft Contribution License Agreements (msftclas) · 2016-10-31T23:04:22Z

Hi Charu Bassi (@charub), I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Charu Bassi). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

Microsoft Contribution License Agreements (msftclas) · 2016-11-01T00:56:10Z

Hi Charu Bassi (@charub), I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Charu Bassi). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

sergei (vors) · 2016-11-01T00:56:24Z

Re-triggered the builds, because master was broken

James Truher (JamesWTruher) · 2016-11-01T18:36:56Z

please add more tests for resolve-path. the ones we have here don't have any which use -literalpath. the resolve-path.tests.ps1 file is the right place for them

James Truher (JamesWTruher) · 2016-11-01T22:24:12Z

LGTM

Dongbo Wang (daxian-dbw) · 2016-11-01T22:34:16Z

These comparison is tedious. We can achieve the same with the following:

if (path.IndexOfAny(new char[]{ '\\', '/' }, 0, 1) == 0 && path.Length > 1) { treatAsRelative = false; }

Note that since this code path may be hot, the new char[]{ '\\', '/' } probably should be declared as a static char array in StringLiterals using StringLiterals.DefaultPathSeparator and StringLiterals.AlternatePathSeparator. This is based on Performance considerations.

How about (path.StartsWith(StringLiterals.DefaultPathSeparatorString, StringComparison.Ordinal) || path.StartsWith(StringLiterals.AlternatePathSeparatorString, StringComparison.Ordinal) && path.length>1

Why not path.IndexOfAny(new char[]{ '\\', '/' }, 0, 1) == 0? It's concise and efficient -- only check the first character of path essentially.

Actually the following is better as path may be an empty string.

if (path.Length > 1 && path.IndexOfAny(new char[]{ '\\', '/' }, 0, 1) == 0) { treatAsRelative = false; }

I think the current check is simple to understand and serves the purpose.

Dongbo Wang (daxian-dbw) · 2016-11-01T22:35:55Z

Please, add more comments explaining what you are doing here. It will be hard to understand the original intention if you look at this code again after some time. This is required by Code comments.

Added comments to explain the change

Dongbo Wang (daxian-dbw) · 2016-11-01T22:38:51Z

Please fix the indentations before It.

Fixed the indentation.

Dongbo Wang (daxian-dbw) · 2016-11-01T22:40:34Z

Is the following working the same with your fix?

PS /> New-PSDrive -Name Test -Root / -PSProvider FileSystem Name Used (GB) Free (GB) Provider Root CurrentLocation ---- --------- --------- -------- ---- --------------- Test 7.59 21.22 FileSystem / PS /> Resolve-Path -LiteralPath Test:/ Path ---- Test:/

Dongbo Wang (daxian-dbw) · 2016-11-01T23:22:12Z

Please reverse the order of "path.Length > 1" and (path.StartsWith() || path.StartsWith()), so that we can avoid unnecessary string comparison under some circumstance.

Reversed the order.

Dongbo Wang (daxian-dbw) · 2016-11-02T00:02:52Z

equal to "" or "/"

One more minor thing. This comment seems not right since the code suggesting "\" or "/".

Fixed the comment.

Resolving PowerShell#2570. Ignore drive paths in the check. If the input is a drive path, then "\" is passed as the path to GetDriveQualifiedPath with drive info. Since, the path here starts with "\", treatAsRelative is set to false which prevents further formatting of this path. The check is only valid for PSDrives with root path equals to UNC paths or the paths in unix.

Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Oct 31, 2016

Charu Bassi (charub) assigned Charu Bassi (charub) and James Truher (JamesWTruher) and unassigned Charu Bassi (charub) Oct 31, 2016

Charu Bassi (charub) force-pushed the 2570 branch from 2e71825 to 2a6b9f6 Compare October 31, 2016 23:49

sergei (vors) closed this Nov 1, 2016

sergei (vors) reopened this Nov 1, 2016

Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Nov 1, 2016

Charu Bassi (charub) force-pushed the 2570 branch from 2a6b9f6 to 5d06c95 Compare November 1, 2016 01:45

Charu Bassi (charub) force-pushed the 2570 branch 2 times, most recently from cd58140 to 09ab983 Compare November 1, 2016 21:14

Charu Bassi (charub) assigned Jason Shirk (lzybkr) Nov 1, 2016

Dongbo Wang (daxian-dbw) requested changes Nov 1, 2016

View reviewed changes

Charu Bassi (charub) force-pushed the 2570 branch from 09ab983 to 8936b8f Compare November 1, 2016 23:13

Dongbo Wang (daxian-dbw) reviewed Nov 1, 2016

View reviewed changes

Charu Bassi (charub) force-pushed the 2570 branch from 8936b8f to 94ea9d0 Compare November 1, 2016 23:47

Dongbo Wang (daxian-dbw) reviewed Nov 2, 2016

View reviewed changes

Charu Bassi (charub) force-pushed the 2570 branch from 94ea9d0 to 513de17 Compare November 2, 2016 00:24

Dongbo Wang (daxian-dbw) approved these changes Nov 2, 2016

View reviewed changes

Dongbo Wang (daxian-dbw) merged commit 85825e2 into PowerShell:master Nov 2, 2016

Charu Bassi (charub) mentioned this pull request Nov 2, 2016

Regression caused by PR #2392, Resolve-Path -LiteralPath returns wrong output. #2570

Closed

Conversation

Charu Bassi (charub) commented Oct 31, 2016

Uh oh!

Microsoft Contribution License Agreements (msftclas) commented Oct 31, 2016

Uh oh!

Microsoft Contribution License Agreements (msftclas) commented Nov 1, 2016

Uh oh!

sergei (vors) commented Nov 1, 2016

Uh oh!

James Truher (JamesWTruher) commented Nov 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

James Truher (JamesWTruher) commented Nov 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dongbo Wang (daxian-dbw) Nov 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dongbo Wang (daxian-dbw) Nov 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

James Truher (JamesWTruher) commented Nov 1, 2016 •

edited

Loading

Dongbo Wang (daxian-dbw) Nov 1, 2016 •

edited

Loading

Dongbo Wang (daxian-dbw) Nov 1, 2016 •

edited

Loading