- const WCHAR fallbackpathW[] = { 'C',':','\',0 };
This doesn't look right, could we make it dynamic using actual system drive?
Same goes for tests, where you got hardcoded C:\ drive.
According to docs this function should follow symlinks, will it potentially work with this implementation? Was it tested?
I think it makes sense to submit tests first, maybe combining them a little, to have 1 tests patch instead of 4.
On Fri, Jun 19, 2015 at 11:49 AM, Nikolay Sivov bunglehead@gmail.com wrote:
- const WCHAR fallbackpathW[] = { 'C',':','\',0 };
This doesn't look right, could we make it dynamic using actual system drive?
It doesn't actually use the system drive, it uses C for some failures or the first character of the path: http://source.winehq.org/patches/data/112168 or the drive of the current directory: http://source.winehq.org/patches/data/112169 ^^ note: this conflicts with MSDN, but my tests on the testbot indicate that MSDN is wrong
Same goes for tests, where you got hardcoded C:\ drive.
All of those tests actually return a hard-coded C:\ value, only when we get to patch 5 do I add a test that actually depends on the drive of the current directory: http://source.winehq.org/patches/data/112169
According to docs this function should follow symlinks, will it potentially work with this implementation? Was it tested?
The behavior depends on the type of mounting, if you mount a volume directly at a location then it will return that location: C:\path\to\a\mountpoint\some\folder will become: C:\path\to\a\mountpoint If it's a junction point on the same drive then it will return: C:\ If it's a junction point to a different drive (say M:) then it will return M: on Windows. The current implementation will return "C:\path\to\a\mountpoint", which is enough to appease Steam. It's important to note that creating junction points to different drives is known to break a lot of applications, including MS applications.
I think it makes sense to submit tests first, maybe combining them a little, to have 1 tests patch instead of 4.
I split it up this way because it helped to demonstrate the special cases in the failure codepath (and allowed me to avoid adding a bunch of todo_wine calls unnecessarily). Since I've actually gone the trouble to acquire the knowledge about each of theses cases, I felt that it was more informative to include the test and the fix together rather than start with a dump of all the tests.
Best, Erich
-
Erich E. Hoover -
Nikolay Sivov