fix: ignoring symlinks (#528)

Co-authored-by: Brian McGee <brian@bmcgee.ie>

Author: gabyx

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
+}

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")
 				}),
 			)
 

test/examples/yaml/test-rel-symlink.yaml

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

walk/git.go

@@ -91,23 +91,30 @@ 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{
 					Path:    path,
 					RelPath: filepath.Join(g.path, entry),
 					Info:    info,
 				}
+
 				n++
 			} else {
 				// nothing more to read

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)
 		}