URL.path should not strip trailing slash for root paths on Windows
#1038
Conversation
|
@swift-ci please test |
| return posixPath | ||
| private static func windowsPath(for urlPath: String) -> String { | ||
| let utf8 = urlPath.utf8 | ||
| guard !utf8.starts(with: [._slash, ._slash]) else { |
There was a problem hiding this comment.
What about \\server\share\directory\? Should that not be \\server\share\directory?
There was a problem hiding this comment.
Ideally, yeah. I think it gets a bit complicated to make sure we do the right thing for all paths, though. I'm not super familiar with UNC paths, so please correct me if I'm wrong, but I think we could have:
\\.\c:\
\\.\BootPartition\
\\server\share\
\\.\server\share\
\\.\UNC\server\share\
so we couldn't rely on just stripping after X segments. And it still might be complex to check for . or ? in the first segment since we could have either driveName or server\share following it.
Do you think we should call PathIsRootW to determine whether we should strip the slash? I'm a bit hesitant to do filesystem I/O in URL.path, but I think that would get us the correct answer.
There was a problem hiding this comment.
Yeah, PathIsRootW is the right thing to use IMO. \\.\ paths are device paths, there are also kernel paths \\?\. Additionally, you might also encounter a root local path with a \??\ prefix. For the UNC path, you can also label it via link, e.g. \\?\UNC\server\share\directory\.
There was a problem hiding this comment.
Switched to checking PathIsRootW for non-drive letter paths in 9d325ee
| array[2] == ._colon, | ||
| array[3] == ._slash { | ||
| return String(Substring(utf8.dropFirst())) | ||
| var iter = utf8.makeIterator() |
There was a problem hiding this comment.
Can we assert that the path starts with [._slash, alpha, ._colon, ._slash]?
There was a problem hiding this comment.
Unfortunately I don't think we can check for alpha like that.
jrflat
force-pushed
the
trailing-slash-windows
branch
from
e540a0b to
9d325ee
Compare
|
@swift-ci please test |
| let path = decodeFilePath(urlPath) | ||
| return path.replacing(._slash, with: ._backslash).withCString(encodedAs: UTF16.self) { pwszPath in | ||
| guard !PathIsRootW(pwszPath) else { | ||
| return path |
There was a problem hiding this comment.
I think that we still get one path incorrect now: /S:. This a non-UNC, drive-relative path. We could either resolve that to an absolute path or return it as S:. You can also have a more complex path such as S:utils\build.ps1 (which I believe would be encoded as /s:utils/build.ps1.
There was a problem hiding this comment.
Yeah, more work definitely needs to be done to support drive-relative paths in URL. Currently, S:utils\build.ps1 is actually treated as a relative path, relative to the current working directory (but not necessarily the cwd of S:), so .path won't resolve it correctly.
I think we should get the cwd of S: during URL(filePath:) initialization and use that cwd path as the base URL, then strip S: from the relative path. If that sounds good I can open up a follow-on PR, since it'll be slightly more involved than this change, which fixes C:\ and other absolute paths' behaviors.
There was a problem hiding this comment.
I think that we should do some checking if the call needs to be done, but otherwise, this sounds like a good way forward. Doing that in a follow up is totally fine.
We should not strip the trailing slash when getting the
fileSystemPathforC:\on Windows, sinceC:\represents the root whileC:does not. Similarly, we should not strip the trailing slash for any UNC path, since the trailing slash might be root.Resolves #976 and #977