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

Remove `arc::handle_prefix()` #21

Closed
Screwtapello opened this Issue Aug 2, 2018 · 6 comments

Comments

Projects
None yet
2 participants
@Screwtapello
Copy link
Collaborator

Screwtapello commented Aug 2, 2018

This helper was added to address a problem with handling Windows paths, but after investigation the problem seems to have gone away. As suggested, the code will be cleaner and simpler without that function.

@vitiral

This comment has been minimized.

Copy link
Owner

vitiral commented Aug 2, 2018

@Screwtapello

This comment has been minimized.

Copy link
Collaborator Author

Screwtapello commented Aug 3, 2018

I'm working on this right now, actually! I just figured I should have an issue number to mention in the commit that makes the change.

Sure, I'd love to help. :)

@vitiral

This comment has been minimized.

Copy link
Owner

vitiral commented Aug 3, 2018

Cool, you've been added as a collaborator.

I actually mistakenly thought this was the PR in the email. Carry on 😄

@Screwtapello

This comment has been minimized.

Copy link
Collaborator Author

Screwtapello commented Aug 7, 2018

A couple of days ago I discovered the problem that arc::handle_prefix() was intended to solve had not gone away, and I figured out how to reproduce it reliably. The following table is one I posted to rust-lang/rust#49342 but with a few more interesting test-cases I thought of in the mean time. As before, the intersection of each column and row represents the result of std::env::set_current_dir(column); Path::new(row).canonicalize() where "-" means "returned an error":

\\?\C:\ C:\ \\vboxsvr\Shared\
\\vboxsvr\Shared\ \\?\UNC\vboxsvr\Shared\ \\?\UNC\vboxsvr\Shared\ \\?\UNC\vboxsvr\Shared\
\\?\C:\Users \\?\C:\Users \\?\C:\Users \\?\C:\Users
C:\Users \\?\C:\Users \\?\C:\Users \\?\C:\Users
C:Users - \\?\C:\Users -
\Users - \\?\C:\Users \\?\UNC\vboxsvr\Shared\Users
Users \\?\C:\Users \\?\C:\Users \\?\UNC\vboxsvr\Shared\Users
C: \\?\C:\Users\IEUser \\?\C:\ \\?\C:\Users\IEUser
\ - \\?\C:\ \\?\UNC\vboxsvr\Shared\

When the current directory is a regular (C:\) or UNC (\\vboxsvr\Shared) path, you can canonicalize just about anything, so we don't have to do anything special to make canonicalization work for those cases.

The path C:Users fails to canonicalize for both the extended-length (\\?\C:\) and UNC paths, which makes sense. As the C: row reveals, in those cases the current directory on C: is "\Users\IEUser" which does not contain a Users directory, so .canonicalize() is correct to return an error in those situations. We don't have to do anything special to make canonicalization work for those cases (beyond the work we already have to do to support canonicalizing non-existent paths).

The paths \Users and \ fail to canonicalize when the current directory uses extended-length syntax. Luckily, we can easily implement this ourselves:

  • take the prefix from the current working directory (say, C:)
  • add a root component (C:\)
  • canonicalize that (\\?\C:\)
  • add the remainder of the input path to that head (\\?\C:\Users)

So, in every situation we can produce a canonical path head for an arbitrary input path, even on Windows, and even if the current working directory uses extended-length path syntax.

I'm gonna try to re-implement PathArc::absolute() like that, and we'll see how far I get.

@vitiral

This comment has been minimized.

Copy link
Owner

vitiral commented Aug 7, 2018

@Screwtapello

This comment has been minimized.

Copy link
Collaborator Author

Screwtapello commented Aug 8, 2018

Not strictly related to solving this issue, but this Raymond Chen blog post explains a whole bunch of the weirdness I've encountered with relative path resolution on windows. The short version is: unlike Unix, where the shell is a thin wrapper over the kernel's idea of a "process", Windows' cmd.exe lies all the time.

Screwtapello added a commit to Screwtapello/path_abs that referenced this issue Aug 14, 2018

Remove the `arc::handle_prefix()` helper.
This makes `PathArc::absolute()` simpler, and makes the
absolute_path_cannot_go_above_root test consistent between Windows and other
platforms.

Fixes vitiral#21.

Screwtapello added a commit that referenced this issue Sep 6, 2018

Remove the `arc::handle_prefix()` helper. (#27)
* Remove the `arc::handle_prefix()` helper.

This makes `PathArc::absolute()` simpler, and makes the
absolute_path_cannot_go_above_root test consistent between Windows and other
platforms.

Fixes #21.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.