Repository: numtide/treefmt
Pull request #528 (2025-03-03 11:05:01)

fix: ignoring symlinks
@brianmcgee : I tried the following:

- Ignore symlinks on the Git reader.
- Adding a symlink to the examples, to test if it gets ignored everywhere this examples are used.
- Add resolving symlinks on the `Run` functions to pass the resolved paths to the readers, such that the readers `Filesystem` or `Git` can ignore symlinks always. `treefmt symlink.yaml`  or `treefmt /a-symlink/b/c/d.yaml` should be allowed.

See some comments I need help on which is unclear to me.

Fixes: #495 



gabyx: @brianmcgee : I have question to the test here:

Why do we even test to run in the `.git` folder? You will never want that IMO, or is this really feature?
Shouldn't treefmt relaly ignore any `.git` directory (got from `git rev-parse --git-common-dir` in the tree root)

https://github.com/numtide/treefmt/blob/bdd8687ef1e83868ed3614b77301f8aa63b4c3ef/cmd/root_test.go#L1401
brianmcgee: > Why do we even test to run in the `.git` folder? You will never want that IMO, or is this really a feature? Shouldn't treefmt ignore any `.git` directory (got from `git rev-parse --git-common-dir` in the tree root)

In that particular test, it's checking that if we momentarily switch to the filesystem walker, the `.git` directory gets picked up and traversed. It's not that we want to traverse the `.git` directory per se, but that it should traverse any directory when the filesystem walker is enabled. 



gabyx: > > Why do we even test to run in the `.git` folder? You will never want that IMO, or is this really a feature? Shouldn't treefmt ignore any `.git` directory (got from `git rev-parse --git-common-dir` in the tree root)
> 
> 
> 
> In that particular test, it's checking that if we momentarily switch to the filesystem walker, the `.git` directory gets picked up and traversed. It's not that we want to traverse the `.git` directory per se, but that it should traverse any directory when the filesystem walker is enabled. 
>
Ah ok.

I need to still refactor the symlink resolving code in the Run function, its not good yet. I think the only correct way is to us EvalSymlinks and check for errors and make the test pass. As this is only done on path inputs it should not degrade performance.
gabyx: @brianmcgee: The thins is done. Happy for a review what you think about the behavior. Hope its ok like that.
brianmcgee: Thanks for this, I'll try and review it in the next few days.
brianmcgee: @gabyx please rebase on latest `main`
gabyx: @brianmcgee: Did the changes and rebased.
brianmcgee: Thanks :pray: I'll give it another review in the next day or so

Patch:
diff --git a/cmd/format/format.go b/cmd/format/format.go
index 3da196d..e216e0d 100644
--- a/cmd/format/format.go
+++ b/cmd/format/format.go
@@ -88,8 +88,8 @@ func Run(v *viper.Viper, statz *stats.Stats, cmd *cobra.Command, paths []string)
 
 		// ensure db is closed after we're finished
 		defer func() {
-			if err := db.Close(); err != nil {
-				log.Errorf("failed to close cache: %v", err)
+			if closeErr := db.Close(); closeErr != nil {
+				log.Errorf("failed to close cache: %v", closeErr)
 			}
 		}()
 	}
@@ -126,30 +126,8 @@ 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")
 	}
 
-	// checks all paths are contained within the tree root and exist
-	// also "normalize" paths so they're relative to cfg.TreeRoot
-	for i, path := range paths {
-		absolutePath, err := filepath.Abs(path)
-		if err != nil {
-			return fmt.Errorf("error computing absolute path of %s: %w", path, err)
-		}
-
-		relativePath, err := filepath.Rel(cfg.TreeRoot, absolutePath)
-		if err != nil {
-			return fmt.Errorf("error computing relative path from %s to %s: %w", cfg.TreeRoot, absolutePath, err)
-		}
-
-		if strings.HasPrefix(relativePath, "..") {
-			return fmt.Errorf("path %s not inside the tree root %s", path, cfg.TreeRoot)
-		}
-
-		paths[i] = relativePath
-
-		if walkType != walk.Stdin {
-			if _, err = os.Stat(absolutePath); err != nil {
-				return fmt.Errorf("path %s not found", path)
-			}
-		}
+	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
@@ -237,3 +215,40 @@ 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 {
+	for i, path := range paths {
+		log.Errorf("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
+		}
+
+		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
+}
diff --git a/walk/git.go b/walk/git.go
index cb5c4c8..84cbbe7 100644
--- a/walk/git.go
+++ b/walk/git.go
@@ -91,16 +91,22 @@ LOOP:
 
 				g.log.Debugf("processing file: %s", path)
 
-				info, err := os.Stat(path)
-				if os.IsNotExist(err) {
+				info, err := os.Lstat(path)
+
+				switch {
+				case os.IsNotExist(err):
 					// the underlying file might have been removed
 					g.log.Warnf(
 						"Path %s is in the worktree but appears to have been removed from the filesystem", path,
 					)
 
 					continue
-				} else if err != nil {
+				case err != nil:
 					return n, fmt.Errorf("failed to stat %s: %w", path, err)
+				case info.Mode()&os.ModeSymlink == os.ModeSymlink:
+					// we skip reporting symlinks stored in Git, they should
+					// point to local files which we would list anyway.
+					continue
 				}
 
 				files[n] = &File{
@@ -108,6 +114,7 @@ LOOP:
 					RelPath: filepath.Join(g.path, entry),
 					Info:    info,
 				}
+
 				n++
 			} else {
 				// nothing more to read
diff --git a/walk/walk.go b/walk/walk.go
index 48507ae..eeecb81 100644
--- a/walk/walk.go
+++ b/walk/walk.go
@@ -234,6 +234,9 @@ func NewReader(
 	return reader, err
 }
 
+// NewCompositeReader returns a composite reader for the `root` and all `paths`. It
+// never follows symlinks.
+//
 //nolint:ireturn
 func NewCompositeReader(
 	walkType Type,
@@ -268,16 +271,21 @@ func NewCompositeReader(
 		// create a clean absolute path
 		path := filepath.Clean(filepath.Join(root, relPath))
 
-		// check the path exists
+		// check the path exists (don't follow symlinks)
 		info, err = os.Lstat(path)
 		if err != nil {
 			return nil, fmt.Errorf("failed to stat %s: %w", path, err)
 		}
 
-		if info.IsDir() {
+		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():
 			// for directories, we honour the walk type as we traverse them
 			readers[idx], err = NewReader(walkType, root, relPath, db, statz)
-		} else {
+		default:
 			// for files, we enforce a simple filesystem read
 			readers[idx], err = NewReader(Filesystem, root, relPath, db, statz)
 		}


Test patch:
diff --git a/cmd/root_test.go b/cmd/root_test.go
index ef99c89..9dfb2c2 100644
--- a/cmd/root_test.go
+++ b/cmd/root_test.go
@@ -1411,9 +1411,9 @@ func TestGit(t *testing.T) {
 		withConfig(configPath, cfg),
 		withNoError(t),
 		withStats(t, map[stats.Type]int{
-			stats.Traversed: 78,
-			stats.Matched:   78,
-			stats.Formatted: 48, // the echo formatter should only be applied to the new files
+			stats.Traversed: 79,
+			stats.Matched:   79,
+			stats.Formatted: 49, // the echo formatter should only be applied to the new files
 			stats.Changed:   0,
 		}),
 	)
@@ -1463,7 +1463,7 @@ func TestGit(t *testing.T) {
 		withArgs("-C", tempDir, "haskell", "foo"),
 		withConfig(configPath, cfg),
 		withError(func(as *require.Assertions, err error) {
-			as.ErrorContains(err, "path foo not found")
+			as.ErrorContains(err, "foo not found")
 		}),
 	)
 
@@ -1592,7 +1592,7 @@ func TestPathsArg(t *testing.T) {
 	treefmt(t,
 		withArgs("rust/src/main.rs", "haskell/Nested/Bar.hs"),
 		withError(func(as *require.Assertions, err error) {
-			as.ErrorContains(err, "path haskell/Nested/Bar.hs not found")
+			as.ErrorContains(err, "Bar.hs not found")
 		}),
 	)
 
@@ -1610,7 +1610,7 @@ func TestPathsArg(t *testing.T) {
 
 	// specify a relative path outside the tree root
 	relativeExternalPath := "../outside_tree.go"
-	as.FileExists(relativeExternalPath, "exernal file must exist")
+	as.FileExists(relativeExternalPath, "external file must exist")
 
 	treefmt(t,
 		withArgs(relativeExternalPath),
@@ -1842,7 +1842,7 @@ func TestRunInSubdir(t *testing.T) {
 			treefmt(t,
 				withArgs("-c", "go/main.go", "haskell/Nested/Foo.hs"),
 				withError(func(as *require.Assertions, err error) {
-					as.ErrorContains(err, "path go/main.go not found")
+					as.ErrorContains(err, "go/main.go not found")
 				}),
 			)
 
diff --git a/test/examples/yaml/test-rel-symlink.yaml b/test/examples/yaml/test-rel-symlink.yaml
new file mode 120000
index 0000000..c40da6f
--- /dev/null
+++ b/test/examples/yaml/test-rel-symlink.yaml
@@ -0,0 +1,1 @@
+../yaml/test.yaml
\ No newline at end of file