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

Do not abuse PathBuf::push("") for appending directory separator #4

Closed
wants to merge 1 commit into from

Conversation

seanyoung
Copy link

Since rust-lang/rust#89270 rust's pathbuf
push() normalizes the path if it is a verbatim path, so that the result
is still a valid verbatim path. This involves parsing reconstructing the
path.

Appending a path separator using push("") will no longer work, and if
it did, it would be very inefficient.

rust-lang/rust#89658

Signed-off-by: Sean Young sean@mess.org

Since rust-lang/rust#89270 rust's pathbuf
push() normalizes the path if it is a verbatim path, so that the result
is still a valid verbatim path. This involves parsing reconstructing the
path.

Appending a path separator using push("") will no longer work, and if
it did, it would be very inefficient.

rust-lang/rust#89658

Signed-off-by: Sean Young <sean@mess.org>
@dylni
Copy link
Owner

dylni commented Oct 8, 2021

Thank you for the fix, but unfortunately, this will not work in all cases.

Pushing ABC to X:\X: should result in X:\X:\ABC but would be X:\X:ABC with this PR. There is not currently a test for this case, but I will be adding one.

Appending a path separator using push("") will no longer work, and if it did, it would be very inefficient.

Unfortunately, this actually is the most efficient implementation currently. This crate is very careful to avoid converting base to a string, since that would mean BasePath::push is effectively cloning base every time it's called. The above test case is another reason why the checks done by Path::push are necessary.

rust-lang/89665 should fix this issue, so I will close this for now. However, given that rust-lang/89270 was accepted, it may be useful to make other fixes from this crate in the standard library as well when I have time.

@dylni dylni closed this Oct 8, 2021
@seanyoung
Copy link
Author

Thank you for the fix, but unfortunately, this will not work in all cases.

Pushing ABC to X:\X: should result in X:\X:\ABC but would be X:\X:ABC with this PR. There is not currently a test for this case, but I will be adding one.

I considered that, but colons are no allowed in files/directories so I did not think this is an issue.

@dylni
Copy link
Owner

dylni commented Oct 11, 2021

That's true, but I think \\?\X:\X: can be a valid path for some uses. Either way, it would be better for pushing methods to behave in an expected way even for invalid paths.

This has been temporarily fixed in version 0.3.1.

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