Merge pull request #30 from arduino/improve_recursive_dirread

Do not stop `ReadDirRecursive*` on broken symlinks
arduino · Jan 15, 2024 · 4ac75f3 · 4ac75f3
2 parents 3226a11 + 075c5c8
commit 4ac75f3
Show file tree

Hide file tree

Showing 18 changed files with 95 additions and 32 deletions.
diff --git a/list.go b/list.go
@@ -78,7 +78,7 @@ func (p *PathList) FilterDirs() {
 func (p *PathList) FilterOutDirs() {
 	res := (*p)[:0]
 	for _, path := range *p {
-		if path.IsNotDir() {
+		if !path.IsDir() {
 			res = append(res, path)
 		}
 	}
@@ -96,7 +96,7 @@ func (p *PathList) FilterOutHiddenFiles() {
 func (p *PathList) Filter(acceptorFunc func(*Path) bool) {
 	res := (*p)[:0]
 	for _, path := range *p {
-        if acceptorFunc(path) {
+		if acceptorFunc(path) {
 			res = append(res, path)
 		}
 	}

diff --git a/paths.go b/paths.go
@@ -418,6 +418,13 @@ func (p *Path) CopyDirTo(dst *Path) error {
 	return nil
 }
 
+// Chmod changes the mode of the named file to mode. If the file is a
+// symbolic link, it changes the mode of the link's target. If there
+// is an error, it will be of type *os.PathError.
+func (p *Path) Chmod(mode fs.FileMode) error {
+	return os.Chmod(p.path, mode)
+}
+
 // Chtimes changes the access and modification times of the named file,
 // similar to the Unix utime() or utimes() functions.
 func (p *Path) Chtimes(atime, mtime time.Time) error {

diff --git a/paths_test.go b/paths_test.go
@@ -284,25 +284,49 @@ func TestFilterDirs(t *testing.T) {
 }
 
 func TestFilterOutDirs(t *testing.T) {
-	testPath := New("testdata", "fileset")
+	{
+		testPath := New("testdata", "fileset")
 
-	list, err := testPath.ReadDir()
-	require.NoError(t, err)
-	require.Len(t, list, 6)
+		list, err := testPath.ReadDir()
+		require.NoError(t, err)
+		require.Len(t, list, 6)
+
+		pathEqualsTo(t, "testdata/fileset/anotherFile", list[0])
+		pathEqualsTo(t, "testdata/fileset/file", list[1])
+		pathEqualsTo(t, "testdata/fileset/folder", list[2])
+		pathEqualsTo(t, "testdata/fileset/symlinktofolder", list[3])
+		pathEqualsTo(t, "testdata/fileset/test.txt", list[4])
+		pathEqualsTo(t, "testdata/fileset/test.txt.gz", list[5])
+
+		list.FilterOutDirs()
+		require.Len(t, list, 4)
+		pathEqualsTo(t, "testdata/fileset/anotherFile", list[0])
+		pathEqualsTo(t, "testdata/fileset/file", list[1])
+		pathEqualsTo(t, "testdata/fileset/test.txt", list[2])
+		pathEqualsTo(t, "testdata/fileset/test.txt.gz", list[3])
+	}
 
-	pathEqualsTo(t, "testdata/fileset/anotherFile", list[0])
-	pathEqualsTo(t, "testdata/fileset/file", list[1])
-	pathEqualsTo(t, "testdata/fileset/folder", list[2])
-	pathEqualsTo(t, "testdata/fileset/symlinktofolder", list[3])
-	pathEqualsTo(t, "testdata/fileset/test.txt", list[4])
-	pathEqualsTo(t, "testdata/fileset/test.txt.gz", list[5])
+	{
+		list, err := New("testdata", "broken_symlink", "dir_1").ReadDirRecursive()
+		require.NoError(t, err)
 
-	list.FilterOutDirs()
-	require.Len(t, list, 4)
-	pathEqualsTo(t, "testdata/fileset/anotherFile", list[0])
-	pathEqualsTo(t, "testdata/fileset/file", list[1])
-	pathEqualsTo(t, "testdata/fileset/test.txt", list[2])
-	pathEqualsTo(t, "testdata/fileset/test.txt.gz", list[3])
+		require.Len(t, list, 7)
+		pathEqualsTo(t, "testdata/broken_symlink/dir_1/broken_link", list[0])
+		pathEqualsTo(t, "testdata/broken_symlink/dir_1/file2", list[1])
+		pathEqualsTo(t, "testdata/broken_symlink/dir_1/linked_dir", list[2])
+		pathEqualsTo(t, "testdata/broken_symlink/dir_1/linked_dir/file1", list[3])
+		pathEqualsTo(t, "testdata/broken_symlink/dir_1/linked_file", list[4])
+		pathEqualsTo(t, "testdata/broken_symlink/dir_1/real_dir", list[5])
+		pathEqualsTo(t, "testdata/broken_symlink/dir_1/real_dir/file1", list[6])
+
+		list.FilterOutDirs()
+		require.Len(t, list, 5)
+		pathEqualsTo(t, "testdata/broken_symlink/dir_1/broken_link", list[0])
+		pathEqualsTo(t, "testdata/broken_symlink/dir_1/file2", list[1])
+		pathEqualsTo(t, "testdata/broken_symlink/dir_1/linked_dir/file1", list[2])
+		pathEqualsTo(t, "testdata/broken_symlink/dir_1/linked_file", list[3])
+		pathEqualsTo(t, "testdata/broken_symlink/dir_1/real_dir/file1", list[4])
+	}
 }
 
 func TestEquivalentPaths(t *testing.T) {

diff --git a/readdir.go b/readdir.go
@@ -116,9 +116,7 @@ func (p *Path) ReadDirRecursiveFiltered(recursionFilter ReadDirFilter, filters .
 			}
 
 			if recursionFilter == nil || recursionFilter(path) {
-				if isDir, err := path.IsDirCheck(); err != nil {
-					return nil, err
-				} else if isDir {
+				if path.IsDir() {
 					subPaths, err := search(path)
 					if err != nil {
 						return nil, err

diff --git a/readdir_test.go b/readdir_test.go
@@ -31,7 +31,9 @@ package paths
 
 import (
 	"fmt"
+	"io/fs"
 	"os"
+	"runtime"
 	"testing"
 	"time"
 
@@ -250,21 +252,11 @@ func TestReadDirRecursiveFiltered(t *testing.T) {
 func TestReadDirRecursiveLoopDetection(t *testing.T) {
 	loopsPath := New("testdata", "loops")
 	unbuondedReaddir := func(testdir string) (PathList, error) {
-		// This is required to unbound the recursion, otherwise it will stop
-		// when the paths becomes too long due to the symlink loop: this is not
-		// what we want, we are looking for an early detection of the loop.
-		skipBrokenLinks := func(p *Path) bool {
-			_, err := p.Stat()
-			return err == nil
-		}
-
 		var files PathList
 		var err error
 		done := make(chan bool)
 		go func() {
-			files, err = loopsPath.Join(testdir).ReadDirRecursiveFiltered(
-				skipBrokenLinks,
-			)
+			files, err = loopsPath.Join(testdir).ReadDirRecursive()
 			done <- true
 		}()
 		require.Eventually(
@@ -313,4 +305,39 @@ func TestReadDirRecursiveLoopDetection(t *testing.T) {
 		pathEqualsTo(t, "testdata/loops/regular_2/dir2/dir1/file1", l[4])
 		pathEqualsTo(t, "testdata/loops/regular_2/dir2/file2", l[5])
 	}
+
+	{
+		l, err := unbuondedReaddir("regular_3")
+		require.NoError(t, err)
+		require.Len(t, l, 7)
+		l.Sort()
+		pathEqualsTo(t, "testdata/loops/regular_3/dir1", l[0])
+		pathEqualsTo(t, "testdata/loops/regular_3/dir1/file1", l[1])
+		pathEqualsTo(t, "testdata/loops/regular_3/dir2", l[2])
+		pathEqualsTo(t, "testdata/loops/regular_3/dir2/dir1", l[3])
+		pathEqualsTo(t, "testdata/loops/regular_3/dir2/dir1/file1", l[4])
+		pathEqualsTo(t, "testdata/loops/regular_3/dir2/file2", l[5])
+		pathEqualsTo(t, "testdata/loops/regular_3/link", l[6]) // broken symlink is reported in files
+	}
+
+	if runtime.GOOS != "windows" {
+		dir1 := loopsPath.Join("regular_4_with_permission_error", "dir1")
+
+		l, err := unbuondedReaddir("regular_4_with_permission_error")
+		require.NoError(t, err)
+		require.NotEmpty(t, l)
+
+		dir1Stat, err := dir1.Stat()
+		require.NoError(t, err)
+		err = dir1.Chmod(fs.FileMode(0)) // Enforce permission error
+		require.NoError(t, err)
+		t.Cleanup(func() {
+			// Restore normal permission after the test
+			dir1.Chmod(dir1Stat.Mode())
+		})
+
+		l, err = unbuondedReaddir("regular_4_with_permission_error")
+		require.Error(t, err)
+		require.Nil(t, l)
+	}
 }
diff --git a/testdata/broken_symlink/dir_1/broken_link b/testdata/broken_symlink/dir_1/broken_link
@@ -0,0 +1 @@
+broken
diff --git a/testdata/broken_symlink/dir_1/file2 b/testdata/broken_symlink/dir_1/file2
diff --git a/testdata/broken_symlink/dir_1/linked_dir b/testdata/broken_symlink/dir_1/linked_dir
@@ -0,0 +1 @@
+real_dir
diff --git a/testdata/broken_symlink/dir_1/linked_file b/testdata/broken_symlink/dir_1/linked_file
@@ -0,0 +1 @@
+file2
diff --git a/testdata/broken_symlink/dir_1/real_dir/file1 b/testdata/broken_symlink/dir_1/real_dir/file1
diff --git a/testdata/loops/regular_3/dir1/file1 b/testdata/loops/regular_3/dir1/file1
diff --git a/testdata/loops/regular_3/dir2/dir1 b/testdata/loops/regular_3/dir2/dir1
@@ -0,0 +1 @@
+../dir1
diff --git a/testdata/loops/regular_3/dir2/file2 b/testdata/loops/regular_3/dir2/file2
diff --git a/testdata/loops/regular_3/link b/testdata/loops/regular_3/link
@@ -0,0 +1 @@
+broken
diff --git a/testdata/loops/regular_4_with_permission_error/dir1/file1 b/testdata/loops/regular_4_with_permission_error/dir1/file1
diff --git a/testdata/loops/regular_4_with_permission_error/dir2/dir1 b/testdata/loops/regular_4_with_permission_error/dir2/dir1
@@ -0,0 +1 @@
+../dir1
diff --git a/testdata/loops/regular_4_with_permission_error/dir2/file2 b/testdata/loops/regular_4_with_permission_error/dir2/file2
diff --git a/testdata/loops/regular_4_with_permission_error/link b/testdata/loops/regular_4_with_permission_error/link
@@ -0,0 +1 @@
+broken