Skip to content

Conversation

@SteveL-MSFT
Copy link
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

// 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('\\');
Copy link

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.

address PR feedback
@SteveL-MSFT
Copy link
Member Author

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

@daxian-dbw
Copy link
Member

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

@SteveL-MSFT
Copy link
Member Author

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

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

@iSazonov iSazonov left a 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!

}

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure

no need to run [feature] anymore since it passed previously and the test case added is CI
@SteveL-MSFT
Copy link
Member Author

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

@iSazonov
Copy link
Collaborator

iSazonov commented Oct 4, 2017

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

move code to reproduce UNC path to GetParentPath()
@SteveL-MSFT
Copy link
Member Author

@iSazonov I created #5002 as the perf work item

@adityapatwardhan
Copy link
Member

@anmenaga Please update your review.

@adityapatwardhan
Copy link
Member

@anmenaga ping...

Copy link

@anmenaga anmenaga left a 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. :)

Copy link
Member

@adityapatwardhan adityapatwardhan left a comment

Choose a reason for hiding this comment

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

LGTM

@adityapatwardhan adityapatwardhan merged commit 90165fc into PowerShell:master Oct 9, 2017
@SteveL-MSFT SteveL-MSFT deleted the filesystem-unc branch October 26, 2018 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants