-
Notifications
You must be signed in to change notification settings - Fork 175
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
URL.fileSystemPath
should strip leading slash for Windows drive letters
#964
Conversation
@swift-ci please test |
array[2] == ._colon, | ||
array[3] == ._slash { | ||
return String(Substring(utf8.dropFirst())) | ||
} |
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.
This is unfortunately insufficient. \\server\share
is considered a root, and therefore, //server/share
should be returned similarly. I think that you should do something like if array.count > 1, path.substring(1).withCString(encodedAs: UTF16.self, PathIsRootW)
instead.
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.
Sorry I don't think I understand. Do we need to strip leading slashes from \\server\share
? A URL like URL(filePath: "\\\\server\\share")
is stored internally as file:////server/share
, so URL.fileSystemPath
will return //server/share
in that case.
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.
Okay, I think that we can get away with this then. The root classification is a bit tricky as [A-Z]:
is not a root, [A-Z]:\
is a root, \\server
is not a root, but \\server\share
is a root.
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.
Oh that's interesting, I wasn't aware of the \\server
case. There's likely room for improvement on how we initialize a URL like URL(filePath: "\\server")
on Windows. Currently, this is treated as absolute (.absoluteString == "file:////server", .path == "//server"
), but it sounds like this might be relative instead?
I don't think it blocks this PR to strip the leading slash from Windows drive letter RFC paths, but it's something we should consider after.
* (133878310) URL.fileSystemPath should drop all trailing slashes (#852) * (133882014) URL(filePath: path, directoryHint: .notDirectory) should strip trailing slashes (#867) * (137129292) URL(filePath:) should not treat "~" as absolute (#961) * (137068266) URL.fileSystemPath should strip leading slash for Windows drive letters (#964) * (137287143) URL path extension APIs should strip trailing slashes (#965)
* (133878310) URL.fileSystemPath should drop all trailing slashes (#852) * (133882014) URL(filePath: path, directoryHint: .notDirectory) should strip trailing slashes (#867) * (137129292) URL(filePath:) should not treat "~" as absolute (#961) * (137068266) URL.fileSystemPath should strip leading slash for Windows drive letters (#964) * (137287143) URL path extension APIs should strip trailing slashes (#965)
On Windows,
URL.fileSystemPath
should ideally provide the path exactly how you would make filesystem calls with it, e.g. by stripping the leading slash from an RFC drive path like/C:/path
and by converting the slashes to backlashes.But given that
URL.fileSystemPath
is vended directly to (and synonymous with) the widely-usedURL.path
, we should start by just stripping the leading slash from drive letters for compatibility. This returnsURL.path
to its previous behavior on Windows.Resolves #857