From e24eef4cb95ba3acfc98f5eca23835a379b412d6 Mon Sep 17 00:00:00 2001 From: Suhas Karanth Date: Sat, 2 Sep 2017 13:51:20 +0530 Subject: [PATCH] check cfg filename case on case insensitive systems - `fs` - Export `IsCaseSensitiveFilesystem`. Add and run test on windows, linux. - Add function `ReadActualFilenames` to read actual file names of given string slice. Add tests to be run on windows. - `project` - Add function `checkCfgFilenames` to check the filenames for manifest and lock have the expected case. Use `fs#IsCaseSensitiveFilesystem` for an early return as the check is costly. Add test to be run on windows. - `context` - Call `project#checkCfgFilenames` after resolving project root. Add test for invalid manifest file name to be run on windows. --- context.go | 5 +++ context_test.go | 45 +++++++++++++++++++++ internal/fs/fs.go | 89 ++++++++++++++++++++++++++++++++++++++--- internal/fs/fs_test.go | 91 +++++++++++++++++++++++++++++++++++++++--- project.go | 31 ++++++++++++++ project_test.go | 56 ++++++++++++++++++++++++++ 6 files changed, 307 insertions(+), 10 deletions(-) diff --git a/context.go b/context.go index feacfddab9..c0c8a1bd64 100644 --- a/context.go +++ b/context.go @@ -103,6 +103,11 @@ func (c *Ctx) LoadProject() (*Project, error) { return nil, err } + err = checkCfgFilenames(root) + if err != nil { + return nil, err + } + p := new(Project) if err = p.SetRoot(root); err != nil { diff --git a/context_test.go b/context_test.go index e522d25b4d..37a3a841e6 100644 --- a/context_test.go +++ b/context_test.go @@ -5,6 +5,7 @@ package dep import ( + "fmt" "io/ioutil" "log" "os" @@ -263,6 +264,50 @@ func TestLoadProjectNoSrcDir(t *testing.T) { } } +func TestLoadProjectCfgFileCase(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("skip this test on non-Windows") + } + + // Here we test that a manifest filename with incorrect case + // throws an error. Similar error will also be thrown for the + // lock file as well which has been tested in + // `project_test.go#TestCheckCfgFilenames`. So not repeating here. + + h := test.NewHelper(t) + defer h.Cleanup() + + invalidMfName := strings.ToLower(ManifestName) + + wd := filepath.Join("src", "test") + h.TempFile(filepath.Join(wd, invalidMfName), "") + + ctx := &Ctx{ + Out: discardLogger, + Err: discardLogger, + } + + err := ctx.SetPaths(h.Path(wd), h.Path(".")) + if err != nil { + t.Fatalf("%+v", err) + } + + _, err = ctx.LoadProject() + + if err == nil { + t.Fatal("should have returned 'Manifest Filename' error") + } + + expectedErrMsg := fmt.Sprintf( + "manifest filename '%s' does not match '%s'", + invalidMfName, ManifestName, + ) + + if err.Error() != expectedErrMsg { + t.Fatalf("unexpected error: %+v", err) + } +} + // TestCaseInsentitive is test for Windows. This should work even though set // difference letter cases in GOPATH. func TestCaseInsentitiveGOPATH(t *testing.T) { diff --git a/internal/fs/fs.go b/internal/fs/fs.go index abcebee6fe..381b39e75d 100644 --- a/internal/fs/fs.go +++ b/internal/fs/fs.go @@ -35,9 +35,9 @@ func HasFilepathPrefix(path, prefix string) bool { // 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. + // IsCaseSensitiveFilesystem and then equality. - // no need to check isCaseSensitiveFilesystem because VolumeName return + // 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)) @@ -84,7 +84,7 @@ func HasFilepathPrefix(path, prefix string) bool { // something like ext4 filesystem mounted on FAT // mountpoint, mounted on ext4 filesystem, i.e. the // problematic filesystem is not the last one. - if isCaseSensitiveFilesystem(filepath.Join(d, dirs[i])) { + if IsCaseSensitiveFilesystem(filepath.Join(d, dirs[i])) { d = filepath.Join(d, dirs[i]) p = filepath.Join(p, prefixes[i]) } else { @@ -140,7 +140,7 @@ func renameByCopy(src, dst string) error { return errors.Wrapf(os.RemoveAll(src), "cannot delete %s", src) } -// isCaseSensitiveFilesystem determines if the filesystem where dir +// IsCaseSensitiveFilesystem determines if the filesystem where dir // exists is case sensitive or not. // // CAVEAT: this function works by taking the last component of the given @@ -159,7 +159,7 @@ func renameByCopy(src, dst string) error { // If the input directory is such that the last component is composed // exclusively of case-less codepoints (e.g. numbers), this function will // return false. -func isCaseSensitiveFilesystem(dir string) bool { +func IsCaseSensitiveFilesystem(dir string) bool { alt := filepath.Join(filepath.Dir(dir), genTestFilename(filepath.Base(dir))) @@ -176,6 +176,85 @@ func isCaseSensitiveFilesystem(dir string) bool { return !os.SameFile(dInfo, aInfo) } +var errPathNotDir = errors.New("given path is not a directory") + +// ReadActualFilenames is used to determine the actual file names in given directory. +// +// On case sensitive file systems like ext4, it will check if those files exist using +// `os#Stat` and return a map with key and value as filenames which exist in the folder. +// +// Otherwise, it reads the contents of the directory +func ReadActualFilenames(dirPath string, names []string) (map[string]string, error) { + actualFilenames := make(map[string]string, len(names)) + if len(names) <= 0 { + // This isn't expected to happen for current usage. + // Adding edge case handling, maybe useful in future + return actualFilenames, nil + } + // First, check that the given path is valid and it is a directory + dirStat, err := os.Stat(dirPath) + if err != nil { + return nil, err + } + + if !dirStat.IsDir() { + return nil, errPathNotDir + } + + // Ideally, we would use `os#Stat` for getting the actual file names + // but that returns the name we passed in as an argument and not the actual filename. + // So we are forced to list the directory contents and check + // against that. Since this check is costly, we do it only if absolutely necessary. + if IsCaseSensitiveFilesystem(dirPath) { + // There will be no difference between actual filename and given filename + // So just check if those files exist. + for _, name := range names { + _, err := os.Stat(filepath.Join(dirPath, name)) + if err == nil { + actualFilenames[name] = name + } else if !os.IsNotExist(err) { + // Some unexpected err, return it. + return nil, err + } + } + return actualFilenames, nil + } + + dir, err := os.Open(dirPath) + if err != nil { + return nil, err + } + defer dir.Close() + + // Pass -1 to read all files in directory + files, err := dir.Readdir(-1) + if err != nil { + return nil, err + } + + // namesMap holds the mapping from lowercase name to search name. + // Using this, we can avoid repeatedly looping through names. + namesMap := make(map[string]string, len(names)) + for _, name := range names { + namesMap[strings.ToLower(name)] = name + } + + for _, file := range files { + if file.Mode().IsRegular() { + searchName, ok := namesMap[strings.ToLower(file.Name())] + if ok { + // We are interested in this file, case insensitive match successful + actualFilenames[searchName] = file.Name() + if len(actualFilenames) == len(names) { + // We found all that we were looking for + return actualFilenames, nil + } + } + } + } + return actualFilenames, nil +} + // genTestFilename returns a string with at most one rune case-flipped. // // The transformation is applied only to the first rune that can be diff --git a/internal/fs/fs_test.go b/internal/fs/fs_test.go index e16f772c9c..1b85fa3597 100644 --- a/internal/fs/fs_test.go +++ b/internal/fs/fs_test.go @@ -8,6 +8,7 @@ import ( "io/ioutil" "os" "path/filepath" + "reflect" "runtime" "strings" "testing" @@ -172,6 +173,85 @@ func TestRenameWithFallback(t *testing.T) { } } +func TestIsCaseSensitiveFilesystem(t *testing.T) { + if runtime.GOOS != "windows" && runtime.GOOS != "linux" { + t.Skip("skip this test on non-Windows, non-Linux") + } + + dir, err := ioutil.TempDir("", "TestCaseSensitivity") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir) + + isSensitive := IsCaseSensitiveFilesystem(dir) + isWindows := runtime.GOOS == "windows" + + if !isSensitive && !isWindows { + t.Fatal("expected isSensitive to be true on linux") + } else if isSensitive && isWindows { + t.Fatal("expected isSensitive to be false on windows") + } +} + +func TestReadActualFilenames(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("skip this test on non-Windows") + } + + h := test.NewHelper(t) + defer h.Cleanup() + + h.TempDir("") + tmpPath := h.Path(".") + + _, err := ReadActualFilenames(filepath.Join(tmpPath, "does_not_exists"), []string{""}) + switch { + case err == nil: + t.Fatal("expected err for non-existing folder") + case !os.IsNotExist(err): + t.Fatalf("unexpected error: %+v", err) + } + h.TempFile("tmpFile", "") + _, err = ReadActualFilenames(h.Path("tmpFile"), []string{""}) + switch { + case err == nil: + t.Fatal("expected err for passing file instead of directory") + case err != errPathNotDir: + t.Fatalf("unexpected error: %+v", err) + } + + cases := []struct { + createFiles []string + names []string + want map[string]string + }{ + {nil, nil, map[string]string{}}, { + []string{"test1.txt"}, + []string{"Test1.txt"}, + map[string]string{"Test1.txt": "test1.txt"}, + }, { + []string{"test2.txt", "test3.TXT"}, + []string{"test2.txt", "Test3.txt", "Test4.txt"}, + map[string]string{ + "test2.txt": "test2.txt", + "Test3.txt": "test3.TXT", + "Test4.txt": "", + }, + }, + } + for _, c := range cases { + for _, file := range c.createFiles { + h.TempFile(file, "") + } + got, err := ReadActualFilenames(tmpPath, c.names) + if err != nil { + t.Fatalf("unexpected error: %+v", err) + } + reflect.DeepEqual(c.want, got) + } +} + func TestGenTestFilename(t *testing.T) { cases := []struct { str string @@ -197,11 +277,11 @@ func TestGenTestFilename(t *testing.T) { func BenchmarkGenTestFilename(b *testing.B) { cases := []string{ - "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", - "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", - "αααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααααα", - "11111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111", - "⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘⌘", + strings.Repeat("a", 128), + strings.Repeat("A", 128), + strings.Repeat("α", 128), + strings.Repeat("1", 128), + strings.Repeat("⌘", 128), } for i := 0; i < b.N; i++ { @@ -556,6 +636,7 @@ func TestCopyFileLongFilePath(t *testing.T) { h := test.NewHelper(t) h.TempDir(".") + defer h.Cleanup() tmpPath := h.Path(".") diff --git a/project.go b/project.go index 6e5ffaae21..f8f5e89b9e 100644 --- a/project.go +++ b/project.go @@ -41,6 +41,37 @@ func findProjectRoot(from string) (string, error) { } } +// checkCfgFilenames validates filename case for the manifest and lock files. +// This is relevant on case-insensitive systems like Windows. +func checkCfgFilenames(projectRoot string) error { + // ReadActualFilenames is actually costly. Since this check is not relevant + // to case-sensitive filesystems like ext4, try for an early return. + if fs.IsCaseSensitiveFilesystem(projectRoot) { + return nil + } + + actualFilenames, err := fs.ReadActualFilenames(projectRoot, []string{ManifestName, LockName}) + + if err != nil { + return err + } + + // Since this check is done after `findProjectRoot`, we can assume that + // manifest file will be present. Even if it is not, error will still be thrown. + // But error message will be a tad inaccurate. + actualMfName := actualFilenames[ManifestName] + if actualMfName != ManifestName { + return fmt.Errorf("manifest filename '%s' does not match '%s'", actualMfName, ManifestName) + } + + actualLfName := actualFilenames[LockName] + if len(actualLfName) > 0 && actualLfName != LockName { + return fmt.Errorf("lock filename '%s' does not match '%s'", actualLfName, LockName) + } + + return nil +} + // A Project holds a Manifest and optional Lock for a project. type Project struct { // AbsRoot is the absolute path to the root directory of the project. diff --git a/project_test.go b/project_test.go index d2dedece7d..9d810b5869 100644 --- a/project_test.go +++ b/project_test.go @@ -5,9 +5,11 @@ package dep import ( + "fmt" "os" "path/filepath" "runtime" + "strings" "testing" "github.com/golang/dep/internal/gps" @@ -61,6 +63,60 @@ func TestFindRoot(t *testing.T) { } } +func TestCheckCfgFilenames(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("skip this test on non-Windows") + } + + errMsgFor := func(filetype, filename string) func(string) string { + return func(name string) string { + return fmt.Sprintf("%s filename '%s' does not match '%s'", filetype, name, filename) + } + } + + manifestErrMsg := errMsgFor("manifest", ManifestName) + lockErrMsg := errMsgFor("lock", LockName) + + invalidMfName := strings.ToLower(ManifestName) + invalidLfName := strings.ToLower(LockName) + + cases := []struct { + errExpected bool + createFiles []string + errMsg string + }{ + {false, []string{ManifestName}, ""}, + {false, []string{ManifestName, LockName}, ""}, + {true, nil, manifestErrMsg("")}, + {true, []string{invalidMfName}, manifestErrMsg(invalidMfName)}, + {true, []string{ManifestName, invalidLfName}, lockErrMsg(invalidLfName)}, + } + + for _, c := range cases { + h := test.NewHelper(t) + defer h.Cleanup() + + h.TempDir("") + tmpPath := h.Path(".") + + for _, file := range c.createFiles { + h.TempFile(file, "") + } + err := checkCfgFilenames(tmpPath) + + if c.errExpected { + if err == nil { + t.Fatalf("did not get expected error - '%s'", c.errMsg) + } else if err.Error() != c.errMsg { + t.Fatalf("expected message was '%s' but got '%s'", c.errMsg, err.Error()) + } + } else if err != nil { + // Error was not expected but still we got one + t.Fatalf("unexpected error: %+v", err) + } + } +} + func TestProjectMakeParams(t *testing.T) { m := NewManifest() m.Ignored = []string{"ignoring this"}