From afaf870131a3470f36a3d93bf3b162a0f2db5542 Mon Sep 17 00:00:00 2001 From: Suhas Karanth Date: Sat, 2 Sep 2017 13:51:20 +0530 Subject: [PATCH 1/3] check cfg filename case on case insensitive systems - `fs` - Export `IsCaseSensitiveFilesystem`. Add and run test on windows, linux and macOS. - Add function `ReadActualFilenames` to read actual file names of given string slice. Add tests to be run on windows and macOS. - `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 and macOS. - `context` - Call `project.go#checkCfgFilenames` after resolving project root. Add test for invalid manifest file name to be run on windows and macOS. --- context.go | 5 ++ context_test.go | 45 +++++++++++++++++ internal/fs/fs.go | 95 +++++++++++++++++++++++++++++++++--- internal/fs/fs_test.go | 107 ++++++++++++++++++++++++++++++++++++++--- project.go | 39 +++++++++++++++ project_test.go | 56 +++++++++++++++++++++ 6 files changed, 335 insertions(+), 12 deletions(-) diff --git a/context.go b/context.go index 14b26b1853..6810777adf 100644 --- a/context.go +++ b/context.go @@ -105,6 +105,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 c08d1e658d..41533da52a 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" && runtime.GOOS != "darwin" { + t.Skip("skip this test on non-Windows, non-macOS") + } + + // 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 0fb84a8e39..913798799b 100644 --- a/internal/fs/fs.go +++ b/internal/fs/fs.go @@ -35,9 +35,9 @@ func HasFilepathPrefix(path, prefix string) (bool, error) { // 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)) @@ -82,7 +82,7 @@ func HasFilepathPrefix(path, prefix string) (bool, error) { // something like ext4 filesystem mounted on FAT // mountpoint, mounted on ext4 filesystem, i.e. the // problematic filesystem is not the last one. - caseSensitive, err := isCaseSensitiveFilesystem(filepath.Join(d, dirs[i])) + caseSensitive, err := IsCaseSensitiveFilesystem(filepath.Join(d, dirs[i])) if err != nil { return false, errors.Wrap(err, "failed to check filepath prefix") } @@ -135,7 +135,7 @@ func EquivalentPaths(p1, p2 string) (bool, error) { } if p1Filename != "" || p2Filename != "" { - caseSensitive, err := isCaseSensitiveFilesystem(filepath.Join(p1, p1Filename)) + caseSensitive, err := IsCaseSensitiveFilesystem(filepath.Join(p1, p1Filename)) if err != nil { return false, errors.Wrap(err, "could not check for filesystem case-sensitivity") } @@ -193,7 +193,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 @@ -212,7 +212,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, error) { +func IsCaseSensitiveFilesystem(dir string) (bool, error) { alt := filepath.Join(filepath.Dir(dir), genTestFilename(filepath.Base(dir))) dInfo, err := os.Stat(dir) @@ -264,6 +264,89 @@ func genTestFilename(str string) string { }, str) } +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, errors.Wrap(err, "failed to read actual filenames") + } + + 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. + caseSensitive, err := IsCaseSensitiveFilesystem(dirPath) + if err != nil { + return nil, errors.Wrap(err, "failed to read actual filenames") + } + if caseSensitive { + // 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, errors.Wrap(err, "failed to read actual filenames") + } + } + return actualFilenames, nil + } + + dir, err := os.Open(dirPath) + if err != nil { + return nil, errors.Wrap(err, "failed to read actual filenames") + } + defer dir.Close() + + // Pass -1 to read all files in directory + files, err := dir.Readdir(-1) + if err != nil { + return nil, errors.Wrap(err, "failed to read actual filenames") + } + + // 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 +} + var ( errSrcNotDir = errors.New("source is not a directory") errDstExist = errors.New("destination already exists") diff --git a/internal/fs/fs_test.go b/internal/fs/fs_test.go index 920d925bb1..6c002f971e 100644 --- a/internal/fs/fs_test.go +++ b/internal/fs/fs_test.go @@ -8,11 +8,13 @@ import ( "io/ioutil" "os" "path/filepath" + "reflect" "runtime" "strings" "testing" "github.com/golang/dep/internal/test" + "github.com/pkg/errors" ) // This function tests HadFilepathPrefix. It should test it on both case @@ -169,7 +171,7 @@ func TestEquivalentPaths(t *testing.T) { {strings.ToLower(h.Path("dir")), strings.ToUpper(h.Path("dir")), false, true, true}, } - caseSensitive, err := isCaseSensitiveFilesystem(h.Path("dir")) + caseSensitive, err := IsCaseSensitiveFilesystem(h.Path("dir")) if err != nil { t.Fatal("unexpcted error:", err) } @@ -229,6 +231,98 @@ func TestRenameWithFallback(t *testing.T) { } } +func TestIsCaseSensitiveFilesystem(t *testing.T) { + isLinux := runtime.GOOS == "linux" + isWindows := runtime.GOOS == "windows" + isMacOS := runtime.GOOS == "darwin" + + if !isLinux && !isWindows && !isMacOS { + t.Skip("Run this test on Windows, Linux and macOS only") + } + + dir, err := ioutil.TempDir("", "TestCaseSensitivity") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir) + + var want bool + if isLinux { + want = true + } else { + want = false + } + + got, err := IsCaseSensitiveFilesystem(dir) + + if err != nil { + t.Fatalf("unexpected error message: \n\t(GOT) %+v", err) + } + + if want != got { + t.Fatalf("unexpected value returned: \n\t(GOT) %t\n\t(WNT) %t", got, want) + } +} + +func TestReadActualFilenames(t *testing.T) { + if runtime.GOOS != "windows" && runtime.GOOS != "darwin" { + t.Skip("skip this test on non-Windows, non-macOS") + } + + 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(errors.Cause(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", + }, + }, + } + 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) + } + if !reflect.DeepEqual(c.want, got) { + t.Fatalf("returned value does not match expected: \n\t(GOT) %v\n\t(WNT) %v", got, c.want) + } + } +} + func TestGenTestFilename(t *testing.T) { cases := []struct { str string @@ -254,11 +348,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++ { @@ -613,6 +707,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..f2168aeaec 100644 --- a/project.go +++ b/project.go @@ -11,6 +11,7 @@ import ( "github.com/golang/dep/internal/fs" "github.com/golang/dep/internal/gps" + "github.com/pkg/errors" ) var ( @@ -41,6 +42,44 @@ 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. + caseSensitive, err := fs.IsCaseSensitiveFilesystem(projectRoot) + if err != nil { + return errors.Wrap(err, "could not check validity of configuration filenames") + } + if caseSensitive { + return nil + } + + actualFilenames, err := fs.ReadActualFilenames(projectRoot, []string{ManifestName, LockName}) + + if err != nil { + return errors.Wrap(err, "could not check validity of configuration filenames") + } + + // 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) + } + + // If a file is not found, the string map returned by `fs.ReadActualFilenames` + // will not have an entry for the given filename. Since the lock file + // is optional, we should check for equality only if it was found. + 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..b3599f1348 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" && runtime.GOOS != "darwin" { + t.Skip("skip this test on non-Windows, non-macOS") + } + + 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 { + wantErr bool + createFiles []string + wantErrMsg 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.wantErr { + if err == nil { + t.Fatalf("unexpected error message: \n\t(GOT) nil\n\t(WNT) %s", c.wantErrMsg) + } else if err.Error() != c.wantErrMsg { + t.Fatalf("unexpected error message: \n\t(GOT) %s\n\t(WNT) %s", err.Error(), c.wantErrMsg) + } + } else if err != nil { + // Error was not expected but still we got one + t.Fatalf("unexpected error message: \n\t(GOT) %+v", err) + } + } +} + func TestProjectMakeParams(t *testing.T) { m := NewManifest() m.Ignored = []string{"ignoring this"} From 28f4f359fe319e3d9f5f71b81c41bb615ef58a99 Mon Sep 17 00:00:00 2001 From: Suhas Karanth Date: Tue, 5 Sep 2017 10:08:06 +0530 Subject: [PATCH 2/3] refactor requested changes, minor improvements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `project` - `checkCfgFilenames` - Improve function and code comments - Use boolean value indicating whether value was found in actual filenames. If manifest file was not found, return `errProjectNotFound`. Use boolean to determine if lock file was not found instead of length check. - `TestCheckCfgFilenames` - Add code comments for test cases explaining the expected behavior - `fs` - `ReadActualFilenames` - Use cleaner check(`<=` ➡ `==`) to see if `names` parameter for `ReadActualFilenames` actually has any values. - Use `Readdirnames` instead of `Readdir`. This is expected to perform better in most cases. - `TestReadActualFilenames` - Add code comments for test cases explaining the expected behavior - general - Make line length for code comments consistent(90), add periods where appropriate. - String formatting, use %q instead of '%s' --- context_test.go | 7 +++--- internal/fs/fs.go | 49 +++++++++++++++++++++--------------------- internal/fs/fs_test.go | 18 ++++++++++++++-- project.go | 36 +++++++++++++++++++------------ project_test.go | 19 ++++++++++++++-- 5 files changed, 82 insertions(+), 47 deletions(-) diff --git a/context_test.go b/context_test.go index 41533da52a..9f01a9f940 100644 --- a/context_test.go +++ b/context_test.go @@ -269,9 +269,8 @@ func TestLoadProjectCfgFileCase(t *testing.T) { t.Skip("skip this test on non-Windows, non-macOS") } - // 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 + // 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) @@ -299,7 +298,7 @@ func TestLoadProjectCfgFileCase(t *testing.T) { } expectedErrMsg := fmt.Sprintf( - "manifest filename '%s' does not match '%s'", + "manifest filename %q does not match %q", invalidMfName, ManifestName, ) diff --git a/internal/fs/fs.go b/internal/fs/fs.go index 913798799b..40695c2d2a 100644 --- a/internal/fs/fs.go +++ b/internal/fs/fs.go @@ -271,12 +271,13 @@ var errPathNotDir = errors.New("given path is not a 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 +// Otherwise, it reads the contents of the directory and returns a map which has the +// given file name as the key and actual filename as the value if it was found. 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 + if len(names) == 0 { + // This isn't expected to happen for current usage. Adding edge case handling, + // as it may be useful in future. return actualFilenames, nil } // First, check that the given path is valid and it is a directory @@ -289,23 +290,23 @@ func ReadActualFilenames(dirPath string, names []string) (map[string]string, err 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. + // 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. caseSensitive, err := IsCaseSensitiveFilesystem(dirPath) if err != nil { return nil, errors.Wrap(err, "failed to read actual filenames") } if caseSensitive { - // There will be no difference between actual filename and given filename - // So just check if those files exist. + // 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. + // Some unexpected err, wrap and return it. return nil, errors.Wrap(err, "failed to read actual filenames") } } @@ -318,29 +319,27 @@ func ReadActualFilenames(dirPath string, names []string) (map[string]string, err } defer dir.Close() - // Pass -1 to read all files in directory - files, err := dir.Readdir(-1) + // Pass -1 to read all filenames in directory + filenames, err := dir.Readdirnames(-1) if err != nil { return nil, errors.Wrap(err, "failed to read actual filenames") } - // namesMap holds the mapping from lowercase name to search name. - // Using this, we can avoid repeatedly looping through names. + // 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 - } + for _, filename := range filenames { + searchName, ok := namesMap[strings.ToLower(filename)] + if ok { + // We are interested in this file, case insensitive match successful. + actualFilenames[searchName] = filename + if len(actualFilenames) == len(names) { + // We found all that we were looking for. + return actualFilenames, nil } } } diff --git a/internal/fs/fs_test.go b/internal/fs/fs_test.go index 6c002f971e..f4e26bfb74 100644 --- a/internal/fs/fs_test.go +++ b/internal/fs/fs_test.go @@ -275,10 +275,12 @@ func TestReadActualFilenames(t *testing.T) { h.TempDir("") tmpPath := h.Path(".") + // First, check the scenarios for which we expect an error. _, err := ReadActualFilenames(filepath.Join(tmpPath, "does_not_exists"), []string{""}) switch { case err == nil: t.Fatal("expected err for non-existing folder") + // use `errors.Cause` because the error is wrapped and returned case !os.IsNotExist(errors.Cause(err)): t.Fatalf("unexpected error: %+v", err) } @@ -296,11 +298,23 @@ func TestReadActualFilenames(t *testing.T) { names []string want map[string]string }{ - {nil, nil, map[string]string{}}, { + // If we supply no filenames to the function, it should return an empty map. + {nil, nil, map[string]string{}}, + // If the directory contains the given file with different case, it should return + // a map which has the given filename as the key and actual filename as the value. + { []string{"test1.txt"}, []string{"Test1.txt"}, map[string]string{"Test1.txt": "test1.txt"}, - }, { + }, + // 1. If the given filename is same as the actual filename, map should have the + // same key and value for the file. + // 2. If the given filename is present with different case for file extension, + // it should return a map which has the given filename as the key and actual + // filename as the value. + // 3. If the given filename is not present even with a different case, the map + // returned should not have an entry for that filename. + { []string{"test2.txt", "test3.TXT"}, []string{"test2.txt", "Test3.txt", "Test4.txt"}, map[string]string{ diff --git a/project.go b/project.go index f2168aeaec..48a554332d 100644 --- a/project.go +++ b/project.go @@ -43,10 +43,16 @@ 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. +// +// This is relevant on case-insensitive file systems like Windows and macOS. +// +// If manifest file is not found, it returns an error indicating the project could not be +// found. If it is found but the case does not match, an error is returned. If a lock +// file is not found, no error is returned as lock file is optional. If it is found but +// the case does not match, an error is returned. 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. + // ReadActualFilenames is actually costly. Since this check is not relevant to + // case-sensitive filesystems like ext4(linux), try for an early return. caseSensitive, err := fs.IsCaseSensitiveFilesystem(projectRoot) if err != nil { return errors.Wrap(err, "could not check validity of configuration filenames") @@ -61,20 +67,22 @@ func checkCfgFilenames(projectRoot string) error { return errors.Wrap(err, "could not check validity of configuration filenames") } - // 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] + actualMfName, found := actualFilenames[ManifestName] + if !found { + // Ideally this part of the code won't ever be executed if it is called after + // `findProjectRoot`. But be thorough and handle it anyway. + return errProjectNotFound + } if actualMfName != ManifestName { - return fmt.Errorf("manifest filename '%s' does not match '%s'", actualMfName, ManifestName) + return fmt.Errorf("manifest filename %q does not match %q", actualMfName, ManifestName) } - // If a file is not found, the string map returned by `fs.ReadActualFilenames` - // will not have an entry for the given filename. Since the lock file - // is optional, we should check for equality only if it was found. - actualLfName := actualFilenames[LockName] - if len(actualLfName) > 0 && actualLfName != LockName { - return fmt.Errorf("lock filename '%s' does not match '%s'", actualLfName, LockName) + // If a file is not found, the string map returned by `fs.ReadActualFilenames` will + // not have an entry for the given filename. Since the lock file is optional, we + // should check for equality only if it was found. + actualLfName, found := actualFilenames[LockName] + if found && actualLfName != LockName { + return fmt.Errorf("lock filename %q does not match %q", actualLfName, LockName) } return nil diff --git a/project_test.go b/project_test.go index b3599f1348..c76702d03a 100644 --- a/project_test.go +++ b/project_test.go @@ -70,7 +70,7 @@ func TestCheckCfgFilenames(t *testing.T) { 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) + return fmt.Sprintf("%s filename %q does not match %q", filetype, name, filename) } } @@ -85,10 +85,20 @@ func TestCheckCfgFilenames(t *testing.T) { createFiles []string wantErrMsg string }{ + // No error should be returned when the project contains a valid manifest file + // but no lock file. {false, []string{ManifestName}, ""}, + // No error should be returned when the project contains a valid manifest file as + // well as a valid lock file. {false, []string{ManifestName, LockName}, ""}, - {true, nil, manifestErrMsg("")}, + // Error indicating the project was not found should be returned if a manifest + // file is not found. + {true, nil, errProjectNotFound.Error()}, + // Error should be returned if the project has a manifest file with invalid name + // but no lock file. {true, []string{invalidMfName}, manifestErrMsg(invalidMfName)}, + // Error should be returned if the project has a valid manifest file and an + // invalid lock file. {true, []string{ManifestName, invalidLfName}, lockErrMsg(invalidLfName)}, } @@ -96,9 +106,12 @@ func TestCheckCfgFilenames(t *testing.T) { h := test.NewHelper(t) defer h.Cleanup() + // Create a temporary directory which we will use as the project folder. h.TempDir("") tmpPath := h.Path(".") + // Create any files that are needed for the test before invoking + // `checkCfgFilenames`. for _, file := range c.createFiles { h.TempFile(file, "") } @@ -106,8 +119,10 @@ func TestCheckCfgFilenames(t *testing.T) { if c.wantErr { if err == nil { + // We were expecting an error but did not get one. t.Fatalf("unexpected error message: \n\t(GOT) nil\n\t(WNT) %s", c.wantErrMsg) } else if err.Error() != c.wantErrMsg { + // We got an error but it is not the one we were expecting. t.Fatalf("unexpected error message: \n\t(GOT) %s\n\t(WNT) %s", err.Error(), c.wantErrMsg) } } else if err != nil { From d451fa03fcb19e38e9141a4a322bcdf328eeb4e8 Mon Sep 17 00:00:00 2001 From: Suhas Karanth Date: Wed, 20 Sep 2017 11:30:34 +0530 Subject: [PATCH 3/3] rename func, improve comments - `project.go` - Rename method {checkCfgFilenames => checkGopkgFilenames} - Improve funciton comment as suggested by @sdboyer - Fix ambigious comment explaining rationale behind early return. - Add comment explaining why we do not use `fs.IsCaseSensitiveFilesystem` for skipping following tests: - context_test.go#TestLoadProjectGopkgFilenames - project_test.go#TestCheckGopkgFilenames - fs_test.go#TestReadActualFilenames --- context.go | 2 +- context_test.go | 10 ++++++++-- internal/fs/fs.go | 2 +- internal/fs/fs_test.go | 9 ++++++++- project.go | 12 +++++++----- project_test.go | 12 +++++++++--- 6 files changed, 34 insertions(+), 13 deletions(-) diff --git a/context.go b/context.go index 6810777adf..f0a80473ab 100644 --- a/context.go +++ b/context.go @@ -105,7 +105,7 @@ func (c *Ctx) LoadProject() (*Project, error) { return nil, err } - err = checkCfgFilenames(root) + err = checkGopkgFilenames(root) if err != nil { return nil, err } diff --git a/context_test.go b/context_test.go index 9f01a9f940..7e5eda6e25 100644 --- a/context_test.go +++ b/context_test.go @@ -264,14 +264,20 @@ func TestLoadProjectNoSrcDir(t *testing.T) { } } -func TestLoadProjectCfgFileCase(t *testing.T) { +func TestLoadProjectGopkgFilenames(t *testing.T) { + // We are trying to skip this test on file systems which are case-sensiive. We could + // have used `fs.IsCaseSensitiveFilesystem` for this check. However, the code we are + // testing also relies on `fs.IsCaseSensitiveFilesystem`. So a bug in + // `fs.IsCaseSensitiveFilesystem` could prevent this test from being run. This is the + // only scenario where we prefer the OS heuristic over doing the actual work of + // validating filesystem case sensitivity via `fs.IsCaseSensitiveFilesystem`. if runtime.GOOS != "windows" && runtime.GOOS != "darwin" { t.Skip("skip this test on non-Windows, non-macOS") } // 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. + // `project_test.go#TestCheckGopkgFilenames`. So not repeating here. h := test.NewHelper(t) defer h.Cleanup() diff --git a/internal/fs/fs.go b/internal/fs/fs.go index 40695c2d2a..bfacfd95af 100644 --- a/internal/fs/fs.go +++ b/internal/fs/fs.go @@ -272,7 +272,7 @@ var errPathNotDir = errors.New("given path is not a directory") // `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 and returns a map which has the -// given file name as the key and actual filename as the value if it was found. +// given file name as the key and actual filename as the value(if it was found). func ReadActualFilenames(dirPath string, names []string) (map[string]string, error) { actualFilenames := make(map[string]string, len(names)) if len(names) == 0 { diff --git a/internal/fs/fs_test.go b/internal/fs/fs_test.go index f4e26bfb74..4b9422dd88 100644 --- a/internal/fs/fs_test.go +++ b/internal/fs/fs_test.go @@ -265,6 +265,12 @@ func TestIsCaseSensitiveFilesystem(t *testing.T) { } func TestReadActualFilenames(t *testing.T) { + // We are trying to skip this test on file systems which are case-sensiive. We could + // have used `fs.IsCaseSensitiveFilesystem` for this check. However, the code we are + // testing also relies on `fs.IsCaseSensitiveFilesystem`. So a bug in + // `fs.IsCaseSensitiveFilesystem` could prevent this test from being run. This is the + // only scenario where we prefer the OS heuristic over doing the actual work of + // validating filesystem case sensitivity via `fs.IsCaseSensitiveFilesystem`. if runtime.GOOS != "windows" && runtime.GOOS != "darwin" { t.Skip("skip this test on non-Windows, non-macOS") } @@ -332,7 +338,8 @@ func TestReadActualFilenames(t *testing.T) { t.Fatalf("unexpected error: %+v", err) } if !reflect.DeepEqual(c.want, got) { - t.Fatalf("returned value does not match expected: \n\t(GOT) %v\n\t(WNT) %v", got, c.want) + t.Fatalf("returned value does not match expected: \n\t(GOT) %v\n\t(WNT) %v", + got, c.want) } } } diff --git a/project.go b/project.go index 48a554332d..9749a723bf 100644 --- a/project.go +++ b/project.go @@ -42,17 +42,19 @@ func findProjectRoot(from string) (string, error) { } } -// checkCfgFilenames validates filename case for the manifest and lock files. +// checkGopkgFilenames validates filename case for the manifest and lock files. // -// This is relevant on case-insensitive file systems like Windows and macOS. +// This is relevant on case-insensitive file systems like the defaults in Windows and +// macOS. // // If manifest file is not found, it returns an error indicating the project could not be // found. If it is found but the case does not match, an error is returned. If a lock // file is not found, no error is returned as lock file is optional. If it is found but // the case does not match, an error is returned. -func checkCfgFilenames(projectRoot string) error { - // ReadActualFilenames is actually costly. Since this check is not relevant to - // case-sensitive filesystems like ext4(linux), try for an early return. +func checkGopkgFilenames(projectRoot string) error { + // ReadActualFilenames is actually costly. Since the check to validate filename case + // for Gopkg filenames is not relevant to case-sensitive filesystems like + // ext4(linux), try for an early return. caseSensitive, err := fs.IsCaseSensitiveFilesystem(projectRoot) if err != nil { return errors.Wrap(err, "could not check validity of configuration filenames") diff --git a/project_test.go b/project_test.go index c76702d03a..c3d1ef00ba 100644 --- a/project_test.go +++ b/project_test.go @@ -63,7 +63,13 @@ func TestFindRoot(t *testing.T) { } } -func TestCheckCfgFilenames(t *testing.T) { +func TestCheckGopkgFilenames(t *testing.T) { + // We are trying to skip this test on file systems which are case-sensiive. We could + // have used `fs.IsCaseSensitiveFilesystem` for this check. However, the code we are + // testing also relies on `fs.IsCaseSensitiveFilesystem`. So a bug in + // `fs.IsCaseSensitiveFilesystem` could prevent this test from being run. This is the + // only scenario where we prefer the OS heuristic over doing the actual work of + // validating filesystem case sensitivity via `fs.IsCaseSensitiveFilesystem`. if runtime.GOOS != "windows" && runtime.GOOS != "darwin" { t.Skip("skip this test on non-Windows, non-macOS") } @@ -111,11 +117,11 @@ func TestCheckCfgFilenames(t *testing.T) { tmpPath := h.Path(".") // Create any files that are needed for the test before invoking - // `checkCfgFilenames`. + // `checkGopkgFilenames`. for _, file := range c.createFiles { h.TempFile(file, "") } - err := checkCfgFilenames(tmpPath) + err := checkGopkgFilenames(tmpPath) if c.wantErr { if err == nil {