Skip to content

Support long file names on Windows #45558

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

Closed
wants to merge 1 commit into from

Conversation

DemiMarie
Copy link
Contributor

@DemiMarie DemiMarie commented Oct 26, 2017

Windows does not support file names of >255 UTF-16 characters, unless
the path is prefixed with \\?\ or \??\. Both of those, however,
supress normalization. So Rust must do that normalization itself. This
commit makes the standard library do just that, by calling the Windows
API function GetFullPathNameW().

This implements rust-lang/rfcs#2188.

This also fixes a bug where a path beginning with \??\ on Windows was incorrectly marked as not being a verbatim path. That it is such was confirmed by reading the .NET CoreFX sources.

Windows does not support file names of >255 UTF-16 characters, unless
the path is prefixed with `\\?\` or `\??\`.  Both of those, however,
supress normalization.  So Rust must do that normalization itself.  This
commit makes the standard library do just that, by calling the Windows
API function GetFullPathNameW().
@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@kennytm kennytm added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 26, 2017
@retep998
Copy link
Member

retep998 commented Oct 27, 2017

One case that your PR does not seem to handle is device paths starting with \\.\. They are equivalent to \\?\ paths except for the fact that they don't have to be normalized. After normalizing them, swapping out the \\.\ for \\?\ should have the right result.

Also, there is no way I'd accept this being merged without an extensive set of tests covering every possible style of path. Because crater won't be able to effectively test the impact of this change, tests are even more critically important.

@shepmaster
Copy link
Member

Heya @DemiMarie — it's been over 7 days since we've heard from you. Will you have any time to address the request for more tests?

@shepmaster
Copy link
Member

Thanks for the PR, @DemiMarie! Unfortunately, we haven't heard from you in a few weeks. I'm going to close this PR for now to keep things tidy, but please feel free to re-open it once you've had a chance to address some of the feedback!

@shepmaster shepmaster closed this Nov 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants