diff options
| author | Paul Buetow <paul@buetow.org> | 2026-07-22 18:53:43 +0300 |
|---|---|---|
| committer | Paul Buetow <paul@buetow.org> | 2026-07-22 18:53:43 +0300 |
| commit | 456cb2d3be55d431ba507367764ccfd13b13b4b4 (patch) | |
| tree | 71c241dcf9fd963661023779e8edefc30ad7a4be /internal/showcase | |
| parent | 23ecaa2c7f731a4f7188aec2404594641faf2f7f (diff) | |
fix(errcheck): handle all unchecked error returns without changing behaviormain
errcheck ./... flagged unchecked HTTP response Close(), file Close(),
os.RemoveAll(), fmt.Scanln(), and fmt.Fprintf() calls across codeberg,
github, release, showcase, and sync. None of these were bugs causing
incorrect behavior today, but leaving them unchecked hid real failure
modes (e.g. a lagging NFS mount failing a file Close() after writes,
which this codebase has hit before per commit 23ecaa2).
- internal/codeberg/codeberg.go, internal/github/github.go,
internal/release/release.go: added a small closeResponseBody(resp)
helper per package and used it for all deferred resp.Body.Close()
calls. The body is always fully read (or abandoned on an earlier
error) by the time Close() runs, so the error is intentionally
discarded - matching the explicit `_ = ...` discard convention
already used elsewhere in this repo (e.g. showcase.go's
os.RemoveAll on the worktree-add failure path).
- internal/release/release.go: the two fmt.Scanln(&response) prompts
now explicitly discard the return values; a Scanln error already
leaves response == "", which the existing y/yes check already
treats as a safe decline, so behavior is unchanged.
- internal/showcase/code_extractor.go, images.go, language_detector.go:
added a shared closeFile(*os.File) helper (package showcase) for the
read-only file Close() calls, matching the same discard rationale.
copyFile's destination Close() is the one write-side case where a
close failure is real data-loss information, so it now uses a named
return to surface it via err without masking any earlier error.
- internal/showcase/showcase.go: the deferred os.RemoveAll(tempRoot)
now explicitly discards its error, matching the sibling `_ =` call
three lines above in the same function.
- internal/sync/branch_analyzer.go: GenerateDeleteScript's per-repository
fmt.Fprintf(file, ...) calls are now checked and wrapped with
fmt.Errorf(...: %w), matching the error-wrapping convention already
used by writeBranchDeletionBlock right below it in the same file
(the repeated repo-header writes were pulled into a new
writeDeleteScriptRepoHeader helper to keep this readable). The
script file's defer file.Close() now uses a named return so a close
failure is reported instead of silently discarded.
Added focused tests for the two behavior-relevant paths: copyFile's
missing-source/success paths after the named-return change, and
PromptConfirmation's empty-input-declines/explicit-yes behavior after
touching the Scanln call. Trivial defer-Close discards elsewhere are
not additionally unit tested per the task's guidance against
overengineering.
Verified: go build ./..., go vet ./..., errcheck ./... (clean), and
go test ./... all pass.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Diffstat (limited to 'internal/showcase')
| -rw-r--r-- | internal/showcase/code_extractor.go | 14 | ||||
| -rw-r--r-- | internal/showcase/images.go | 17 | ||||
| -rw-r--r-- | internal/showcase/images_test.go | 55 | ||||
| -rw-r--r-- | internal/showcase/language_detector.go | 4 | ||||
| -rw-r--r-- | internal/showcase/showcase.go | 6 |
5 files changed, 87 insertions, 9 deletions
diff --git a/internal/showcase/code_extractor.go b/internal/showcase/code_extractor.go index 92ee1e4..7c40573 100644 --- a/internal/showcase/code_extractor.go +++ b/internal/showcase/code_extractor.go @@ -9,6 +9,16 @@ import ( "strings" ) +// closeFile closes a file opened for reading. These reads are best-effort +// (shebang sniffing, line counting, snippet extraction) and the file is never +// written to, so a close failure here cannot affect correctness - the error +// is intentionally discarded rather than treated as actionable. Shared by +// the other showcase package files that open read-only files (images.go, +// language_detector.go). +func closeFile(file *os.File) { + _ = file.Close() +} + // extractCodeSnippet extracts a random code snippet from the repository func extractCodeSnippet(repoPath string, languages []LanguageStats) (string, string, error) { if len(languages) == 0 { @@ -113,7 +123,7 @@ func extractCodeSnippet(repoPath string, languages []LanguageStats) (string, str matched = true } } - file.Close() + closeFile(file) } } @@ -185,7 +195,7 @@ func extractSnippetFromFile(filePath string, minLines, maxLines int) (string, er if err != nil { return "", err } - defer file.Close() + defer closeFile(file) // Read all lines var lines []string diff --git a/internal/showcase/images.go b/internal/showcase/images.go index b6fe6d7..2c09a8e 100644 --- a/internal/showcase/images.go +++ b/internal/showcase/images.go @@ -226,19 +226,28 @@ func isGitHostedImage(url string) bool { strings.Contains(url, "codeberg.page") } -// copyFile copies a file from src to dst -func copyFile(src, dst string) error { +// copyFile copies a file from src to dst. The destination is a write, so +// unlike the read-only closeFile helper used elsewhere in this package, a +// failure to close it is reported via the named return: Sync() already +// covers the common flush-failure case, but a close error on a lagging +// network filesystem is still real data-loss information worth surfacing, +// and it must not mask an earlier, more specific error. +func copyFile(src, dst string) (err error) { sourceFile, err := os.Open(src) if err != nil { return err } - defer sourceFile.Close() + defer closeFile(sourceFile) destFile, err := os.Create(dst) if err != nil { return err } - defer destFile.Close() + defer func() { + if cerr := destFile.Close(); cerr != nil && err == nil { + err = fmt.Errorf("failed to close destination file %s: %w", dst, cerr) + } + }() _, err = io.Copy(destFile, sourceFile) if err != nil { diff --git a/internal/showcase/images_test.go b/internal/showcase/images_test.go new file mode 100644 index 0000000..c88da00 --- /dev/null +++ b/internal/showcase/images_test.go @@ -0,0 +1,55 @@ +package showcase + +import ( + "os" + "path/filepath" + "testing" +) + +// TestCopyFile_CopiesContent verifies the basic success path still works +// after copyFile was changed to use a named return so that a destination +// Close() error can be surfaced without changing the happy-path result. +func TestCopyFile_CopiesContent(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + src := filepath.Join(dir, "source.txt") + dst := filepath.Join(dir, "dest.txt") + want := "hello showcase\n" + + if err := os.WriteFile(src, []byte(want), 0644); err != nil { + t.Fatalf("failed to write source file: %v", err) + } + + if err := copyFile(src, dst); err != nil { + t.Fatalf("copyFile() returned error: %v", err) + } + + got, err := os.ReadFile(dst) + if err != nil { + t.Fatalf("failed to read destination file: %v", err) + } + if string(got) != want { + t.Fatalf("copyFile() wrote %q, want %q", string(got), want) + } +} + +// TestCopyFile_MissingSourceReturnsError verifies that a missing source file +// still produces an error (and never a nil error masked by the deferred +// destination-close handling) and does not leave a destination file behind. +func TestCopyFile_MissingSourceReturnsError(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + src := filepath.Join(dir, "does-not-exist.txt") + dst := filepath.Join(dir, "dest.txt") + + err := copyFile(src, dst) + if err == nil { + t.Fatal("copyFile() with missing source returned nil error, want non-nil") + } + + if _, statErr := os.Stat(dst); statErr == nil { + t.Fatal("copyFile() with missing source created a destination file, want none") + } +} diff --git a/internal/showcase/language_detector.go b/internal/showcase/language_detector.go index 692f048..9a42be1 100644 --- a/internal/showcase/language_detector.go +++ b/internal/showcase/language_detector.go @@ -201,7 +201,7 @@ func detectLanguages(repoPath string) (languages []LanguageStats, documentation } } } - file.Close() + closeFile(file) } } @@ -281,7 +281,7 @@ func countFileLines(path string) (int, error) { if err != nil { return 0, err } - defer file.Close() + defer closeFile(file) scanner := bufio.NewScanner(file) lines := 0 diff --git a/internal/showcase/showcase.go b/internal/showcase/showcase.go index ae938cb..9582c1f 100644 --- a/internal/showcase/showcase.go +++ b/internal/showcase/showcase.go @@ -350,7 +350,11 @@ func (g *Generator) prepareStatsRepoPath(repoName, repoPath string) (string, fun } cleanup := func() error { - defer os.RemoveAll(tempRoot) + // Best-effort cleanup of the temporary worktree root, matching the + // discard above for the same call on the worktree-add failure path: + // this is scratch space, and by the time we get here the actual + // worktree removal below is the operation whose error matters. + defer func() { _ = os.RemoveAll(tempRoot) }() if _, err := runCommandWithCustomTimeout(45*time.Second, "git", "-C", repoPath, "worktree", "remove", "--force", worktreePath); err != nil { return fmt.Errorf("failed to remove temporary worktree for %s: %w", repoName, err) |
