Bubble errors up from worker goroutines; exit non-zero on failure

file_handling.go

@@ -1,6 +1,7 @@
 package main
 
 import (
+	"fmt"
 	"io"
 	"log"
 	"os"
@@ -15,12 +16,15 @@ type File struct {
 	info os.FileInfo
 }
 
-func NewFile(path string) *File {
+// NewFile resolves path to an absolute path and wraps it in a *File. It
+// returns an error if the working directory cannot be determined (the only
+// failure mode of filepath.Abs).
+func NewFile(path string) (*File, error) {
 	absPath, err := filepath.Abs(path)
 	if err != nil {
-		log.Fatalf("Unable to resolve absolute path of %v: %v", path, err)
+		return nil, fmt.Errorf("resolve absolute path of %v: %w", path, err)
 	}
-	return &File{Path: absPath}
+	return &File{Path: absPath}, nil
 }
 
 func (f *File) Base() string {
@@ -31,58 +35,81 @@ func (f *File) Dir() string {
 	return filepath.Dir(f.Path)
 }
 
-func (f *File) Info() os.FileInfo {
+// Info lazily stats the file and caches the result. It returns an error if
+// the underlying os.Stat fails.
+func (f *File) Info() (os.FileInfo, error) {
 	if f.info == nil {
 		stat, err := os.Stat(f.Path)
 		if err != nil {
-			log.Fatalf("Failed to stat %v: %v", f.Path, err)
+			return nil, fmt.Errorf("stat %v: %w", f.Path, err)
 		}
 		f.info = stat
 	}
-	return f.info
+	return f.info, nil
 }
 
-func (f *File) Mode() os.FileMode {
-	return f.Info().Mode()
+// Mode returns the cached mode bits. It is only safe to call after Info() has
+// succeeded; callers that have a *File handed to them by the walker can rely
+// on that precondition because the walker calls Info() before dispatching.
+func (f *File) Mode() (os.FileMode, error) {
+	info, err := f.Info()
+	if err != nil {
+		return 0, err
+	}
+	return info.Mode(), nil
 }
 
-// Read the file into a string.
-func (f *File) Read() string {
+// Read reads the file into a string, or returns the empty string for binary
+// files. An error indicates the file could not be opened or fully read; the
+// caller should log-and-skip rather than abort.
+func (f *File) Read() (string, error) {
 	handle, err := os.Open(f.Path)
 	if err != nil {
-		log.Fatalf("Unable to open %v: %v", f.Path, err)
+		return "", fmt.Errorf("open %v: %w", f.Path, err)
 	}
 	defer handle.Close()
 
 	// Check if the file looks like text before reading the entire file.
 	var buf [1024]byte
 	n, err := handle.Read(buf[0:])
 	if err != nil || !util.IsText(buf[0:n]) {
-		return ""
+		return "", nil
 	}
 
 	// Reset file handle so we can read the entire file.
 	if _, err := handle.Seek(0, io.SeekStart); err != nil {
-		log.Fatalf("Failed to seek back to beginning of %v: %v", f.Path, err)
+		return "", fmt.Errorf("seek to start of %v: %w", f.Path, err)
 	}
 
 	builder := new(strings.Builder)
 	if _, err := io.Copy(builder, handle); err != nil {
-		log.Fatalf("Failed to read %v to a string: %v", f.Path, err)
+		return "", fmt.Errorf("read %v: %w", f.Path, err)
 	}
-	return builder.String()
+	return builder.String(), nil
 }
 
-// Write content to file atomically, by writing it to a temporary file first,
-// and then moving it to the destination, overwriting the original.
-func (f *File) Write(content string) {
+// Write atomically replaces the file with content, via a temp file + rename.
+// A deferred os.Remove(tempName) ensures the temp file is cleaned up if any
+// step after its creation fails (including the rename); on success the remove
+// is a no-op because the file has already been renamed away.
+func (f *File) Write(content string) error {
+	mode, err := f.Mode()
+	if err != nil {
+		return err
+	}
+
 	tempName := filepath.Join(f.Dir(), RandomString(20))
-	if err := os.WriteFile(tempName, []byte(content), f.Mode()); err != nil {
-		log.Fatalf("Error creating tempfile in %v: %v", f.Dir(), err)
+	if err := os.WriteFile(tempName, []byte(content), mode); err != nil {
+		return fmt.Errorf("create tempfile in %v: %w", f.Dir(), err)
 	}
+	// Make sure the temp file is removed if the rename below fails. On
+	// success, the rename has already moved the file to f.Path so this is
+	// a no-op (we deliberately ignore the not-exist error).
+	defer os.Remove(tempName)
 
 	log.Printf("Rewriting %v", f.Path)
 	if err := os.Rename(tempName, f.Path); err != nil {
-		log.Fatalf("Unable to atomically move temp file %v to %v: %v", tempName, f.Path, err)
+		return fmt.Errorf("atomically move temp file %v to %v: %w", tempName, f.Path, err)
 	}
+	return nil
 }

file_handling_test.go

@@ -5,11 +5,7 @@ import (
 	"testing"
 )
 
-// TestNewFile exercises NewFile's path-resolution behavior. It does NOT cover
-// the filepath.Abs error path: NewFile currently calls log.Fatalf on that
-// failure, which would kill the test binary, and that surface is not reachable
-// on most platforms in any case. The error path will become testable when
-// NewFile is refactored to return an error (see issue #6).
+// TestNewFile exercises NewFile's path-resolution behavior.
 func TestNewFile(t *testing.T) {
 	tmp := t.TempDir()
 
@@ -65,7 +61,10 @@ func TestNewFile(t *testing.T) {
 				want = abs
 			}
 
-			got := NewFile(tc.input)
+			got, err := NewFile(tc.input)
+			if err != nil {
+				t.Fatalf("NewFile(%q) returned unexpected error: %v", tc.input, err)
+			}
 			if got == nil {
 				t.Fatalf("NewFile(%q) returned nil", tc.input)
 			}