Skip to content
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(filePath:) should resolve Windows drive-relative paths #1044

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jrflat
Copy link
Contributor

@jrflat jrflat commented Nov 12, 2024

When a URL is initialized with a Windows drive-relative path like URL(filePath: "S:relative/path"), we need to check the current working directory of the S: drive (not necessarily the cwd of the process) and use that as the base URL. Then, we should strip the S: from the relative path so that it resolves correctly against the base path.

E.g. if the current working directory of the S: drive is /current/directory/, we would get

let url = URL(filePath: "S:relative/path")
print(url.relativePath) // "relative/path"
print(url.baseURL?.path) // "S:/current/directory"
print(url.path) // "S:/current/directory/relative/path"

@jrflat
Copy link
Contributor Author

jrflat commented Nov 12, 2024

@swift-ci please test

Comment on lines +2228 to +2236
let isAbsolute: Bool
var iter = filePath.utf8.makeIterator()
if let driveLetter = iter.next(), driveLetter.isAlpha,
iter.next() == ._colon,
iter.next() != ._slash {
// Drive-relative path: use the current directory for the given drive letter
// as the base URL, and remove the drive letter from the relative path.
let relativePath = String(Substring(filePath.utf8.dropFirst(2)))
let basePath: String? = "\(Unicode.Scalar(driveLetter)):".withCString(encodedAs: UTF16.self) { pwszDriveLetter in
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this is better written as:

let bIsAbsolute: Bool = path.withNTPathRepresentation { !PathIsRelativeW($0) }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that sounds like a much nicer/robust replacement for the isAbsolute code, I'll do that, thanks!

I think we would still need to differentiate between C:relative\path and relative\path if we want to strip the C: before storing the relative path in URL, so we'll still need to do the alpha/colon/slash check here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I'm seeing PathIsRelativeW return FALSE for C:relative\path (probably because it's resolved against the absolute cwd for C:), so it might not work for this use case.

Copy link
Contributor Author

@jrflat jrflat Nov 13, 2024

Choose a reason for hiding this comment

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

We could rework the URL(filePath:) initializer to immediately standardize C:relative\path to its absolute representation, and not use the relative path + base URL distinction. This would essentially be:

bIsAbsolute = !PathIsRelativeW(path)
if bIsAbsolute {
  path = GetFullPathNameW(path)
  baseURL = nil
} else {
  // path is relative
  baseURL = URL.currentDirectory()
}

PathIsRelative("/slash/path") does return TRUE, though, so we'd need to make sure we strip the leading slash in other code when merging with the base path.

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.

2 participants