-
Notifications
You must be signed in to change notification settings - Fork 8.1k
enable using filesystem from a UNC location #4998
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.
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
enable using filesystem from a UNC location #4998
Conversation
| // 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('\\'); |
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.
|
I don't like the change being outside of the filesystemprovider, let me see if I can rework this. |
|
@SteveL-MSFT yeah, it would be the best if we can fix the |
|
@daxian-dbw |
414ec67 to
1193a00
Compare
1193a00 to
cf3736c
Compare
iSazonov
left a comment
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!
| } | ||
|
|
||
| Describe "UNC paths" -Tags 'CI' { | ||
| It "Can Get-ChildItems from a UNC location" -Skip:(!$IsWindows) { |
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?
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
no need to run [feature] anymore since it passed previously and the test case added is CI
|
@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. |
|
I wonder why we have to call NormalizePath method so many times on the same path? Maybe we can fix this? |
|
@anmenaga Please update your review. |
|
@anmenaga ping... |
anmenaga
left a comment
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. :)
adityapatwardhan
left a comment
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
The problem is while validating the path, it splits
\\server\share\folderby the backslash so the root is just\and the path ends up being\server\share\folderwhich isn't valid. Fix is on Windows to add extra slash if the root is\Fix #4990