Skip to content
This repository was archived by the owner on Sep 9, 2020. It is now read-only.

Fixes #675. Changes to HasFilepathPrefix to handle Windows better. #704

Merged
merged 2 commits into from
Jun 4, 2017
Merged

Fixes #675. Changes to HasFilepathPrefix to handle Windows better. #704

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1cf15a6
Fixes #675. Changes to HasFilepathPrefix to handle Windows better.
mikijov Jun 1, 2017
5a7783f
Added comments explaining need for special Windows behaviour.
mikijov Jun 2, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions internal/fs/fs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,28 @@ import (
// will return true. The implementation is *not* OS-specific, so a FAT32
// filesystem mounted on Linux will be handled correctly.
func HasFilepathPrefix(path, prefix string) bool {
if filepath.VolumeName(path) != filepath.VolumeName(prefix) {
// this function is more convoluted then ideal due to need for special
// handling of volume name/drive letter on Windows. vnPath and vnPrefix
// are first compared, and then used to initialize initial values of p and
// d which will be appended to for incremental checks using
// isCaseSensitiveFilesystem and then equality.

// no need to check isCaseSensitiveFilesystem because VolumeName return
// empty string on all non-Windows machines
vnPath := strings.ToLower(filepath.VolumeName(path))
vnPrefix := strings.ToLower(filepath.VolumeName(prefix))
if vnPath != vnPrefix {
return false
}

// because filepath.Join("c:","dir") returns "c:dir", we have to manually add path separator to drive letters
if strings.HasSuffix(vnPath, ":") {
vnPath += string(os.PathSeparator)
}
if strings.HasSuffix(vnPrefix, ":") {
vnPrefix += string(os.PathSeparator)
}

var dn string

if isDir, err := IsDir(path); err != nil {
Expand All @@ -46,14 +64,17 @@ func HasFilepathPrefix(path, prefix string) bool {
dn = strings.TrimSuffix(dn, string(os.PathSeparator))
prefix = strings.TrimSuffix(prefix, string(os.PathSeparator))

// [1:] in the lines below eliminates empty string on *nix and volume name on Windows
dirs := strings.Split(dn, string(os.PathSeparator))[1:]
prefixes := strings.Split(prefix, string(os.PathSeparator))[1:]

if len(prefixes) > len(dirs) {
return false
}

var d, p string
// d,p are initialized with "" on *nix and volume name on Windows
d := vnPath
p := vnPrefix

for i := range prefixes {
// need to test each component of the path for
Expand Down
79 changes: 63 additions & 16 deletions internal/fs/fs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,37 +9,61 @@ import (
"os"
"path/filepath"
"runtime"
"strings"
"testing"

"github.com/golang/dep/internal/test"
)

// This function tests HadFilepathPrefix. It should test it on both case
// sensitive and insensitive situations. However, the only reliable way to test
// case-insensitive behaviour is if using case-insensitive filesystem. This
// cannot be guaranteed in an automated test. Therefore, the behaviour of the
// tests is not to test case sensitivity on *nix and to assume that Windows is
// case-insensitive. Please see link below for some background.
//
// https://superuser.com/questions/266110/how-do-you-make-windows-7-fully-case-sensitive-with-respect-to-the-filesystem
//
// NOTE: NTFS can be made case-sensitive. However many Windows programs,
// including Windows Explorer do not handle gracefully multiple files that
// differ only in capitalization. It is possible that this can cause these tests
// to fail on some setups.
func TestHasFilepathPrefix(t *testing.T) {
dir, err := ioutil.TempDir("", "dep")
if err != nil {
t.Fatal(err)
}
defer os.RemoveAll(dir)

// dir2 is the same as dir but with different capitalization on Windows to
// test case insensitivity
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, expanded comment coming up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

var dir2 string
if runtime.GOOS == "windows" {
dir = strings.ToLower(dir)
dir2 = strings.ToUpper(dir)
} else {
dir2 = dir
}

cases := []struct {
path string
prefix string
want bool
}{
{filepath.Join(dir, "a", "b"), filepath.Join(dir), true},
{filepath.Join(dir, "a", "b"), filepath.Join(dir, "a"), true},
{filepath.Join(dir, "a", "b"), filepath.Join(dir, "a", "b"), true},
{filepath.Join(dir, "a", "b"), filepath.Join(dir, "c"), false},
{filepath.Join(dir, "a", "b"), filepath.Join(dir, "a", "d", "b"), false},
{filepath.Join(dir, "a", "b"), filepath.Join(dir, "a", "b2"), false},
{filepath.Join(dir), filepath.Join(dir, "a", "b"), false},
{filepath.Join(dir, "ab"), filepath.Join(dir, "a", "b"), false},
{filepath.Join(dir, "ab"), filepath.Join(dir, "a"), false},
{filepath.Join(dir, "123"), filepath.Join(dir, "123"), true},
{filepath.Join(dir, "123"), filepath.Join(dir, "1"), false},
{filepath.Join(dir, "⌘"), filepath.Join(dir, "⌘"), true},
{filepath.Join(dir, "a"), filepath.Join(dir, "⌘"), false},
{filepath.Join(dir, "⌘"), filepath.Join(dir, "a"), false},
{filepath.Join(dir, "a", "b"), filepath.Join(dir2), true},
{filepath.Join(dir, "a", "b"), filepath.Join(dir2, "a"), true},
{filepath.Join(dir, "a", "b"), filepath.Join(dir2, "a", "b"), true},
{filepath.Join(dir, "a", "b"), filepath.Join(dir2, "c"), false},
{filepath.Join(dir, "a", "b"), filepath.Join(dir2, "a", "d", "b"), false},
{filepath.Join(dir, "a", "b"), filepath.Join(dir2, "a", "b2"), false},
{filepath.Join(dir), filepath.Join(dir2, "a", "b"), false},
{filepath.Join(dir, "ab"), filepath.Join(dir2, "a", "b"), false},
{filepath.Join(dir, "ab"), filepath.Join(dir2, "a"), false},
{filepath.Join(dir, "123"), filepath.Join(dir2, "123"), true},
{filepath.Join(dir, "123"), filepath.Join(dir2, "1"), false},
{filepath.Join(dir, "⌘"), filepath.Join(dir2, "⌘"), true},
{filepath.Join(dir, "a"), filepath.Join(dir2, "⌘"), false},
{filepath.Join(dir, "⌘"), filepath.Join(dir2, "a"), false},
}

for _, c := range cases {
Expand All @@ -57,13 +81,36 @@ func TestHasFilepathPrefix(t *testing.T) {
}
}

// This function tests HadFilepathPrefix. It should test it on both case
// sensitive and insensitive situations. However, the only reliable way to test
// case-insensitive behaviour is if using case-insensitive filesystem. This
// cannot be guaranteed in an automated test. Therefore, the behaviour of the
// tests is not to test case sensitivity on *nix and to assume that Windows is
// case-insensitive. Please see link below for some background.
//
// https://superuser.com/questions/266110/how-do-you-make-windows-7-fully-case-sensitive-with-respect-to-the-filesystem
//
// NOTE: NTFS can be made case-sensitive. However many Windows programs,
// including Windows Explorer do not handle gracefully multiple files that
// differ only in capitalization. It is possible that this can cause these tests
// to fail on some setups.
func TestHasFilepathPrefix_Files(t *testing.T) {
dir, err := ioutil.TempDir("", "dep")
if err != nil {
t.Fatal(err)
}
defer os.RemoveAll(dir)

// dir2 is the same as dir but with different capitalization on Windows to
// test case insensitivity
var dir2 string
if runtime.GOOS == "windows" {
dir = strings.ToLower(dir)
dir2 = strings.ToUpper(dir)
} else {
dir2 = dir
}

existingFile := filepath.Join(dir, "exists")
if _, err := os.Create(existingFile); err != nil {
t.Fatal(err)
Expand All @@ -76,8 +123,8 @@ func TestHasFilepathPrefix_Files(t *testing.T) {
prefix string
want bool
}{
{existingFile, filepath.Join(dir), false},
{nonExistingFile, filepath.Join(dir), true},
{existingFile, filepath.Join(dir2), false},
{nonExistingFile, filepath.Join(dir2), true},
}

for _, c := range cases {
Expand Down