From 797683e2e58aa066c18e1df9bcfcaf7eb88db6c3 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 18 May 2026 02:54:48 +0000 Subject: [PATCH 1/3] Bubble errors up from worker goroutines; exit non-zero on failure Replace every log.Fatal* in find_replace.go and file_handling.go with returned errors so deferred cleanup can run, then collect errors across walker goroutines and surface them at main. Architecture: - File methods (NewFile, Info, Read, Write, plus a new error-returning Mode) now return (T, error). The walker calls Info on each child so Write can rely on cached Mode without re-statting. - findReplace.{WalkDir,HandleFile,RenameFile,ReplaceContents} return errors (WalkDir records them on an embedded accumulator so its goroutines can stay fire-and-forget). - errAccumulator is a sync.Mutex-guarded []error with an errors.Join exit point. Each error is also log.Print'd at the point of failure so the existing operator-visible stderr UX is preserved; the join exists so main can scrape stderr and so errors.Is unwraps the whole chain. - main is now a thin os.Exit wrapper around a testable run(args, stderr) that returns 1 on bad arg count or any traversal error and 0 on a clean walk. - File.Write now defer-Removes its temp file immediately after creating it; on a successful rename the file no longer exists at tempName so the deferred remove is a no-op, but on a rename failure the temp file is cleaned up instead of being leaked. New tests (each was confirmed to fail for the right reason before the matching production change was finalized): - TestWalkDir_PermissionDeniedSubdirContinues: a chmod-0 subdirectory no longer aborts the walk; the sibling subtree is rewritten and the walker records an fs.ErrPermission referencing the failing path. Skips under root (where chmod 0 is bypassed) and Windows. - TestRenameFile_ReturnsErrorOnExistingDestination: an occupied destination is refused with an error, not log.Fatal. - TestWalkDir_BadRenameTargetDoesNotAbortSiblings: a sibling whose post-rename name is already occupied does not abort the rest of the tree; the walker logs and records the failure and continues. - TestWriteCleansUpTempFileOnRenameFailure: forcing the rename step to fail (target is a non-empty directory; deterministic across users) leaves no stray temp file behind. - TestRun_{ExitsZeroOnSuccess,ExitsNonZeroOnTraversalError, BadArgCountPrintsUsage}: run() returns 0 on a clean walk, non-zero when any error was recorded, and non-zero with a usage line on bad arg count. Closes #6 Closes #11 Closes #5 https://claude.ai/code/session_01Tep5t8h97Q9KKbpLMbUEJr --- file_handling.go | 69 +++++--- file_handling_test.go | 11 +- find_replace.go | 186 +++++++++++++++------ find_replace_test.go | 368 ++++++++++++++++++++++++++++++++++++++---- 4 files changed, 533 insertions(+), 101 deletions(-) diff --git a/file_handling.go b/file_handling.go index 9844c3a..7c0b1a7 100644 --- a/file_handling.go +++ b/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,26 +35,37 @@ 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() @@ -58,31 +73,43 @@ func (f *File) Read() string { 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 } diff --git a/file_handling_test.go b/file_handling_test.go index 0296964..91ee538 100644 --- a/file_handling_test.go +++ b/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) } diff --git a/find_replace.go b/find_replace.go index a2f2c65..f228854 100644 --- a/find_replace.go +++ b/find_replace.go @@ -2,6 +2,8 @@ package main import ( "errors" + "fmt" + "io" "log" "math/rand" "os" @@ -16,6 +18,45 @@ import ( type findReplace struct { find string replace string + + // errs accumulates non-fatal errors that occurred during a walk. The + // walker logs each error at the point of failure (preserving the + // operator-visible UX) and appends it here so main can surface a + // non-zero exit code at the end. + errs errAccumulator +} + +// errAccumulator is a tiny thread-safe collector for errors that occur in +// concurrent walker goroutines. It is intentionally small: just enough to +// preserve "log everything, exit non-zero if anything failed" semantics +// without committing the codebase to a particular concurrency primitive +// (see issue #7 for the eventual bounded worker pool). +type errAccumulator struct { + mu sync.Mutex + errs []error +} + +// add records err. A nil err is ignored so callers can write +// `acc.add(fn())` without a guard. +func (a *errAccumulator) add(err error) { + if err == nil { + return + } + a.mu.Lock() + defer a.mu.Unlock() + a.errs = append(a.errs, err) +} + +// err returns the accumulated errors joined with errors.Join, or nil if +// nothing was recorded. The returned error is safe to unwrap with errors.Is +// / errors.As over every accumulated error. +func (a *errAccumulator) err() error { + a.mu.Lock() + defer a.mu.Unlock() + if len(a.errs) == 0 { + return nil + } + return errors.Join(a.errs...) } // main processes command line arguments, builds the context struct, and begins @@ -27,45 +68,80 @@ type findReplace struct { // • baseName: the relative name of a file, without a directory // • path: the relative path to a specific file or directory, including both dirName and baseName func main() { - // Remove date/time from logging output + os.Exit(run(os.Args, os.Stderr)) +} + +// run is the testable body of main. It returns the process exit code: 0 on +// clean success, 1 if argument parsing failed or any traversal error was +// recorded. Output documented in the README (Renaming/Rewriting lines) still +// goes to log.Default(); usage and aggregated error summaries go to stderr. +func run(args []string, stderr io.Writer) int { + // Remove date/time from logging output. log.SetFlags(0) + // Vestigial seed for the math/rand-based temp file names; see issue #3. rand.Seed(time.Now().UnixNano()) - if len(os.Args) != 3 { - log.Fatal("Usage: find-replace FIND REPLACE") + if len(args) != 3 { + fmt.Fprintln(stderr, "Usage: find-replace FIND REPLACE") + return 1 } - find := os.Args[1] - replace := os.Args[2] - - fr := findReplace{find: find, replace: replace} + fr := findReplace{find: args[1], replace: args[2]} // Recursively explore the hierarchy depth first, rewrite files as needed, // and rename files last (after we don't have to revisit them). - // path.filepath.WalkDir() won't work here because it walks files - // alphabetically, breadth-first (and you'd be renaming files that you - // haven't explored yet). - - fr.WalkDir(NewFile(".")) + // filepath.WalkDir won't work here because it walks files alphabetically, + // breadth-first (and you'd be renaming files that you haven't explored + // yet). + root, err := NewFile(".") + if err != nil { + fmt.Fprintln(stderr, err) + return 1 + } + fr.WalkDir(root) + + if err := fr.errs.err(); err != nil { + // Each individual error has already been printed at the point of + // failure; the join here is for completeness in case a caller is + // scraping stderr. + fmt.Fprintln(stderr, err) + return 1 + } + return 0 } -// Walks files in the directory given by dirName, which is a relative path to a -// directory. Calls HandleFile for each file it finds, if it's not ignored. +// WalkDir lists files in the directory given by f and dispatches each child +// to HandleFile in its own goroutine. Per-child errors are logged at their +// failure site and recorded on fr so main can surface a non-zero exit code. +// A failure to read the directory itself is recorded and returned to the +// caller, but does not abort the rest of the walk in any other subtree. func (fr *findReplace) WalkDir(f *File) { var wg sync.WaitGroup // List the files in this directory. files, err := os.ReadDir(f.Path) if err != nil { - log.Fatalf("Unable to read directory: %v", err) + wrapped := fmt.Errorf("read directory %v: %w", f.Path, err) + log.Print(wrapped) + fr.errs.add(wrapped) + return } for _, file := range files { - childFile := NewFile(filepath.Join(f.Path, file.Name())) + childPath := filepath.Join(f.Path, file.Name()) + childFile, err := NewFile(childPath) + if err != nil { + log.Print(err) + fr.errs.add(err) + continue + } wg.Add(1) go func() { defer wg.Done() - fr.HandleFile(childFile) + if err := fr.HandleFile(childFile); err != nil { + log.Print(err) + fr.errs.add(err) + } }() } @@ -75,50 +151,66 @@ func (fr *findReplace) WalkDir(f *File) { // HandleFile immediately recurses depth-first into directories it finds, // otherwise calls ReplaceContents for regular files. When either operation is // complete, the file is renamed (if necessary) since no subsequent operations -// will need to access it again. -func (fr *findReplace) HandleFile(f *File) { +// will need to access it again. Errors from ReplaceContents are not fatal to +// the rename step; the failure is returned so the walker can log it and +// continue with siblings. +func (fr *findReplace) HandleFile(f *File) error { + info, err := f.Info() + if err != nil { + return err + } + // If file is a directory, recurse immediately (depth-first). - if f.Info().IsDir() { + if info.IsDir() { // Ignore certain directories if f.Base() == ".git" { - return + return nil } fr.WalkDir(f) } else { - // Replace the contents of regular files - fr.ReplaceContents(f) + // Replace the contents of regular files. + if err := fr.ReplaceContents(f); err != nil { + return err + } } - // Rename the file now that we're otherwise done with it - fr.RenameFile(f) + // Rename the file now that we're otherwise done with it. + return fr.RenameFile(f) } -// RenameFile renames a file if the destination file name does not already -// exist. -func (fr *findReplace) RenameFile(f *File) { - newBaseName := strings.Replace(f.Base(), fr.find, fr.replace, -1) +// RenameFile renames f to its post-replacement name if (a) the name actually +// changes and (b) no file already exists at the destination. It returns an +// error if the destination is occupied or if the os.Rename itself fails. +func (fr *findReplace) RenameFile(f *File) error { + newBaseName := strings.ReplaceAll(f.Base(), fr.find, fr.replace) + if f.Base() == newBaseName { + return nil + } + newPath := filepath.Join(f.Dir(), newBaseName) + if _, err := os.Stat(newPath); err == nil { + return fmt.Errorf("refusing to rename %v to %v: %v already exists", f.Path, newBaseName, newPath) + } else if !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("stat rename destination %v: %w", newPath, err) + } - if f.Base() != newBaseName { - if _, err := os.Stat(newPath); errors.Is(err, os.ErrNotExist) { - log.Printf("Renaming %v to %v", f.Path, newBaseName) - if err := os.Rename(f.Path, newPath); err != nil { - log.Fatalf("Unable to rename %v to %v: %v", f.Path, newBaseName, err) - } - } else { - log.Fatalf("Refusing to rename %v to %v: %v already exists", f.Path, newBaseName, newPath) - } + log.Printf("Renaming %v to %v", f.Path, newBaseName) + if err := os.Rename(f.Path, newPath); err != nil { + return fmt.Errorf("rename %v to %v: %w", f.Path, newBaseName, err) } + return nil } -// Replaces the contents of the given file, using the find & replace values in -// context. -func (fr *findReplace) ReplaceContents(f *File) { - // Find & replace the contents of text files. Binary-looking files return - // an empty string and will be skipped here. - content := f.Read() - if strings.Contains(content, fr.find) { - newContent := strings.Replace(content, fr.find, fr.replace, -1) - f.Write(newContent) +// ReplaceContents rewrites the file at f if its contents contain the find +// string. Binary-looking files (where Read returns "") are skipped silently. +func (fr *findReplace) ReplaceContents(f *File) error { + content, err := f.Read() + if err != nil { + return err + } + if !strings.Contains(content, fr.find) { + return nil } + newContent := strings.ReplaceAll(content, fr.find, fr.replace) + return f.Write(newContent) } diff --git a/find_replace_test.go b/find_replace_test.go index e52014e..9a54564 100644 --- a/find_replace_test.go +++ b/find_replace_test.go @@ -1,10 +1,13 @@ package main import ( + "bytes" "errors" + "io/fs" "os" "os/exec" "path/filepath" + "runtime" "strings" "testing" ) @@ -33,7 +36,7 @@ func newTestFile(tb testing.TB, path string, baseName string, content string) *F tb.Fatalf("close %v: %v", f.Name(), err) } - return NewFile(f.Name()) + return newFileOrFatal(tb, f.Name()) } // newTestDir creates a directory in the given directory path, with the given @@ -47,11 +50,32 @@ func newTestDir(tb testing.TB, path string, baseName string) *File { if err != nil { tb.Fatalf("MkdirTemp(%q, %q): %v", path, baseName, err) } - return NewFile(dirPath) + return newFileOrFatal(tb, dirPath) +} + +// newFileOrFatal wraps NewFile for tests that should never see the +// (vanishingly rare) filepath.Abs error. +func newFileOrFatal(tb testing.TB, path string) *File { + tb.Helper() + f, err := NewFile(path) + if err != nil { + tb.Fatalf("NewFile(%q): %v", path, err) + } + return f +} + +// readOrFatal returns the contents of f or fails the test. +func readOrFatal(tb testing.TB, f *File) string { + tb.Helper() + s, err := f.Read() + if err != nil { + tb.Fatalf("Read(%q): %v", f.Path, err) + } + return s } func expectedPathAfterRename(f *File, fr *findReplace) string { - return filepath.Join(f.Dir(), strings.Replace(f.Base(), fr.find, fr.replace, -1)) + return filepath.Join(f.Dir(), strings.ReplaceAll(f.Base(), fr.find, fr.replace)) } /* @@ -81,7 +105,7 @@ func assertFileNonexistent(t *testing.T, f *File) { func assertPathExistsAfterRename(t *testing.T, f *File, expectedPath string) *File { t.Helper() assertFileNonexistent(t, f) - newFile := NewFile(expectedPath) + newFile := newFileOrFatal(t, expectedPath) assertFileExists(t, newFile) return newFile } @@ -147,17 +171,20 @@ func TestWalkDir(t *testing.T) { fr := findReplace{find: find, replace: replace} fr.WalkDir(d) + if err := fr.errs.err(); err != nil { + t.Fatalf("WalkDir reported errors: %v", err) + } // d1: who/ > fo/ d1ExpectedPath := expectedPathAfterRename(d1, &fr) assertPathExistsAfterRename(t, d1, d1ExpectedPath) // d1d1: who/what/ > fo/foat/ - d1d1ExpectedPath := filepath.Join(d1ExpectedPath, strings.Replace(d1d1.Base(), fr.find, fr.replace, -1)) + d1d1ExpectedPath := filepath.Join(d1ExpectedPath, strings.ReplaceAll(d1d1.Base(), fr.find, fr.replace)) assertPathExistsAfterRename(t, d1d1, d1d1ExpectedPath) // d1d1f1: who/what/when > fo/fat/fen (contains "fere") - d1d1f1ExpectedPath := filepath.Join(d1d1ExpectedPath, strings.Replace(d1d1f1.Base(), fr.find, fr.replace, -1)) + d1d1f1ExpectedPath := filepath.Join(d1d1ExpectedPath, strings.ReplaceAll(d1d1f1.Base(), fr.find, fr.replace)) assertPathExistsAfterRename(t, d1d1f1, d1d1f1ExpectedPath) assertNewContentsOfFile(t, d1d1f1ExpectedPath, d1d1f1Contents, find, replace, "fere") @@ -166,11 +193,11 @@ func TestWalkDir(t *testing.T) { assertPathExistsAfterRename(t, d2, d2ExpectedPath) // d2d1: what/when/ - d2d1ExpectedPath := filepath.Join(d2ExpectedPath, strings.Replace(d2d1.Base(), fr.find, fr.replace, -1)) + d2d1ExpectedPath := filepath.Join(d2ExpectedPath, strings.ReplaceAll(d2d1.Base(), fr.find, fr.replace)) assertPathExistsAfterRename(t, d2d1, d2d1ExpectedPath) // d2d1d1: what/when/where (directories with no files) - d2d1d1ExpectedPath := filepath.Join(d2d1ExpectedPath, strings.Replace(d2d1d1.Base(), fr.find, fr.replace, -1)) + d2d1d1ExpectedPath := filepath.Join(d2d1ExpectedPath, strings.ReplaceAll(d2d1d1.Base(), fr.find, fr.replace)) assertPathExistsAfterRename(t, d2d1d1, d2d1d1ExpectedPath) // d3: when/ @@ -178,7 +205,7 @@ func TestWalkDir(t *testing.T) { assertPathExistsAfterRename(t, d3, d3ExpectedPath) // d3f1: when/where (contains "why") - d3f1ExpectedPath := filepath.Join(d3ExpectedPath, strings.Replace(d3f1.Base(), fr.find, fr.replace, -1)) + d3f1ExpectedPath := filepath.Join(d3ExpectedPath, strings.ReplaceAll(d3f1.Base(), fr.find, fr.replace)) assertPathExistsAfterRename(t, d3f1, d3f1ExpectedPath) assertNewContentsOfFile(t, d3f1ExpectedPath, d3f1Contents, find, replace, "fy") @@ -199,12 +226,14 @@ func TestHandleFileWithDir(t *testing.T) { f := newTestDir(t, "", initial) defer os.Remove(f.Path) - expectedPath := filepath.Join(f.Dir(), strings.Replace(f.Base(), find, replace, -1)) + expectedPath := filepath.Join(f.Dir(), strings.ReplaceAll(f.Base(), find, replace)) defer os.Remove(expectedPath) fr := findReplace{find: find, replace: replace} assertFileExists(t, f) - fr.HandleFile(f) + if err := fr.HandleFile(f); err != nil { + t.Fatalf("HandleFile(%q): %v", f.Path, err) + } assertPathExistsAfterRename(t, f, expectedPath) } @@ -213,21 +242,22 @@ func TestHandleFileWithIgnoredDir(t *testing.T) { find := "git" replace := "got" - dirPath := filepath.Join(os.TempDir(), initial) + dirPath := filepath.Join(t.TempDir(), initial) if err := os.Mkdir(dirPath, 0700); err != nil { t.Fatalf("Mkdir(%q): %v", dirPath, err) } - f := NewFile(dirPath) - defer os.Remove(f.Path) + f := newFileOrFatal(t, dirPath) // Just in case it's unexpectedly renamed, let's make sure we cleanup the // anticipated name. - unexpectedName := strings.Replace(f.Base(), find, replace, -1) + unexpectedName := strings.ReplaceAll(f.Base(), find, replace) unexpectedPath := filepath.Join(f.Dir(), unexpectedName) defer os.Remove(unexpectedPath) fr := findReplace{find: find, replace: replace} assertFileExists(t, f) - fr.HandleFile(f) + if err := fr.HandleFile(f); err != nil { + t.Fatalf("HandleFile(%q): %v", f.Path, err) + } assertFileExists(t, f) } @@ -239,16 +269,18 @@ func TestHandleFileWithFile(t *testing.T) { f := newTestFile(t, "", initial, initial) defer os.Remove(f.Path) - expectedName := strings.Replace(f.Base(), find, replace, -1) + expectedName := strings.ReplaceAll(f.Base(), find, replace) expectedPath := filepath.Join(f.Dir(), expectedName) defer os.Remove(expectedPath) fr := findReplace{find: find, replace: replace} assertFileExists(t, f) - fr.HandleFile(f) + if err := fr.HandleFile(f); err != nil { + t.Fatalf("HandleFile(%q): %v", f.Path, err) + } assertPathExistsAfterRename(t, f, expectedPath) - got := NewFile(expectedPath).Read() + got := readOrFatal(t, newFileOrFatal(t, expectedPath)) if got != want { t.Errorf("replace %v with %v in %v, but got %v; want %v", find, replace, initial, got, want) } @@ -261,13 +293,15 @@ func TestRenameFile(t *testing.T) { f := newTestFile(t, "", initial, "") defer os.Remove(f.Path) - expectedName := strings.Replace(f.Base(), find, replace, -1) + expectedName := strings.ReplaceAll(f.Base(), find, replace) expectedPath := filepath.Join(f.Dir(), expectedName) defer os.Remove(expectedPath) fr := findReplace{find: find, replace: replace} assertFileExists(t, f) - fr.RenameFile(f) + if err := fr.RenameFile(f); err != nil { + t.Fatalf("RenameFile(%q): %v", f.Path, err) + } assertPathExistsAfterRename(t, f, expectedPath) } @@ -275,7 +309,7 @@ func TestRenameFile(t *testing.T) { // path exactly match the desired string. func assertNewContentsOfFile(t *testing.T, path string, initial string, find string, replace string, want string) { t.Helper() - got := NewFile(path).Read() + got := readOrFatal(t, newFileOrFatal(t, path)) if got != want { t.Errorf("replace %v with %v in %v, but got %v; want %v", find, replace, initial, got, want) } @@ -290,7 +324,9 @@ func TestReplaceContents(t *testing.T) { f := newTestFile(t, "", "*", initial) defer os.Remove(f.Path) fr := findReplace{find: find, replace: replace} - fr.ReplaceContents(f) + if err := fr.ReplaceContents(f); err != nil { + t.Fatalf("ReplaceContents(%q): %v", f.Path, err) + } assertNewContentsOfFile(t, f.Path, initial, find, replace, want) } @@ -303,7 +339,9 @@ func TestReplaceContentsEntireFile(t *testing.T) { f := newTestFile(t, "", "*", initial) defer os.Remove(f.Path) fr := findReplace{find: find, replace: replace} - fr.ReplaceContents(f) + if err := fr.ReplaceContents(f); err != nil { + t.Fatalf("ReplaceContents(%q): %v", f.Path, err) + } assertNewContentsOfFile(t, f.Path, initial, find, replace, want) } @@ -316,7 +354,9 @@ func TestReplaceContentsMultipleMatchesSingleLine(t *testing.T) { f := newTestFile(t, "", "*", initial) defer os.Remove(f.Path) fr := findReplace{find: find, replace: replace} - fr.ReplaceContents(f) + if err := fr.ReplaceContents(f); err != nil { + t.Fatalf("ReplaceContents(%q): %v", f.Path, err) + } assertNewContentsOfFile(t, f.Path, initial, find, replace, want) } @@ -329,7 +369,9 @@ func TestReplaceContentsMultipleMatchesMultipleLines(t *testing.T) { f := newTestFile(t, "", "*", initial) defer os.Remove(f.Path) fr := findReplace{find: find, replace: replace} - fr.ReplaceContents(f) + if err := fr.ReplaceContents(f); err != nil { + t.Fatalf("ReplaceContents(%q): %v", f.Path, err) + } assertNewContentsOfFile(t, f.Path, initial, find, replace, want) } @@ -342,10 +384,282 @@ func TestReplaceContentsNoMatches(t *testing.T) { f := newTestFile(t, "", "*", initial) defer os.Remove(f.Path) fr := findReplace{find: find, replace: replace} - fr.ReplaceContents(f) + if err := fr.ReplaceContents(f); err != nil { + t.Fatalf("ReplaceContents(%q): %v", f.Path, err) + } assertNewContentsOfFile(t, f.Path, initial, find, replace, want) } +// TestWalkDir_PermissionDeniedSubdirContinues ensures that an unreadable +// subdirectory does not abort the walk. The sibling subtree must still be +// rewritten, and the walker must record an error referencing the failing +// subdirectory. +func TestWalkDir_PermissionDeniedSubdirContinues(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("permission semantics differ on Windows") + } + if os.Geteuid() == 0 { + t.Skip("test requires non-root: chmod 0 directories are still readable as root") + } + + root := t.TempDir() + + // Build root/a/inside.txt (unreadable parent) and root/b/inside.txt + // (normal). After WalkDir we expect b's file to be rewritten and a's + // directory to surface an error. + denied := filepath.Join(root, "a") + if err := os.Mkdir(denied, 0700); err != nil { + t.Fatalf("Mkdir(%q): %v", denied, err) + } + deniedChild := filepath.Join(denied, "inside.txt") + if err := os.WriteFile(deniedChild, []byte("alpha"), 0600); err != nil { + t.Fatalf("WriteFile(%q): %v", deniedChild, err) + } + + siblingDir := filepath.Join(root, "b") + if err := os.Mkdir(siblingDir, 0700); err != nil { + t.Fatalf("Mkdir(%q): %v", siblingDir, err) + } + siblingFile := filepath.Join(siblingDir, "inside.txt") + if err := os.WriteFile(siblingFile, []byte("alpha"), 0600); err != nil { + t.Fatalf("WriteFile(%q): %v", siblingFile, err) + } + + // Remove read+exec from the denied directory only. Restore the bit at + // cleanup so t.TempDir's RemoveAll can succeed. + if err := os.Chmod(denied, 0); err != nil { + t.Fatalf("Chmod(%q, 0): %v", denied, err) + } + t.Cleanup(func() { _ = os.Chmod(denied, 0700) }) + + rootFile := newFileOrFatal(t, root) + fr := findReplace{find: "alpha", replace: "beta"} + fr.WalkDir(rootFile) + + // The sibling file should have been rewritten despite the denied subtree. + got, err := os.ReadFile(siblingFile) + if err != nil { + t.Fatalf("ReadFile(%q): %v", siblingFile, err) + } + if string(got) != "beta" { + t.Errorf("sibling file contents = %q; want %q (denied sibling aborted the walk)", string(got), "beta") + } + + // The walker must have recorded an error referencing the denied subtree. + err = fr.errs.err() + if err == nil { + t.Fatalf("WalkDir succeeded; want error mentioning %q", denied) + } + if !strings.Contains(err.Error(), denied) { + t.Errorf("WalkDir error = %v; want one mentioning %q", err, denied) + } + // errors.Is should walk the joined chain and find the permission error. + if !errors.Is(err, fs.ErrPermission) { + t.Errorf("WalkDir error = %v; want errors.Is(_, fs.ErrPermission) == true", err) + } +} + +// TestRenameFile_ReturnsErrorOnExistingDestination ensures a clobbering +// rename is refused (returning an error) rather than crashing the process. +func TestRenameFile_ReturnsErrorOnExistingDestination(t *testing.T) { + tmp := t.TempDir() + src := filepath.Join(tmp, "alpha") + if err := os.WriteFile(src, nil, 0600); err != nil { + t.Fatalf("WriteFile(%q): %v", src, err) + } + dst := filepath.Join(tmp, "beta") + if err := os.WriteFile(dst, nil, 0600); err != nil { + t.Fatalf("WriteFile(%q): %v", dst, err) + } + + f := newFileOrFatal(t, src) + fr := findReplace{find: "alpha", replace: "beta"} + err := fr.RenameFile(f) + if err == nil { + t.Fatalf("RenameFile(%q): err = nil; want an error referencing the occupied destination", src) + } + if !strings.Contains(err.Error(), "beta") { + t.Errorf("RenameFile error = %v; want one mentioning %q", err, "beta") + } + // The source must still be present — RenameFile must not have clobbered + // the destination either. + if _, statErr := os.Stat(src); statErr != nil { + t.Errorf("Stat(%q) after refused rename: %v", src, statErr) + } + if _, statErr := os.Stat(dst); statErr != nil { + t.Errorf("Stat(%q) after refused rename: %v", dst, statErr) + } +} + +// TestWalkDir_BadRenameTargetDoesNotAbortSiblings sets up two sibling files +// whose post-rename names would collide with already-existing files. The +// walker must rename what it can, record errors for what it cannot, and not +// abort the rest of the tree. +func TestWalkDir_BadRenameTargetDoesNotAbortSiblings(t *testing.T) { + root := t.TempDir() + + // Files that will be renamed alpha -> beta. The "occupied" path already + // has a beta target so its rename must fail. The "free" path has a + // distinct prefix and should succeed. + occupied := filepath.Join(root, "occupied-alpha") + if err := os.WriteFile(occupied, nil, 0600); err != nil { + t.Fatalf("WriteFile(%q): %v", occupied, err) + } + occupiedTarget := filepath.Join(root, "occupied-beta") + if err := os.WriteFile(occupiedTarget, nil, 0600); err != nil { + t.Fatalf("WriteFile(%q): %v", occupiedTarget, err) + } + + free := filepath.Join(root, "free-alpha") + if err := os.WriteFile(free, nil, 0600); err != nil { + t.Fatalf("WriteFile(%q): %v", free, err) + } + + rootFile := newFileOrFatal(t, root) + fr := findReplace{find: "alpha", replace: "beta"} + fr.WalkDir(rootFile) + + // The free file should have been renamed. + freeRenamed := filepath.Join(root, "free-beta") + if _, err := os.Stat(freeRenamed); err != nil { + t.Errorf("Stat(%q) after walk: %v (free-alpha should have been renamed despite occupied-alpha's failure)", freeRenamed, err) + } + + // The walker must have recorded an error referencing the occupied target. + err := fr.errs.err() + if err == nil { + t.Fatalf("WalkDir succeeded; want a 'refusing to rename' error for occupied-alpha") + } + if !strings.Contains(err.Error(), "occupied-beta") { + t.Errorf("WalkDir error = %v; want one mentioning %q", err, "occupied-beta") + } +} + +// TestWriteCleansUpTempFileOnRenameFailure ensures that File.Write does not +// leak a temp file when the rename step fails. It forces the rename to fail +// (after the temp file has been created) by making the destination a +// non-empty directory; os.Rename of a regular file onto a non-empty +// directory returns ENOTEMPTY ("file exists") on Linux regardless of the +// running user, so this exercises the deferred-cleanup path under both root +// and non-root. +func TestWriteCleansUpTempFileOnRenameFailure(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("rename-over-directory semantics differ on Windows") + } + + dir := t.TempDir() + target := filepath.Join(dir, "target") + + // Create the target as a non-empty directory. Write will succeed in + // creating its tempfile next to it, then fail the rename step. + if err := os.Mkdir(target, 0700); err != nil { + t.Fatalf("Mkdir(%q): %v", target, err) + } + if err := os.WriteFile(filepath.Join(target, "sentinel"), nil, 0600); err != nil { + t.Fatalf("WriteFile sentinel: %v", err) + } + + // Snapshot the directory contents before the Write so we can confirm no + // stray files survive afterwards. + beforeEntries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("ReadDir(%q): %v", dir, err) + } + before := make(map[string]struct{}, len(beforeEntries)) + for _, e := range beforeEntries { + before[e.Name()] = struct{}{} + } + + // We need f.Mode() to succeed, so prime the cached info. + f := newFileOrFatal(t, target) + if _, err := f.Info(); err != nil { + t.Fatalf("Info(%q): %v", target, err) + } + + if err := f.Write("beta"); err == nil { + t.Fatalf("Write succeeded over a non-empty directory; expected an error") + } + + // Confirm no new entries (other than the existing target directory) + // linger in the parent. + afterEntries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("ReadDir(%q): %v", dir, err) + } + for _, e := range afterEntries { + if _, ok := before[e.Name()]; ok { + continue + } + t.Errorf("leftover entry %q in %q after Write failure (tempfile was not cleaned up)", e.Name(), dir) + } +} + +// TestRun_ExitsZeroOnSuccess confirms run() returns 0 for a clean walk. +func TestRun_ExitsZeroOnSuccess(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "alpha.txt"), []byte("alpha"), 0600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + withWorkingDir(t, dir) + + var stderr bytes.Buffer + got := run([]string{"find-replace", "alpha", "beta"}, &stderr) + if got != 0 { + t.Errorf("run = %d; want 0 (stderr: %q)", got, stderr.String()) + } +} + +// TestRun_ExitsNonZeroOnTraversalError confirms run() returns a non-zero +// exit code when any file failed to be processed. We force a failure by +// putting a file whose rename target is occupied. +func TestRun_ExitsNonZeroOnTraversalError(t *testing.T) { + dir := t.TempDir() + src := filepath.Join(dir, "occupied-alpha") + if err := os.WriteFile(src, nil, 0600); err != nil { + t.Fatalf("WriteFile(%q): %v", src, err) + } + dst := filepath.Join(dir, "occupied-beta") + if err := os.WriteFile(dst, nil, 0600); err != nil { + t.Fatalf("WriteFile(%q): %v", dst, err) + } + + withWorkingDir(t, dir) + + var stderr bytes.Buffer + got := run([]string{"find-replace", "alpha", "beta"}, &stderr) + if got == 0 { + t.Errorf("run = 0; want non-zero (stderr: %q)", stderr.String()) + } +} + +// TestRun_BadArgCountPrintsUsage confirms the usage message goes to stderr +// and the exit code is non-zero. +func TestRun_BadArgCountPrintsUsage(t *testing.T) { + var stderr bytes.Buffer + got := run([]string{"find-replace"}, &stderr) + if got == 0 { + t.Errorf("run = 0; want non-zero") + } + if !strings.Contains(stderr.String(), "Usage: find-replace") { + t.Errorf("stderr = %q; want it to contain a usage line", stderr.String()) + } +} + +// withWorkingDir chdirs to dir for the duration of the test and restores the +// previous working directory at cleanup. +func withWorkingDir(t *testing.T, dir string) { + t.Helper() + prev, err := os.Getwd() + if err != nil { + t.Fatalf("Getwd: %v", err) + } + if err := os.Chdir(dir); err != nil { + t.Fatalf("Chdir(%q): %v", dir, err) + } + t.Cleanup(func() { _ = os.Chdir(prev) }) +} + func CloneRepoToTestDir(b *testing.B, repoUrl string) *File { b.Helper() d := newTestDir(b, "", "*") From 1f354bb62320252515bba816110aa323be455efe Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 18 May 2026 02:56:56 +0000 Subject: [PATCH 2/3] Bump go.mod to 1.20 to enable errors.Join The error-aggregation primitive introduced in this PR uses errors.Join, which was added in Go 1.20. CI was pinned to 1.19 via go.mod and 'go vet' failed with 'Join not declared by package errors'. This is the minimum bump needed for the chosen primitive. The broader 'be on a supported Go release' work (Go 1.19 has been EOL since August 2023) stays as #28. --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index 96a1ddb..66fd9af 100644 --- a/go.mod +++ b/go.mod @@ -1,5 +1,5 @@ module github.com/dolph/find-replace -go 1.19 +go 1.20 require golang.org/x/tools v0.7.0 From 609d3f7aa767cd6644dd992c28b0bcce0ba8fb57 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 18 May 2026 02:58:28 +0000 Subject: [PATCH 3/3] Remove deprecated rand.Seed call Since Go 1.20 (the new toolchain floor in this PR), the math/rand global generator is automatically seeded by the runtime; explicit rand.Seed is a deprecated no-op and staticcheck (run by golangci-lint) flags it as SA1019. The line was already vestigial -- left in place by the original commit to keep the diff narrow. The toolchain bump in the previous commit makes that compromise no longer viable. strings.go's RandomString still uses math/rand.Intn; the change to crypto/rand for filesystem paths remains tracked under issue #3. --- find_replace.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/find_replace.go b/find_replace.go index f228854..9bdbe79 100644 --- a/find_replace.go +++ b/find_replace.go @@ -5,12 +5,10 @@ import ( "fmt" "io" "log" - "math/rand" "os" "path/filepath" "strings" "sync" - "time" ) // findReplace is a struct used to provide context to all find & replace @@ -78,8 +76,6 @@ func main() { func run(args []string, stderr io.Writer) int { // Remove date/time from logging output. log.SetFlags(0) - // Vestigial seed for the math/rand-based temp file names; see issue #3. - rand.Seed(time.Now().UnixNano()) if len(args) != 3 { fmt.Fprintln(stderr, "Usage: find-replace FIND REPLACE")