Merge pull request #595 from jfly/fix-treefmt-dot-in-symlinked-project-root

fix: `treefmt .` no longer segfaults if `.` is root and symlink

Author: brianmcgee

cmd/format/format.go

@@ -7,9 +7,7 @@ import (
 	"io"
 	"os"
 	"os/signal"
-	"path/filepath"
 	"runtime/pprof"
-	"strings"
 	"syscall"
 	"time"
 
@@ -124,10 +122,6 @@ func Run(v *viper.Viper, statz *stats.Stats, cmd *cobra.Command, paths []string)
 		return errors.New("exactly one path should be specified when using the --stdin flag")
 	}
 
-	if err = resolvePaths(paths, walkType, cfg.TreeRoot); err != nil {
-		return err
-	}
-
 	// create a composite formatter which will handle applying the correct formatters to each file we traverse
 	formatter, err := format.NewCompositeFormatter(cfg, statz, BatchSize)
 	if err != nil {
@@ -213,60 +207,3 @@ func Run(v *viper.Viper, statz *stats.Stats, cmd *cobra.Command, paths []string)
 
 	return nil
 }
-
-// resolvePaths checks all paths are contained within the tree root and exist
-// also "normalize" paths so they're relative to `cfg.TreeRoot`
-// Symlinks are allowed in `paths` and we resolve them here, since
-// the readers will ignore symlinks.
-func resolvePaths(paths []string, walkType walk.Type, treeRoot string) error {
-	// Note: `treeRoot` may itself be or contain a symlink (e.g. it is in
-	// `$TMPDIR` on macOS or a user has set a symlink to shorten the repository
-	// path for path length restrictions), so we resolve it here first.
-	//
-	// See: https://github.com/numtide/treefmt/issues/578
-	treeRoot, err := resolvePath(walkType, treeRoot)
-	if err != nil {
-		return fmt.Errorf("error computing absolute path of %s: %w", treeRoot, err)
-	}
-
-	for i, path := range paths {
-		absolutePath, err := resolvePath(walkType, path)
-		if err != nil {
-			return fmt.Errorf("error computing absolute path of %s: %w", path, err)
-		}
-
-		relativePath, err := filepath.Rel(treeRoot, absolutePath)
-		if err != nil {
-			return fmt.Errorf("error computing relative path from %s to %s: %w", treeRoot, absolutePath, err)
-		}
-
-		if strings.HasPrefix(relativePath, "..") {
-			return fmt.Errorf("path %s not inside the tree root %s", path, treeRoot)
-		}
-
-		paths[i] = relativePath
-	}
-
-	return nil
-}
-
-// Resolve a path to an absolute path, resolving symlinks if necessary.
-func resolvePath(walkType walk.Type, path string) (string, error) {
-	log.Debugf("Resolving path '%s': %v", path, walkType)
-
-	absolutePath, err := filepath.Abs(path)
-	if err != nil {
-		return "", fmt.Errorf("error computing absolute path of %s: %w", path, err)
-	}
-
-	if walkType != walk.Stdin {
-		realPath, err := filepath.EvalSymlinks(absolutePath)
-		if err != nil {
-			return "", fmt.Errorf("path %s not found: %w", absolutePath, err)
-		}
-
-		absolutePath = realPath
-	}
-
-	return absolutePath, nil
-}

cmd/root_test.go

@@ -1439,9 +1439,9 @@ func TestGit(t *testing.T) {
 		withConfig(configPath, cfg),
 		withNoError(t),
 		withStats(t, map[stats.Type]int{
-			stats.Traversed: 79,
-			stats.Matched:   79,
-			stats.Formatted: 51, // the echo formatter should only be applied to the new files
+			stats.Traversed: 80,
+			stats.Matched:   80,
+			stats.Formatted: 52, // the echo formatter should only be applied to the new files
 			stats.Changed:   0,
 		}),
 	)
@@ -1923,7 +1923,7 @@ func TestStdin(t *testing.T) {
 			as.ErrorContains(err, "path ../test.nix not inside the tree root "+tempDir)
 		}),
 		withStderr(func(out []byte) {
-			as.Contains(string(out), "Error: path ../test.nix not inside the tree root")
+			as.Contains(string(out), "Error: failed to create walker: path ../test.nix not inside the tree root")
 		}),
 	)
 
@@ -2136,9 +2136,40 @@ func TestProjectRootIsSymlink(t *testing.T) {
 	configPath := filepath.Join(symlinkRoot, "/treefmt.toml")
 	test.WriteConfig(t, configPath, cfg)
 
+	// Verify we can format a specific file.
 	treefmt(t,
 		withArgs("-c", "go/main.go"),
 		withNoError(t),
+		withStats(t, map[stats.Type]int{
+			stats.Traversed: 1,
+			stats.Matched:   1,
+			stats.Formatted: 1,
+			stats.Changed:   0,
+		}),
+	)
+
+	// Verify we can format a specific directory that is a symlink.
+	treefmt(t,
+		withArgs("-c", "symlink-to-yaml-dir"),
+		withNoError(t),
+		withStats(t, map[stats.Type]int{
+			stats.Traversed: 1,
+			stats.Matched:   1,
+			stats.Formatted: 1,
+			stats.Changed:   0,
+		}),
+	)
+
+	// Verify we can format the current directory (which is a symlink!).
+	treefmt(t,
+		withArgs("-c", "."),
+		withNoError(t),
+		withStats(t, map[stats.Type]int{
+			stats.Traversed: 32,
+			stats.Matched:   32,
+			stats.Formatted: 32,
+			stats.Changed:   0,
+		}),
 	)
 }
 

test/examples/symlink-to-yaml-dir

@@ -0,0 +1 @@
+yaml/

walk/walk.go

@@ -9,7 +9,9 @@ import (
 	"io/fs"
 	"os"
 	"path/filepath"
+	"strings"
 
+	"github.com/charmbracelet/log"
 	"github.com/numtide/treefmt/v2/stats"
 	bolt "go.etcd.io/bbolt"
 )
@@ -245,7 +247,17 @@ func NewCompositeReader(
 	db *bolt.DB,
 	statz *stats.Stats,
 ) (Reader, error) {
-	// if not paths are provided we default to processing the tree root
+	// Note: `root` may itself be or contain a symlink (e.g. it is in
+	// `$TMPDIR` on macOS or a user has set a symlink to shorten the repository
+	// path for path length restrictions), so we resolve it here first.
+	//
+	// See: https://github.com/numtide/treefmt/issues/578
+	root, err := resolvePath(root)
+	if err != nil {
+		return nil, fmt.Errorf("error resolving path %s: %w", root, err)
+	}
+
+	// if no paths are provided we default to processing the tree root
 	if len(paths) == 0 {
 		return NewReader(walkType, root, "", db, statz)
 	}
@@ -258,44 +270,73 @@ func NewCompositeReader(
 			return nil, errors.New("stdin walk requires exactly one path")
 		}
 
-		return NewStdinReader(root, paths[0], statz), nil
+		path := paths[0]
+
+		if strings.HasPrefix(path, "..") {
+			return nil, fmt.Errorf("path %s not inside the tree root %s", path, root)
+		}
+
+		return NewStdinReader(root, path, statz), nil
 	}
 
 	// create a reader for each provided path
-	for idx, relPath := range paths {
+	for idx, path := range paths {
 		var (
 			err  error
 			info os.FileInfo
 		)
 
-		// create a clean absolute path
-		path := filepath.Clean(filepath.Join(root, relPath))
+		resolvedPath, err := resolvePath(path)
+		if err != nil {
+			return nil, fmt.Errorf("error resolving path %s: %w", path, err)
+		}
+
+		relativePath, err := filepath.Rel(root, resolvedPath)
+		if err != nil {
+			return nil, fmt.Errorf("error computing relative path from %s to %s: %w", root, resolvedPath, err)
+		}
+
+		if strings.HasPrefix(relativePath, "..") {
+			return nil, fmt.Errorf("path %s not inside the tree root %s", path, root)
+		}
 
-		// check the path exists (don't follow symlinks)
-		info, err = os.Lstat(path)
+		// check the path exists
+		info, err = os.Lstat(resolvedPath)
 		if err != nil {
-			return nil, fmt.Errorf("failed to stat %s: %w", path, err)
+			return nil, fmt.Errorf("failed to stat %s: %w", resolvedPath, err)
 		}
 
-		switch {
-		case info.Mode()&os.ModeSymlink == os.ModeSymlink:
-			// for symlinks -> we ignore them since it does not make sense to follow them
-			// as normal files in the `root` will be picked up nevertheless.
-			continue
-		case info.IsDir():
+		if info.IsDir() {
 			// for directories, we honour the walk type as we traverse them
-			readers[idx], err = NewReader(walkType, root, relPath, db, statz)
-		default:
+			readers[idx], err = NewReader(walkType, root, relativePath, db, statz)
+		} else {
 			// for files, we enforce a simple filesystem read
-			readers[idx], err = NewReader(Filesystem, root, relPath, db, statz)
+			readers[idx], err = NewReader(Filesystem, root, relativePath, db, statz)
 		}
 
 		if err != nil {
-			return nil, fmt.Errorf("failed to create reader for %s: %w", relPath, err)
+			return nil, fmt.Errorf("failed to create reader for %s: %w", relativePath, err)
 		}
 	}
 
 	return &CompositeReader{
 		readers: readers,
 	}, nil
 }
+
+// Resolve a path to an absolute path, resolving any symlinks along the way.
+func resolvePath(path string) (string, error) {
+	log.Debugf("Resolving path '%s'", path)
+
+	absolutePath, err := filepath.Abs(path)
+	if err != nil {
+		return "", fmt.Errorf("error computing absolute path of %s: %w", path, err)
+	}
+
+	resolvedPath, err := filepath.EvalSymlinks(absolutePath)
+	if err != nil {
+		return "", fmt.Errorf("path %s not found: %w", absolutePath, err)
+	}
+
+	return resolvedPath, nil
+}