Repository: numtide/treefmt
Pull request #595 (2025-05-25 09:30:31)

fix: `treefmt .` no longer segfaults if `.` is root and symlink
In short, the old code would massage the given paths to relative paths (in the `resolvePaths` function I removed). It operated under an impossible-to-satisfy requirement: to return relative paths that when joined with the project root would create paths to regular files/folders (*not* symlinks). This is possible for all paths except `.`: if your project root is a symlink, then `join(root, ".") == root`, which is still a symlink.

Rather than try to fix that flawed assumption, I opted to refactor things. I think that was the right call: excluding the tests I added, this is a net reduction in code!

Basically, I moved moved `resolvePath` into `walk.go`, and interleaved the remaining `resolvePaths` logic into `walk.go::NewCompositeReader`. A happy side effect of this is that we no longer have 2 functions with very similar names (`resolvePaths` and `resolvePath`) that do different things.

This fixes https://github.com/numtide/treefmt/issues/594


Patch:
diff --git a/cmd/format/format.go b/cmd/format/format.go
index ae1ba16..3031e5f 100644
--- a/cmd/format/format.go
+++ b/cmd/format/format.go
@@ -7,9 +7,7 @@ import (
 	"io"
 	"os"
 	"os/signal"
-	"path/filepath"
 	"runtime/pprof"
-	"strings"
 	"syscall"
 	"time"
 
@@ -77,13 +75,6 @@ func Run(v *viper.Viper, statz *stats.Stats, cmd *cobra.Command, paths []string)
 		}()
 	}
 
-	// Remove the cache first before potentially opening a new one.
-	if cfg.ClearCache {
-		if err := cache.Remove(cfg.TreeRoot); err != nil {
-			return fmt.Errorf("failed to clear cache: %w", err)
-		}
-	}
-
 	var db *bolt.DB
 
 	// open the db unless --no-cache was specified
@@ -101,6 +92,15 @@ func Run(v *viper.Viper, statz *stats.Stats, cmd *cobra.Command, paths []string)
 		}()
 	}
 
+	if db != nil {
+		// clear the cache if desired
+		if cfg.ClearCache {
+			if err = cache.Clear(db); err != nil {
+				return fmt.Errorf("failed to clear cache: %w", err)
+			}
+		}
+	}
+
 	// create an overall app context
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
@@ -124,10 +124,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 +209,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
-}
diff --git a/nix/packages/treefmt/formatters.nix b/nix/packages/treefmt/formatters.nix
index a515f44..28b50f9 100644
--- a/nix/packages/treefmt/formatters.nix
+++ b/nix/packages/treefmt/formatters.nix
@@ -45,16 +45,4 @@ with pkgs; [
       done
     '';
   })
-  (pkgs.writeShellApplication {
-    name = "test-fmt-delayed-append";
-    text = ''
-      DELAY="$1"
-      shift
-
-      # sleep first
-      sleep "$DELAY"
-
-      test-fmt-append "$@"
-    '';
-  })
 ]
diff --git a/walk/cache/cache.go b/walk/cache/cache.go
index 59acc18..44c510f 100644
--- a/walk/cache/cache.go
+++ b/walk/cache/cache.go
@@ -4,7 +4,6 @@ import (
 	"crypto/sha256"
 	"encoding/hex"
 	"fmt"
-	"os"
 	"time"
 
 	"github.com/adrg/xdg"
@@ -15,33 +14,26 @@ const (
 	bucketPaths = "paths"
 )
 
-// Path returns a unique local cache file path for the given root string, using its SHA-256 hash.
-func Path(root string) (string, error) {
+func Open(root string) (*bolt.DB, error) {
+	var (
+		err  error
+		path string
+	)
+
+	// Otherwise, the database will be located in `XDG_CACHE_DIR/treefmt/eval-cache/<name>.db`, where <name> is
+	// determined by hashing the treeRoot path.
+	// This associates a given treeRoot with a given instance of the cache.
 	digest := sha256.Sum256([]byte(root))
 
 	name := hex.EncodeToString(digest[:])
-
-	path, err := xdg.CacheFile(fmt.Sprintf("treefmt/eval-cache/%v.db", name))
-	if err != nil {
-		return "", fmt.Errorf("could not resolve local path for the cache: %w", err)
-	}
-
-	return path, nil
-}
-
-// Open initialises and opens a Bolt database for the specified root path.
-// Returns a pointer to the opened database or an error if initialisation fails.
-func Open(root string) (*bolt.DB, error) {
-	// determine the db location
-	path, err := Path(root)
-	if err != nil {
-		return nil, err
+	if path, err = xdg.CacheFile(fmt.Sprintf("treefmt/eval-cache/%v.db", name)); err != nil {
+		return nil, fmt.Errorf("could not resolve local path for the cache: %w", err)
 	}
 
 	// open db
 	db, err := bolt.Open(path, 0o600, &bolt.Options{Timeout: 1 * time.Second})
 	if err != nil {
-		return nil, fmt.Errorf("failed to open cache db at %s: %w", path, err)
+		return nil, fmt.Errorf("failed to open cache db: %w", err)
 	}
 
 	// ensure bucket exist
@@ -64,19 +56,28 @@ func PathsBucket(tx *bolt.Tx) *bolt.Bucket {
 	return tx.Bucket([]byte("paths"))
 }
 
-func Remove(root string) error {
-	// determine the db location
-	path, err := Path(root)
-	if err != nil {
-		return err
+func deleteAll(bucket *bolt.Bucket) error {
+	c := bucket.Cursor()
+	for k, v := c.First(); !(k == nil && v == nil); k, v = c.Next() {
+		if err := c.Delete(); err != nil {
+			return fmt.Errorf("failed to remove cache entry for key %s: %w", string(k), err)
+		}
 	}
 
-	// Remove any db which might already exist.
-	// If a treefmt process is currently running with a db open at the same location, it will continue to function
-	// as normal, however, when it exits the disk space its inode was referencing will be reclaimed.
-	// This will not work on Windows if we ever support it.
-	if err = os.Remove(path); !(err == nil || os.IsNotExist(err)) {
-		return fmt.Errorf("failed to remove cache db at %s: %w", path, err)
+	return nil
+}
+
+func Clear(db *bolt.DB) error {
+	err := db.Update(func(tx *bolt.Tx) error {
+		err := deleteAll(PathsBucket(tx))
+		if err != nil {
+			return fmt.Errorf("failed to clear cache: %w", err)
+		}
+
+		return nil
+	})
+	if err != nil {
+		return fmt.Errorf("failed to clear cache: %w", err)
 	}
 
 	return nil
diff --git a/walk/walk.go b/walk/walk.go
index eeecb81..fdb73af 100644
--- a/walk/walk.go
+++ b/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,40 +270,52 @@ 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)
 		}
 	}
 
@@ -299,3 +323,20 @@ func NewCompositeReader(
 		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
+}


Test patch:
diff --git a/cmd/root_test.go b/cmd/root_test.go
index 4d4b68f..bb664a7 100644
--- a/cmd/root_test.go
+++ b/cmd/root_test.go
@@ -24,7 +24,6 @@ import (
 	"github.com/numtide/treefmt/v2/walk"
 	cp "github.com/otiai10/copy"
 	"github.com/stretchr/testify/require"
-	"golang.org/x/sync/errgroup"
 )
 
 func TestOnUnmatched(t *testing.T) {
@@ -1439,9 +1438,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 +1922,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,83 +2135,41 @@ 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,
+		}),
 	)
-}
-
-func TestConcurrentInvocation(t *testing.T) {
-	as := require.New(t)
-
-	tempDir := test.TempExamples(t)
-	configPath := filepath.Join(tempDir, "/treefmt.toml")
-
-	test.ChangeWorkDir(t, tempDir)
-
-	cfg := &config.Config{
-		FormatterConfigs: map[string]*config.Formatter{
-			"echo": {
-				Command:  "echo",
-				Includes: []string{"*"},
-			},
-			"slow": {
-				Command: "test-fmt-delayed-append",
-				// connect timeout for the db is 1 second
-				// wait 2 seconds before appending ' ' to each provided path
-				Options:  []string{"2", " "},
-				Includes: []string{"*"},
-			},
-		},
-	}
-
-	eg := errgroup.Group{}
-
-	// concurrent invocation with one slow instance and one not
-
-	eg.Go(func() error {
-		treefmt(t,
-			withArgs("--formatters", "slow"),
-			withConfig(configPath, cfg),
-			withNoError(t),
-		)
-
-		return nil
-	})
-
-	time.Sleep(500 * time.Millisecond)
 
+	// Verify we can format a specific directory that is a symlink.
 	treefmt(t,
-		withArgs("--formatters", "echo"),
-		withConfig(configPath, cfg),
-		withError(func(as *require.Assertions, err error) {
-			as.ErrorContains(err, "failed to open cache")
+		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,
 		}),
 	)
 
-	as.NoError(eg.Wait())
-
-	// concurrent invocation with one slow instance and one configured to clear the cache
-
-	eg.Go(func() error {
-		treefmt(t,
-			withArgs("--formatters", "slow"),
-			withConfig(configPath, cfg),
-			withNoError(t),
-		)
-
-		return nil
-	})
-
-	time.Sleep(500 * time.Millisecond)
-
+	// Verify we can format the current directory (which is a symlink!).
 	treefmt(t,
-		withArgs("-c", "--formatters", "echo"),
-		withConfig(configPath, cfg),
+		withArgs("-c", "."),
 		withNoError(t),
+		withStats(t, map[stats.Type]int{
+			stats.Traversed: 32,
+			stats.Matched:   32,
+			stats.Formatted: 32,
+			stats.Changed:   0,
+		}),
 	)
-
-	as.NoError(eg.Wait())
 }
 
 type options struct {
diff --git a/test/examples/symlink-to-yaml-dir b/test/examples/symlink-to-yaml-dir
new file mode 120000
index 0000000..fb76888
--- /dev/null
+++ b/test/examples/symlink-to-yaml-dir
@@ -0,0 +1,1 @@
+yaml/
\ No newline at end of file