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.

Sign up for GitHub

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

Fix relpath when path and startpath are in the same drive #40323

Merged
merged 2 commits into from
Apr 4, 2021
Merged

Fix relpath when path and startpath are in the same drive #40323

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
251c5dd
Fix relpath when path and startpath are in the same drive
musm Apr 2, 2021
a8036f5
Address code review
musm Apr 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions base/path.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,12 +517,20 @@ function relpath(path::String, startpath::String = ".")
curdir = "."
pardir = ".."
path == startpath && return curdir
path_drive, path_without_drive = splitdrive(path)
startpath_drive, startpath_without_drive = splitdrive(startpath)
path_arr = split(abspath(path_without_drive), path_separator_re)
start_arr = split(abspath(startpath_without_drive), path_separator_re)
if Sys.iswindows()
lowercase(path_drive) != lowercase(startpath_drive) && return abspath(path)
path_drive, path_without_drive = splitdrive(path)
startpath_drive, startpath_without_drive = splitdrive(startpath)
isempty(startpath_drive) && (startpath_drive = path_drive) # by default assume same as path drive

path_drive = uppercase(path_drive) # canonicalize drive letters to uppercasing for comparison
startpath_drive = uppercase(startpath_drive)

path_drive != startpath_drive && return abspath(path) # if drives differ return first path
path_arr = split(abspath(path_drive * path_without_drive), path_separator_re)
start_arr = split(abspath(startpath_drive * startpath_without_drive), path_separator_re)
Copy link
Member

Choose a reason for hiding this comment

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

Since path_drive may involve a DNS lookup, I think we should preserve the input case:

Suggested change
path_drive = uppercase(path_drive) # canonicalize drive letters to uppercasing for comparison
startpath_drive = uppercase(startpath_drive)
path_drive != startpath_drive && return abspath(path) # if drives differ return first path
path_arr = split(abspath(path_drive * path_without_drive), path_separator_re)
start_arr = split(abspath(startpath_drive * startpath_without_drive), path_separator_re)
startpath_drive = uppercase(startpath_drive) # canonicalize drive letters to uppercasing for comparison
uppercase(path_drive) == startpath_drive || return abspath(path) # if drives differ return first path
path_arr = split(abspath(path_drive * path_without_drive), path_separator_re)
start_arr = split(abspath(path_drive * startpath_without_drive), path_separator_re)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even better, thank you for the review. PR updated.

else
path_arr = split(abspath(path), path_separator_re)
start_arr = split(abspath(startpath), path_separator_re)
end
i = 0
while i < min(length(path_arr), length(start_arr))
Expand Down
4 changes: 4 additions & 0 deletions test/path.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,10 @@
# Additional cases
@test_throws ArgumentError relpath(S("$(sep)home$(sep)user$(sep)dir_withendsep$(sep)"), "")
@test_throws ArgumentError relpath(S(""), S("$(sep)home$(sep)user$(sep)dir_withendsep$(sep)"))

# issue 40237
path = "..$(sep)a$(sep)b$(sep)c"
@test relpath(abspath(path)) == path
end
test_relpath()
end
Expand Down