From 30e1ecd6fba32471ce0f932c2f4476a29595044e Mon Sep 17 00:00:00 2001
From: Brian McGee <brian@bmcgee.ie>
Date: Thu, 23 Jan 2025 16:52:44 +0000
Subject: [PATCH] feat: improve error handling in format Run method

When reviewing #511, I noticed there was some shadowing of variables and edge cases which were not being handled gracefully.

Signed-off-by: Brian McGee <brian@bmcgee.ie>
---
 cmd/format/format.go | 63 +++++++++++++++++++++++++++++++-------------
 cmd/root_test.go     |  4 +--
 2 files changed, 46 insertions(+), 21 deletions(-)

diff --git a/cmd/format/format.go b/cmd/format/format.go
index 041a28de..3da196df 100644
--- a/cmd/format/format.go
+++ b/cmd/format/format.go
@@ -167,45 +167,70 @@ func Run(v *viper.Viper, statz *stats.Stats, cmd *cobra.Command, paths []string)
 	// start traversing
 	files := make([]*walk.File, BatchSize)
 
+	var (
+		n                  int
+		readErr, formatErr error
+	)
+
 	for {
 		// read the next batch
-		readCtx, cancel := context.WithTimeout(ctx, 1*time.Second)
-		n, err := walker.Read(readCtx, files)
+		readCtx, cancelRead := context.WithTimeout(ctx, 1*time.Second)
+
+		n, readErr = walker.Read(readCtx, files)
+		log.Debugf("read %d files", n)
 
 		// ensure context is cancelled to release resources
-		cancel()
+		cancelRead()
 
-		// format
-		if err := formatter.Apply(ctx, files[:n]); err != nil {
-			return fmt.Errorf("formatting failure: %w", err)
+		// format any files that were read before processing the read error
+		if formatErr = formatter.Apply(ctx, files[:n]); formatErr != nil {
+			break
 		}
 
-		if errors.Is(err, io.EOF) {
-			// we have finished traversing
+		// stop reading files if there was a read error
+		if readErr != nil {
 			break
-		} else if err != nil {
-			// something went wrong
-			return fmt.Errorf("failed to read files: %w", err)
 		}
 	}
 
-	// finalize formatting
-	formatErr := formatter.Close(ctx)
+	// finalize formatting (there could be formatting tasks in-flight)
+	formatCloseErr := formatter.Close(ctx)
 
 	// close the walker, ensuring any pending file release hooks finish
-	if err = walker.Close(); err != nil {
-		return fmt.Errorf("failed to close walker: %w", err)
-	}
+	walkerCloseErr := walker.Close()
 
 	// print stats to stderr
 	if !cfg.Quiet {
 		statz.PrintToStderr()
 	}
 
+	// process errors
+
+	//nolint:gocritic
+	if errors.Is(readErr, io.EOF) {
+		// nothing more to read, reset the error and break out of the read loop
+		log.Debugf("no more files to read")
+	} else if errors.Is(readErr, context.DeadlineExceeded) {
+		// the read timed-out
+		return errors.New("timeout reading files")
+	} else if readErr != nil {
+		// something unexpected happened
+		return fmt.Errorf("failed to read files: %w", readErr)
+	}
+
 	if formatErr != nil {
-		// return an error if any formatting failures were detected
-		return formatErr //nolint:wrapcheck
-	} else if cfg.FailOnChange && statz.Value(stats.Changed) != 0 {
+		return fmt.Errorf("failed to format files: %w", formatErr)
+	}
+
+	if formatCloseErr != nil {
+		return fmt.Errorf("failed to finalise formatting: %w", formatCloseErr)
+	}
+
+	if walkerCloseErr != nil {
+		return fmt.Errorf("failed to close walker: %w", walkerCloseErr)
+	}
+
+	if cfg.FailOnChange && statz.Value(stats.Changed) != 0 {
 		// if fail on change has been enabled, check that no files were actually changed, throwing an error if so
 		return ErrFailOnChange
 	}
diff --git a/cmd/root_test.go b/cmd/root_test.go
index 628a69b4..d47d3df9 100644
--- a/cmd/root_test.go
+++ b/cmd/root_test.go
@@ -1433,12 +1433,12 @@ func TestGit(t *testing.T) {
 		}),
 	)
 
-	// try with a path not in the git index, e.g. it is skipped
+	// try with a path not in the git index
 	_, err := os.Create(filepath.Join(tempDir, "foo.txt"))
 	as.NoError(err)
 
 	treefmt(t,
-		withArgs("haskell", "foo.txt"),
+		withArgs("haskell", "foo.txt", "-vv"),
 		withConfig(configPath, cfg),
 		withNoError(t),
 		withStats(t, map[stats.Type]int{