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

check Gopkg filenames case on case insensitive systems #1114

Merged
merged 3 commits into from
Sep 22, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions context.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,11 @@ func (c *Ctx) LoadProject() (*Project, error) {
return nil, err
}

err = checkGopkgFilenames(root)
if err != nil {
return nil, err
}

p := new(Project)

if err = p.SetRoot(root); err != nil {
Expand Down
50 changes: 50 additions & 0 deletions context_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package dep

import (
"fmt"
"io/ioutil"
"log"
"os"
Expand Down Expand Up @@ -263,6 +264,55 @@ func TestLoadProjectNoSrcDir(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#TestCheckGopkgFilenames`. 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 %q does not match %q",
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) {
Expand Down
94 changes: 88 additions & 6 deletions internal/fs/fs.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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")
}
Expand Down Expand Up @@ -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")
}
Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand Down Expand Up @@ -264,6 +264,88 @@ 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 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,
// as it may be 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, wrap and 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 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 := make(map[string]string, len(names))
for _, name := range names {
namesMap[strings.ToLower(name)] = name
}

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
}
}
}
return actualFilenames, nil
}

var (
errSrcNotDir = errors.New("source is not a directory")
errDstExist = errors.New("destination already exists")
Expand Down
128 changes: 122 additions & 6 deletions internal/fs/fs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -229,6 +231,119 @@ func TestRenameWithFallback(t *testing.T) {
}
}

func TestIsCaseSensitiveFilesystem(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a bit inaccurate. You can mount a case-sensitive filesystem on Linux, a case-insensitive one on Linux/macOS...

Can we think of a better way to check this rather than relying on the OS?

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 agree. But I cannot think of a better way. To some extent, I think we can make some assumptions about the test environment.

Copy link

@colek42 colek42 Sep 6, 2017

Choose a reason for hiding this comment

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

This will not work on Windows Subsystem for Linux. dF -Th will give you the actual file system type if the OS is linux. I'm not sure how it works with samba shares, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@colek42 Is Windows Subsystem really something that the tests should handle? Isn't it an edge case we can safely ignore?

Copy link

@colek42 colek42 Sep 7, 2017

Choose a reason for hiding this comment

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

@sudo-suhas it is easy enough to test for. See microsoft/WSL#423, can you just skip the test if it is WSL? Some of us are forced to use 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.

Some of us are forced to use Windows.

Ha ha.. Don't worry I am on windows too. I will look into this. Thank you.

Copy link

Choose a reason for hiding this comment

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

@sudo-suhas let me know if you need me to test any changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure what's the best way to test this tbh. It's not easy to find a cross-platform solution for this as far as I know...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sdboyer Do I need to check for BashOnWindows? How would I check this in golang? Is it as simple as a file read from /proc/version?

Copy link
Member

Choose a reason for hiding this comment

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

@sudo-suhas not here - this is just a test. unless we have an actual reason to believe the real logic (not the test logic) fails on WSL, then it's fine to omit here.

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) {
// 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")
}

h := test.NewHelper(t)
defer h.Cleanup()

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)
}
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
}{
// 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{
"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
Expand All @@ -254,11 +369,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++ {
Expand Down Expand Up @@ -613,6 +728,7 @@ func TestCopyFileLongFilePath(t *testing.T) {

h := test.NewHelper(t)
h.TempDir(".")
defer h.Cleanup()

tmpPath := h.Path(".")

Expand Down
Loading