Fix assertion error when a drive letter path is passed to command line

[rhysd]

Fix #367

Joining paths with Path::join does not always return an absolute path even if the left hand side is an absolute path. We need to check it before calling path::normalize.

[rhysd]

@lhecker Thanks for your review. I addressed the comment and force-pushed the change. Would you review it again?

[lhecker]

I think I understand what the issue is now and I believe this needs a fundamentally different approach. When you call Path::join or PathBuf::push in Rust and the joining path is "C:" it'll return just "C:" but that's not how Windows works. "C:" means "the current working directory, but with a prefix of C:". For instance, "D:\foo\bar" joining with "C:" should result in "C:\foo\bar". I'm not sure why Rust does it differently... I'll need a bit to think about this.

[lhecker]

One possible approach could be to replace normalize with a normalized_join method which joins a given absolute path and an absolute or relative path and forms a final absolute path. It would work just like PathBuf::push with the difference that we would resolve all . and all .. immediately without preserving them. We would also fix the behavior of path prefixes like C: that way.

[rhysd]

Yeah, that's the behavior I roughly explained in #367. Path::join behavior is surprising in this case (I don't know the reason though). Since drive letter paths like C: are special, I think Edit also should handle them specially. I'll reconsider the way on next Monday (in JST) because I don't have enough time today.

[jstarks]

I think I understand what the issue is now and I believe this needs a fundamentally different approach. When you call Path::join or PathBuf::push in Rust and the joining path is "C:" it'll return just "C:" but that's not how Windows works. "C:" means "the current working directory, but with a prefix of C:". For instance, "D:\foo\bar" joining with "C:" should result in "C:\foo\bar". I'm not sure why Rust does it differently... I'll need a bit to think about this.

That's not right. C: does not mean the current working directory with a prefix of C:. It means C:'s current working directory:

• If the current directory begins with C:, then this is just the current directory.
• Otherwise, the value of the =C: environment variable, if it exists and contains a valid path. (This environment variable is generally only set by shells such as cmd.exe. It's not set by SetCurrentDirectory. Ain't compat grand?)
• Otherwise,C:\

This is the behavior of the directory handling code in ntdll, shared across all Win32 APIs.

So, in this sense, C: is a third type of path, neither relative nor absolute--it is not (always) relative to the current path, but it is also does not specify a context-independent location within the file system.

So, Rust's join behavior seems fairly reasonable. Joining D:\foo\bar with C: should produce C:, and getting an absolute path back out of that will require an OS call to fetch the current directory and possibly environment. (You could make an argument that joining C:\foo\bar with C: should produce C:\foo\bar. And in some contexts, that might be what the user wants and expects. So maybe you'd want to special case that.)

[lhecker]

Oh, I see... Thank you for the correction! Since this method isn't bound to the behavior of others, I think we may just want to always go with the 3rd bullet point. I suspect that will be easy to implement and not that surprising of a behavior.

[rhysd]

Yeah, so I think we have two options:

• Remain the drive letter paths as-is like C: and make OS handle them (This is the behavior I originally implemented in this PR)
• Resolve the drive letter paths to absolute paths using some Win32 API

Which one should we take considering the Edit's behavior?

[lhecker]

I believe we should go with the 3rd options for now and simply append a \ on Windows if the result path is not absolute. We can then add a comment, that we may want to use GetFullPathNameW in the future, if this behavior ever becomes undesired. Calling GetFullPathNameW requires doing the WTF8 <> WTF16 conversion and I'd rather see first if anyone even notices the difference. 😅

[jstarks]

I believe we should go with the 3rd options for now and simply append a \ on Windows if the result path is not absolute. We can then add a comment, that we may want to use GetFullPathNameW in the future, if this behavior ever becomes undesired. Calling GetFullPathNameW requires doing the WTF8 <> WTF16 conversion and I'd rather see first if anyone even notices the difference. 😅

Note that std::path::absolute already calls GetFullPathNameW on Windows.

[lhecker]

Yep, exactly like the equivalent in C++. I'd of course also be fine with documenting it as "may want to use std::path::absolute on Windows" alternatively.

[jstarks]

Yep, exactly like the equivalent in C++.

If you say so. I left the C++ world before std::filesystem made it to my build environment 🙂.

[rhysd]

I believe we should go with the 3rd options for now and simply append a \ on Windows if the result path is not absolute.

Let me confirm the behavior before implementing it. I understood that C: is converted to C:\. However what happens if C:foo? C:foo\ is still not absolute. Do you intend C:\foo? (When the current directory is C:\a, C:b\c means C:\a\b\c)

[lhecker]

These are the kinds of Windows edge cases that I bet do not come up much in practice today, so whatever we do will be such a rare occurrence that the choice now doesn't matter much. I'll leave this up to you. I do have a specific approach in mind, however, in case you aren't sure yourself.

[rhysd]

Okay, I'll try to implement it. I think the workaround should be as simple as possible so I'll simply try to insert the path separator. Once the implementation is done I'll request your review.