GeezerVison

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

GeezerVison is showing github.com — enable using filesystem from a UNC location by SteveL-MSFT · Pull Request #4998 · PowerShell/PowerShell

Conversation

Steve Lee (SteveL-MSFT) commented Oct 3, 2017

Copy link

Copy Markdown

Member

The problem is while validating the path, it splits \\server\share\folder by the backslash so the root is just \ and the path ends up being \server\share\folder which isn't valid. Fix is on Windows to add extra slash if the root is \

Fix #4990

Sorry, something went wrong.

Uh oh!

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

bergmeister and LYP951018 reacted with thumbs up emoji

bergmeister reacted with heart emoji

All reactions

👍 2 reactions
❤️ 1 reaction


          enable using filesystem from a UNC location

04c9ab9

Steve Lee (SteveL-MSFT) assigned Aditya Patwardhan (adityapatwardhan)

Oct 3, 2017

Steve Lee (SteveL-MSFT) requested review from Aditya Patwardhan (adityapatwardhan) and Andrew (anmenaga)

October 3, 2017 22:26

Steve Lee (SteveL-MSFT) requested a review from Bruce Payette (BrucePay) as a code owner

October 3, 2017 22:26

Andrew (anmenaga) suggested changes

Oct 3, 2017

View reviewed changes

Comment thread

src/System.Management.Automation/namespaces/NavigationProviderBase.cs Outdated

		// On Windows, if we have a single backslash for the parent, it's a UNC path so we need to add another backslash back
		if (String.Compare(parent, StringLiterals.DefaultPathSeparatorString, StringComparison.Ordinal) == 0)
		{
		builder.Append('\\');

Andrew (anmenaga) Oct 3, 2017

Copy link

Copy Markdown

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.

Probably better to use DefaultPathSeparatorString instead of char constant.

Sorry, something went wrong.

Uh oh!

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

All reactions


          [feature]

7d4db21

address PR feedback

Steve Lee (SteveL-MSFT) commented Oct 4, 2017

Copy link

Copy Markdown

Member Author

I don't like the change being outside of the filesystemprovider, let me see if I can rework this.

All reactions

Sorry, something went wrong.

Uh oh!

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

Dongbo Wang (daxian-dbw) commented Oct 4, 2017

Copy link

Copy Markdown

Member

Steve Lee (@SteveL-MSFT) yeah, it would be the best if we can fix the GetParentPath to take into account UNC paths.

All reactions

Sorry, something went wrong.

Uh oh!

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

Steve Lee (SteveL-MSFT) commented Oct 4, 2017

Copy link

Copy Markdown

Member Author

Dongbo Wang (@daxian-dbw) GetParentPath gets called repeatedly to validate the entire path from leaf to root. I think putting the fix in NormalizePath() may be better.

All reactions

Sorry, something went wrong.

Uh oh!

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

Steve Lee (SteveL-MSFT) force-pushed the filesystem-unc branch from 414ec67 to 1193a00 Compare

October 4, 2017 01:24


          [feature]

cf3736c

removed changed in NavigationProviderBase and made change in FileSystemProvider where it should belong

Steve Lee (SteveL-MSFT) force-pushed the filesystem-unc branch from 1193a00 to cf3736c Compare

October 4, 2017 01:24

Ilya (iSazonov) reviewed

Oct 4, 2017

View reviewed changes

Ilya (iSazonov) left a comment

Copy link

Copy Markdown

Collaborator

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.

I unlike the fix - it seems a performance is bad.
I do cd \\localhost\c$ and discover that we call NormalizePath 6-9 times for the same path!

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 Outdated

		}

		Describe "UNC paths" -Tags 'CI' {
		It "Can Get-ChildItems from a UNC location" -Skip:(!$IsWindows) {

Ilya (iSazonov) Oct 4, 2017

Copy link

Copy Markdown

Collaborator

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.

Should we check Set-Location too?

Sorry, something went wrong.

Uh oh!

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

All reactions

Steve Lee (SteveL-MSFT) Oct 4, 2017

Copy link

Copy Markdown

Member 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.

Sure

Sorry, something went wrong.

Uh oh!

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

All reactions


          added variations of tests for set-location and push-location

070f70b

no need to run [feature] anymore since it passed previously and the test case added is CI

Steve Lee (SteveL-MSFT) commented Oct 4, 2017

Copy link

Copy Markdown

Member Author

Ilya (@iSazonov) I've noticed that the providers get called a lot. Perhaps GetParentPath() is the right place even thought that gets called a lot as well, but we only need to make it a UNC path again if it's at the root.

All reactions

Sorry, something went wrong.

Uh oh!

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

Ilya (iSazonov) commented Oct 4, 2017

Copy link

Copy Markdown

Collaborator

I wonder why we have to call NormalizePath method so many times on the same path? Maybe we can fix this?

All reactions

Sorry, something went wrong.

Uh oh!

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


          [feature]

87b153c

move code to reproduce UNC path to GetParentPath()

Steve Lee (SteveL-MSFT) commented Oct 4, 2017

Copy link

Copy Markdown

Member Author

Ilya (@iSazonov) I created #5002 as the perf work item

iSazonov reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

Aditya Patwardhan (adityapatwardhan) commented Oct 6, 2017

Copy link

Copy Markdown

Member

Andrew (@anmenaga) Please update your review.

All reactions

Sorry, something went wrong.

Uh oh!

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

Aditya Patwardhan (adityapatwardhan) commented Oct 9, 2017

Copy link

Copy Markdown

Member

Andrew (@anmenaga) ping...

All reactions

Sorry, something went wrong.

Uh oh!

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

Andrew (anmenaga) approved these changes

Oct 9, 2017

View reviewed changes

Andrew (anmenaga) left a comment

Copy link

Copy Markdown

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.

LGTM.
Too many paths. :)

Sorry, something went wrong.

Uh oh!

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

All reactions

Aditya Patwardhan (adityapatwardhan) approved these changes

Oct 9, 2017

View reviewed changes

Aditya Patwardhan (adityapatwardhan) left a comment

Copy link

Copy Markdown

Member

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.

LGTM

Sorry, something went wrong.

Uh oh!

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

All reactions

Aditya Patwardhan (adityapatwardhan) merged commit 90165fc into PowerShell:master

Oct 9, 2017

Steve Lee (SteveL-MSFT) deleted the filesystem-unc branch

October 26, 2018 21:34

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request

Aug 6, 2026


          enable using filesystem from a UNC location (PowerShell#4998)

aefa95e

* enable using filesystem from a UNC location

* [feature]
address PR feedback

* [feature]
removed changed in NavigationProviderBase and made change in FileSystemProvider where it should belong

* added variations of tests for set-location and push-location
no need to run [feature] anymore since it passed previously and the test case added is CI

* [feature]
move code to reproduce UNC path to GetParentPath()

Reviewers

Ilya (iSazonov)

iSazonov left review comments

Aditya Patwardhan (adityapatwardhan)

adityapatwardhan approved these changes

Bruce Payette (BrucePay)

Awaiting requested review from BrucePay

+1 more reviewer

Andrew (anmenaga)

anmenaga approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

Aditya Patwardhan (adityapatwardhan)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

5 participants

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.