Fixes #675. Changes to HasFilepathPrefix to handle Windows better. #704
Conversation
Old logic lost the drive letter hence subsequent comparisons were often wrong. New code adds logic to handle volume name separately. Test changed to always have mismatching case on windows.
internal/fs/fs.go
Outdated
| @@ -53,7 +63,8 @@ func HasFilepathPrefix(path, prefix string) bool { | |||
| return false | |||
| } | |||
|
|
|||
| var d, p string | |||
| d := vnPath | |||
There was a problem hiding this comment.
Given that we know that Windows will always treat volume name as case insensitive, this seems unnecessary - why isn't it sufficient to just test the strings.ToLower() volume names, as you do above?
There was a problem hiding this comment.
It's a combination of many small things. strings.Split later on splits the path. On Windows dirs[0] is the drive letter, on Linux it's empty string. Because of that whoever wrote the function ignored the first element by slicing dirs = (...)[1:] it away. On *nix this is perfect. But on Windows loss of the first element meant that isCaseSensitive later on does not have the full path, i.e. C:\Windows becomes only Windows, and isCaseSensitive becomes unpredictable. I honestly do not know how did the tests previously work. In any case, I had to change the code to remember the volume name and initialize d and p with it. One more thing here. filepath.Join("c:", "Windows") returns "c:Windows". So I had to check if volume name is a drive letter (ending with ':') and only in that situation append \.
Sorry the explanation is so convoluted, but it's small idiosyncrasies that caused this not to be so trivial.
| @@ -21,25 +22,35 @@ func TestHasFilepathPrefix(t *testing.T) { | |||
| } | |||
| defer os.RemoveAll(dir) | |||
|
|
|||
| // dir2 is the same as dir but with different capitalization on Windows to | |||
| // test case insensitivity | |||
There was a problem hiding this comment.
As noted in the issue discussion, with the exception of the volume name (i guess?), case insensitivity isn't a windows property, but actually a filesystem property. (And within that, I guess, a property of how CreateFile() is called - whether FILE_FLAG_POSIX_SEMANTICS is set). This all suggests to me that some Windows configurations could fail on this test.
That seems like a rare case, though, and this does add coverage for case insensitivity in a way that makes our tests more robust. So, I'll accept it. However, please expand the comment a bit to note that failure may be possible on Windows.
There was a problem hiding this comment.
I could have elaborated more in my comment. I did a bit of research and the information is not very thorough. The best explanation of the situation is in the answers here.
How I read this and other material I have seen is that while NTFS itself is case-sensitive, Windows is not. I.e. you can create a file with specific capitalization, but Windows will not let you create another one with different capitalization.
Now, your point is also well made. The reason I decided to assume case-insensitivity is because I see some value in it as-is, and on the other hand I am not sure how I would write this test properly as I we don't know what file-system temp directory is on.
There was a problem hiding this comment.
BTW, expanded comment coming up.
There was a problem hiding this comment.
The reason I decided to assume case-insensitivity is because I see some value in it as-is, and on the other hand I am not sure how I would write this test properly as I we don't know what file-system temp directory is on.
Yep, agreed with all of this - this is why I'm board with testing it this way. Even though it seems strictly possible there could be errors, restricting to Windows is the best heuristic we have.
There was a problem hiding this comment.
I added the comments you asked for (and some more). I do not know if you get notified of commits or I have to call them out in comments like this...
|
OK cool, that'll do it 😄 - thanks, and yay first PR! 🎉 |
Fixes #675. Old logic lost the drive letter hence subsequent comparisons
were often wrong. New code adds logic to handle volume name
separately. Test changed to always have mismatching case on
windows.