Merge pull request #348 from numtide/fix/relative-path-resolution

fix: relative path resolution in filesystem walker

cache/cache.go

@@ -218,11 +218,6 @@ func ChangeSet(ctx context.Context, walker walk.Walker, filesCh chan<- *walk.Fil
 			}
 		}
 
-		// ignore symlinks
-		if file.Info.Mode()&os.ModeSymlink == os.ModeSymlink {
-			return nil
-		}
-
 		// open a new read tx if there isn't one in progress
 		// we have to periodically open a new read tx to prevent writes from being blocked
 		if tx == nil {

cli/format.go

@@ -237,12 +237,9 @@ func (f *Format) walkFilesystem(ctx context.Context) func() error {
 				case <-ctx.Done():
 					return ctx.Err()
 				default:
-					// ignore symlinks and directories
-					if !(file.Info.IsDir() || file.Info.Mode()&os.ModeSymlink == os.ModeSymlink) {
-						stats.Add(stats.Traversed, 1)
-						stats.Add(stats.Emitted, 1)
-						f.filesCh <- file
-					}
+					stats.Add(stats.Traversed, 1)
+					stats.Add(stats.Emitted, 1)
+					f.filesCh <- file
 					return nil
 				}
 			})

walk/filesystem.go

@@ -4,39 +4,45 @@ import (
 	"context"
 	"fmt"
 	"io/fs"
+	"os"
 	"path/filepath"
 )
 
 type filesystemWalker struct {
-	root    string
-	pathsCh chan string
+	root          string
+	pathsCh       chan string
+	relPathOffset int
 }
 
 func (f filesystemWalker) Root() string {
 	return f.root
 }
 
-func (f filesystemWalker) Walk(_ context.Context, fn WalkFunc) error {
-	relPathOffset := len(f.root) + 1
-
-	relPathFn := func(path string) (string, error) {
-		// quick optimisation for the majority of use cases
-		// todo check that root is a prefix in path?
-		if len(path) >= relPathOffset {
-			return path[relPathOffset:], nil
-		}
-		return filepath.Rel(f.root, path)
+func (f filesystemWalker) relPath(path string) (string, error) {
+	// quick optimization for the majority of use cases
+	if len(path) >= f.relPathOffset && path[:len(f.root)] == f.root {
+		return path[f.relPathOffset:], nil
 	}
+	// fallback to proper relative path resolution
+	return filepath.Rel(f.root, path)
+}
 
+func (f filesystemWalker) Walk(_ context.Context, fn WalkFunc) error {
 	walkFn := func(path string, info fs.FileInfo, _ error) error {
 		if info == nil {
 			return fmt.Errorf("no such file or directory '%s'", path)
 		}
 
-		relPath, err := relPathFn(path)
+		// ignore directories and symlinks
+		if info.IsDir() || info.Mode()&os.ModeSymlink == os.ModeSymlink {
+			return nil
+		}
+
+		relPath, err := f.relPath(path)
 		if err != nil {
 			return fmt.Errorf("failed to determine a relative path for %s: %w", path, err)
 		}
+
 		file := File{
 			Path:    path,
 			RelPath: relPath,
@@ -55,5 +61,9 @@ func (f filesystemWalker) Walk(_ context.Context, fn WalkFunc) error {
 }
 
 func NewFilesystem(root string, paths chan string) (Walker, error) {
-	return filesystemWalker{root, paths}, nil
+	return filesystemWalker{
+		root:          root,
+		pathsCh:       paths,
+		relPathOffset: len(root) + 1,
+	}, nil
 }

walk/filesystem_test.go

@@ -0,0 +1,76 @@
+package walk
+
+import (
+	"context"
+	"os"
+	"testing"
+
+	"git.numtide.com/numtide/treefmt/test"
+	"github.com/stretchr/testify/require"
+)
+
+var examplesPaths = []string{
+	"elm/elm.json",
+	"elm/src/Main.elm",
+	"go/go.mod",
+	"go/main.go",
+	"haskell/CHANGELOG.md",
+	"haskell/Foo.hs",
+	"haskell/Main.hs",
+	"haskell/Nested/Foo.hs",
+	"haskell/Setup.hs",
+	"haskell/haskell.cabal",
+	"haskell/treefmt.toml",
+	"haskell-frontend/CHANGELOG.md",
+	"haskell-frontend/Main.hs",
+	"haskell-frontend/Setup.hs",
+	"haskell-frontend/haskell-frontend.cabal",
+	"html/index.html",
+	"html/scripts/.gitkeep",
+	"javascript/source/hello.js",
+	"nix/sources.nix",
+	"nixpkgs.toml",
+	"python/main.py",
+	"python/requirements.txt",
+	"python/virtualenv_proxy.py",
+	"ruby/bundler.rb",
+	"rust/Cargo.toml",
+	"rust/src/main.rs",
+	"shell/foo.sh",
+	"terraform/main.tf",
+	"terraform/two.tf",
+	"touch.toml",
+	"treefmt.toml",
+	"yaml/test.yaml",
+}
+
+func TestFilesystemWalker_Walk(t *testing.T) {
+	tempDir := test.TempExamples(t)
+
+	paths := make(chan string, 1)
+	go func() {
+		paths <- tempDir
+		close(paths)
+	}()
+
+	as := require.New(t)
+
+	walker, err := NewFilesystem(tempDir, paths)
+	as.NoError(err)
+
+	idx := 0
+	err = walker.Walk(context.Background(), func(file *File, err error) error {
+		as.Equal(examplesPaths[idx], file.RelPath)
+		idx += 1
+		return nil
+	})
+	as.NoError(err)
+
+	// capture current cwd, so we can replace it after the test is finished
+	cwd, err := os.Getwd()
+	as.NoError(err)
+	t.Cleanup(func() {
+		// return to the previous working directory
+		as.NoError(os.Chdir(cwd))
+	})
+}

walk/git.go

@@ -13,26 +13,26 @@ import (
 )
 
 type gitWalker struct {
-	root  string
-	paths chan string
-	repo  *git.Repository
+	root          string
+	paths         chan string
+	repo          *git.Repository
+	relPathOffset int
 }
 
-func (g *gitWalker) Root() string {
+func (g gitWalker) Root() string {
 	return g.root
 }
 
-func (g *gitWalker) Walk(ctx context.Context, fn WalkFunc) error {
-	// for quick relative paths
-	relPathOffset := len(g.root) + 1
-
-	relPathFn := func(path string) (relPath string) {
-		if len(path) >= relPathOffset {
-			relPath = path[relPathOffset:]
-		}
-		return
+func (g gitWalker) relPath(path string) (string, error) {
+	// quick optimization for the majority of use cases
+	if len(path) >= g.relPathOffset && path[:len(g.root)] == g.root {
+		return path[g.relPathOffset:], nil
 	}
+	// fallback to proper relative path resolution
+	return filepath.Rel(g.root, path)
+}
 
+func (g gitWalker) Walk(ctx context.Context, fn WalkFunc) error {
 	idx, err := g.repo.Storer.Index()
 	if err != nil {
 		return fmt.Errorf("failed to open git index: %w", err)
@@ -50,14 +50,28 @@ func (g *gitWalker) Walk(ctx context.Context, fn WalkFunc) error {
 				case <-ctx.Done():
 					return ctx.Err()
 				default:
-					path := filepath.Join(g.root, entry.Name)
+					// we only want regular files, not directories or symlinks
+					if !entry.Mode.IsRegular() {
+						continue
+					}
 
 					// stat the file
+					path := filepath.Join(g.root, entry.Name)
+
 					info, err := os.Lstat(path)
+					if err != nil {
+						return fmt.Errorf("failed to stat %s: %w", path, err)
+					}
+
+					// determine a relative path
+					relPath, err := g.relPath(path)
+					if err != nil {
+						return fmt.Errorf("failed to determine a relative path for %s: %w", path, err)
+					}
 
 					file := File{
 						Path:    path,
-						RelPath: relPathFn(path),
+						RelPath: relPath,
 						Info:    info,
 					}
 
@@ -93,9 +107,9 @@ func (g *gitWalker) Walk(ctx context.Context, fn WalkFunc) error {
 				return nil
 			}
 
-			relPath, err := filepath.Rel(g.root, path)
+			relPath, err := g.relPath(path)
 			if err != nil {
-				return err
+				return fmt.Errorf("failed to determine a relative path for %s: %w", path, err)
 			}
 
 			if _, ok := cache[relPath]; !ok {
@@ -105,7 +119,7 @@ func (g *gitWalker) Walk(ctx context.Context, fn WalkFunc) error {
 
 			file := File{
 				Path:    path,
-				RelPath: relPathFn(path),
+				RelPath: relPath,
 				Info:    info,
 			}
 
@@ -121,5 +135,10 @@ func NewGit(root string, paths chan string) (Walker, error) {
 	if err != nil {
 		return nil, fmt.Errorf("failed to open git repo: %w", err)
 	}
-	return &gitWalker{root, paths, repo}, nil
+	return &gitWalker{
+		root:          root,
+		paths:         paths,
+		repo:          repo,
+		relPathOffset: len(root) + 1,
+	}, nil
 }