From 1cf15a6ca0784191ee180c00a7ffe1966e2f33fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Milutin=20Jovanovic=CC=81?= Date: Thu, 1 Jun 2017 17:07:24 -0400 Subject: [PATCH 1/2] Fixes #675. Changes to HasFilepathPrefix to handle Windows better. 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 | 15 ++++++++++-- internal/fs/fs_test.go | 53 +++++++++++++++++++++++++++++------------- 2 files changed, 50 insertions(+), 18 deletions(-) diff --git a/internal/fs/fs.go b/internal/fs/fs.go index 36e7d089ed..50d83a762a 100644 --- a/internal/fs/fs.go +++ b/internal/fs/fs.go @@ -29,9 +29,19 @@ 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) { + // 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 } + if strings.HasSuffix(vnPath, ":") { + vnPath += string(os.PathSeparator) + } + if strings.HasSuffix(vnPrefix, ":") { + vnPrefix += string(os.PathSeparator) + } var dn string @@ -53,7 +63,8 @@ func HasFilepathPrefix(path, prefix string) bool { return false } - var d, p string + d := vnPath + p := vnPrefix for i := range prefixes { // need to test each component of the path for diff --git a/internal/fs/fs_test.go b/internal/fs/fs_test.go index f832b7346e..aa4b4ff96b 100644 --- a/internal/fs/fs_test.go +++ b/internal/fs/fs_test.go @@ -9,6 +9,7 @@ import ( "os" "path/filepath" "runtime" + "strings" "testing" "github.com/golang/dep/internal/test" @@ -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 + 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 { @@ -64,6 +75,16 @@ func TestHasFilepathPrefix_Files(t *testing.T) { } 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) @@ -76,8 +97,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 { From 5a7783f7a8f8213e972232bd6db8060f4e4c87fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Milutin=20Jovanovic=CC=81?= Date: Thu, 1 Jun 2017 23:03:28 -0400 Subject: [PATCH 2/2] Added comments explaining need for special Windows behaviour. --- internal/fs/fs.go | 10 ++++++++++ internal/fs/fs_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/internal/fs/fs.go b/internal/fs/fs.go index 50d83a762a..cd526f6401 100644 --- a/internal/fs/fs.go +++ b/internal/fs/fs.go @@ -29,6 +29,12 @@ 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 { + // 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)) @@ -36,6 +42,8 @@ func HasFilepathPrefix(path, prefix string) bool { 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) } @@ -56,6 +64,7 @@ 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:] @@ -63,6 +72,7 @@ func HasFilepathPrefix(path, prefix string) bool { return false } + // d,p are initialized with "" on *nix and volume name on Windows d := vnPath p := vnPrefix diff --git a/internal/fs/fs_test.go b/internal/fs/fs_test.go index aa4b4ff96b..d209eba3a5 100644 --- a/internal/fs/fs_test.go +++ b/internal/fs/fs_test.go @@ -15,6 +15,19 @@ import ( "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 { @@ -68,6 +81,19 @@ 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 {