Skip to content

Conversation

12bitentropy
Copy link

Motivation

The component check only works on windows. If this was run on linux, the code will always push the component to the path_to_file variable, even if it contains c: in the path.

Solution

Use #[cfg(target_os = "windows")] to disable it

@jplatte
Copy link
Member

jplatte commented Apr 15, 2025

The particular example of c: indeed is not going to hit the early-exit branch on unix. That does not mean that this check should only happen on Windows though. I'm pretty sure it's possible to hit non-Normal path segments on non-Windows targets and I'd need to see strong proof that it's impossible for a particular target before this check for that target.

@jplatte jplatte closed this Apr 15, 2025
@12bitentropy
Copy link
Author

https://doc.rust-lang.org/std/path/enum.Component.html
It says that the prefix is a windows path prefix, so it should only happen on windows

@jplatte
Copy link
Member

jplatte commented Apr 17, 2025

I'm not following your reasoning. Actually I think this change may be fine because this is only for checking individual segments reinterpreted as separate paths, but it's really not obvious to me, and what the documentation about prefix says seems not very relevant to the discussion. (the code in question rejects anything that is not Normal, not just Prefix)

@jplatte jplatte reopened this Apr 17, 2025
@12bitentropy
Copy link
Author

I'm not following your reasoning. Actually I think this change may be fine because this is only for checking individual segments reinterpreted as separate paths, but it's really not obvious to me, and what the documentation about prefix says seems not very relevant to the discussion. (the code in question rejects anything that is not Normal, not just Prefix)

What I meant is that is does not matter if the reinterpreted prefix is pushed to the path buffer as the absolute behavior from #204 only happens on windows. This change should be fine and does the check if it's running on windows.

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