Fix recursive globs through symlinks

filepath.Walk does not walk symlinks to directories, which leads to
inconsitent behavior with following symlinks.  Replace the use of
filepath.Walk in ListDirsRecursive with a helper function that lists
each directory and walks anything for which IsDir reports true, which
includes following symlinks, and then reconstructs the path through
the symlink.

Test: fs_test.go
Test: glob_test.go
Change-Id: Ie4dd0820e9c7c0a124caa65210ce20439a44da16

pathtools/fs.go

@@ -16,6 +16,7 @@ package pathtools
 
 import (
 	"bytes"
+	"fmt"
 	"io"
 	"io/ioutil"
 	"os"
@@ -93,6 +94,9 @@ type FileSystem interface {
 
 	// ListDirsRecursive returns a list of all the directories in a path, following symlinks.
 	ListDirsRecursive(name string) (dirs []string, err error)
+
+	// ReadDirNames returns a list of everything in a directory.
+	ReadDirNames(name string) ([]string, error)
 }
 
 // osFs implements FileSystem using the local disk.
@@ -140,24 +144,23 @@ func (osFs) Lstat(path string) (stats os.FileInfo, err error) {
 
 // Returns a list of all directories under dir
 func (osFs) ListDirsRecursive(name string) (dirs []string, err error) {
-	// TODO: follow symbolic links
-	err = filepath.Walk(name, func(path string, info os.FileInfo, err error) error {
-		if err != nil {
-			return err
-		}
+	return listDirsRecursive(OsFs, name)
+}
 
-		if info.Mode().IsDir() {
-			name := info.Name()
-			if name[0] == '.' && name != "." {
-				return filepath.SkipDir
-			}
+func (osFs) ReadDirNames(name string) ([]string, error) {
+	dir, err := os.Open(name)
+	if err != nil {
+		return nil, err
+	}
+	defer dir.Close()
 
-			dirs = append(dirs, path)
-		}
-		return nil
-	})
+	contents, err := dir.Readdirnames(-1)
+	if err != nil {
+		return nil, err
+	}
 
-	return dirs, err
+	sort.Strings(contents)
+	return contents, nil
 }
 
 type mockFs struct {
@@ -348,22 +351,96 @@ func (m *mockFs) Lstat(name string) (os.FileInfo, error) {
 	return &ms, nil
 }
 
-func (m *mockFs) ListDirsRecursive(name string) (dirs []string, err error) {
+func (m *mockFs) ReadDirNames(name string) ([]string, error) {
 	name = filepath.Clean(name)
-	dirs = append(dirs, name)
-	if name == "." {
-		name = ""
-	} else if name != "/" {
-		name = name + "/"
+	name = m.followSymlinks(name)
+
+	exists, isDir, err := m.Exists(name)
+	if err != nil {
+		return nil, err
 	}
+	if !exists {
+		return nil, os.ErrNotExist
+	}
+	if !isDir {
+		return nil, os.NewSyscallError("readdir", syscall.ENOTDIR)
+	}
+
+	var ret []string
 	for _, f := range m.all {
-		if _, isDir := m.dirs[f]; isDir && filepath.Base(f)[0] != '.' {
-			if strings.HasPrefix(f, name) &&
-				strings.HasPrefix(f, "/") == strings.HasPrefix(name, "/") {
-				dirs = append(dirs, f)
-			}
+		dir, file := saneSplit(f)
+		if dir == name && len(file) > 0 && file[0] != '.' {
+			ret = append(ret, file)
 		}
 	}
+	return ret, nil
+}
+
+func (m *mockFs) ListDirsRecursive(name string) ([]string, error) {
+	return listDirsRecursive(m, name)
+}
+
+func listDirsRecursive(fs FileSystem, name string) ([]string, error) {
+	name = filepath.Clean(name)
+
+	isDir, err := fs.IsDir(name)
+	if err != nil {
+		return nil, err
+	}
+
+	if !isDir {
+		return nil, nil
+	}
+
+	dirs := []string{name}
+
+	subDirs, err := listDirsRecursiveRelative(fs, name, 0)
+	if err != nil {
+		return nil, err
+	}
+
+	for _, d := range subDirs {
+		dirs = append(dirs, filepath.Join(name, d))
+	}
+
+	return dirs, nil
+}
+
+func listDirsRecursiveRelative(fs FileSystem, name string, depth int) ([]string, error) {
+	depth++
+	if depth > 255 {
+		return nil, fmt.Errorf("too many symlinks")
+	}
+	contents, err := fs.ReadDirNames(name)
+	if err != nil {
+		return nil, err
+	}
+
+	var dirs []string
+	for _, f := range contents {
+		if f[0] == '.' {
+			continue
+		}
+		f = filepath.Join(name, f)
+		if isDir, _ := fs.IsDir(f); isDir {
+			dirs = append(dirs, f)
+			subDirs, err := listDirsRecursiveRelative(fs, f, depth)
+			if err != nil {
+				return nil, err
+			}
+			for _, s := range subDirs {
+				dirs = append(dirs, filepath.Join(f, s))
+			}
+		}
+	}
+
+	for i, d := range dirs {
+		rel, err := filepath.Rel(name, d)
+		if err != nil {
+			return nil, err
+		}
+		dirs[i] = rel
+	}
 
 	return dirs, nil
 }

pathtools/fs_test.go

@@ -101,7 +101,7 @@ func TestMockFs_followSymlinks(t *testing.T) {
 	}
 }
 
-func TestMockFs_IsDir(t *testing.T) {
+func TestFs_IsDir(t *testing.T) {
 	testCases := []struct {
 		name  string
 		isDir bool
@@ -149,15 +149,74 @@ func TestMockFs_IsDir(t *testing.T) {
 	}
 
 	mock := symlinkMockFs()
+	fsList := []FileSystem{mock, OsFs}
+	names := []string{"mock", "os"}
 
-	for _, test := range testCases {
-		t.Run(test.name, func(t *testing.T) {
-			got, err := mock.IsDir(test.name)
-			if !reflect.DeepEqual(err, test.err) {
-				t.Errorf("want: %v, got %v", test.err, err)
+	os.Chdir("testdata/dangling")
+	defer os.Chdir("../..")
+
+	for i, fs := range fsList {
+		t.Run(names[i], func(t *testing.T) {
+			for _, test := range testCases {
+				t.Run(test.name, func(t *testing.T) {
+					got, err := fs.IsDir(test.name)
+					checkErr(t, test.err, err)
+					if got != test.isDir {
+						t.Errorf("want: %v, got %v", test.isDir, got)
+					}
+				})
 			}
-			if got != test.isDir {
-				t.Errorf("want: %v, got %v", test.isDir, got)
+		})
+	}
+}
+
+func TestFs_ListDirsRecursive(t *testing.T) {
+	testCases := []struct {
+		name string
+		dirs []string
+		err  error
+	}{
+		{".", []string{".", "a", "a/a", "b", "b/a", "c", "d"}, nil},
+
+		{"a", []string{"a", "a/a"}, nil},
+		{"a/a", []string{"a/a"}, nil},
+		{"a/a/a", nil, nil},
+
+		{"b", []string{"b", "b/a"}, nil},
+		{"b/a", []string{"b/a"}, nil},
+		{"b/a/a", nil, nil},
+
+		{"c", []string{"c"}, nil},
+		{"c/a", nil, nil},
+
+		{"d", []string{"d"}, nil},
+		{"d/a", nil, nil},
+
+		{"e", nil, nil},
+
+		{"dangling", nil, os.ErrNotExist},
+
+		{"missing", nil, os.ErrNotExist},
+	}
+
+	mock := symlinkMockFs()
+	fsList := []FileSystem{mock, OsFs}
+	names := []string{"mock", "os"}
+
+	os.Chdir("testdata/dangling")
+	defer os.Chdir("../..")
+
+	for i, fs := range fsList {
+		t.Run(names[i], func(t *testing.T) {
+
+			for _, test := range testCases {
+				t.Run(test.name, func(t *testing.T) {
+					got, err := fs.ListDirsRecursive(test.name)
+					checkErr(t, test.err, err)
+					if !reflect.DeepEqual(got, test.dirs) {
+						t.Errorf("want: %v, got %v", test.dirs, got)
+					}
+				})
 			}
 		})
 	}
@@ -202,16 +261,39 @@ func TestMockFs_glob(t *testing.T) {
 	}
 
 	mock := symlinkMockFs()
+	fsList := []FileSystem{mock, OsFs}
+	names := []string{"mock", "os"}
 
-	for _, test := range testCases {
-		t.Run(test.pattern, func(t *testing.T) {
-			got, err := mock.glob(test.pattern)
-			if err != nil {
-				t.Fatal(err)
-			}
-			if !reflect.DeepEqual(got, test.files) {
-				t.Errorf("want: %v, got %v", test.files, got)
+	os.Chdir("testdata/dangling")
+	defer os.Chdir("../..")
+
+	for i, fs := range fsList {
+		t.Run(names[i], func(t *testing.T) {
+			for _, test := range testCases {
+				t.Run(test.pattern, func(t *testing.T) {
+					got, err := fs.glob(test.pattern)
+					if err != nil {
+						t.Fatal(err)
+					}
+					if !reflect.DeepEqual(got, test.files) {
+						t.Errorf("want: %v, got %v", test.files, got)
+					}
+				})
 			}
 		})
 	}
 }
+
+func checkErr(t *testing.T, want, got error) {
+	t.Helper()
+	if os.IsNotExist(want) {
+		if os.IsNotExist(got) != os.IsNotExist(want) {
+			t.Errorf("want: IsNotExist(err) = %v, got IsNotExist(%v) = %v",
+				os.IsNotExist(want), got, os.IsNotExist(got))
+		}
+	} else if (got != nil) != (want != nil) {
+		t.Errorf("want: %v, got %v", want, got)
+	} else if got != nil && got.Error() != want.Error() {
+		t.Errorf("want: %v, got %v", want, got)
+	}
+}

pathtools/glob_test.go

@@ -565,6 +565,49 @@ func TestGlobEscapes(t *testing.T) {
 
 }
 
+var globSymlinkTestCases = []globTestCase{
+	{
+		pattern: `**/*`,
+		matches: []string{"a/", "b/", "c/", "d/", "e", "a/a/", "a/a/a", "b/a/", "b/a/a", "c/a", "d/a"},
+		deps:    []string{".", "a", "a/a", "b", "b/a", "c", "d"},
+	},
+}
+
+func TestMockGlobSymlinks(t *testing.T) {
+	files := []string{
+		"a/a/a",
+		"b -> a",
+		"c -> a/a",
+		"d -> c",
+		"e -> a/a/a",
+	}
+
+	mockFiles := make(map[string][]byte)
+
+	for _, f := range files {
+		mockFiles[f] = nil
+	}
+
+	mock := MockFs(mockFiles)
+
+	for _, testCase := range globSymlinkTestCases {
+		t.Run(testCase.pattern, func(t *testing.T) {
+			testGlob(t, mock, testCase)
+		})
+	}
+}
+
+func TestGlobSymlinks(t *testing.T) {
+	os.Chdir("testdata/symlinks")
+	defer os.Chdir("../..")
+
+	for _, testCase := range globSymlinkTestCases {
+		t.Run(testCase.pattern, func(t *testing.T) {
+			testGlob(t, OsFs, testCase)
+		})
+	}
+}
+
 func testGlob(t *testing.T, fs FileSystem, testCase globTestCase) {
 	t.Helper()
 	matches, deps, err := fs.Glob(testCase.pattern, testCase.excludes)

pathtools/testdata/dangling/b

@@ -0,0 +1 @@
+a

pathtools/testdata/dangling/c

@@ -0,0 +1 @@
+a/a

pathtools/testdata/dangling/d

@@ -0,0 +1 @@
+c

pathtools/testdata/dangling/dangling

@@ -0,0 +1 @@
+missing

pathtools/testdata/dangling/e

@@ -0,0 +1 @@
+a/a/a

pathtools/testdata/symlinks/b

@@ -0,0 +1 @@
+a

pathtools/testdata/symlinks/c

@@ -0,0 +1 @@
+a/a

pathtools/testdata/symlinks/d

@@ -0,0 +1 @@
+c

pathtools/testdata/symlinks/e

@@ -0,0 +1 @@
+a/a/a