Three defects in progress.go, all small, all in the same file.
1. Non-TTY prints nothing until the first item completes.newProgress returns immediately without printing when stderr is not a TTY (progress.go:54-58); the first plain line only appears from increment() (progress.go:102-106). README §Progress requires that each phase's display be "rendered the moment the phase starts — a scan must never look hung." Under cron, tee or CI, stderr is a pipe. If the walk's first directory read blocks for minutes on a busy pool, the log is completely silent — precisely the failure the requirement exists to prevent. The hash phase can emit nothing at all if its first read is slow.
2. stderrIsTTY treats any character device as a terminal.progress.go:26-33 tests fi.Mode()&os.ModeCharDevice != 0. /dev/null and /dev/zero are character devices, so sfdupes scan ... 2>/dev/null takes the full ANSI redraw path and runs the library's 100ms spinner goroutine, writing escape sequences into the void.
3. Warnings race the spinner's background renderer.warnf (progress.go:109-119) calls p.bar.Clear() and then writes to os.Stderr from the main goroutine. For the indeterminate (-1) bars, progressbar.NewOptions64 starts an internal goroutine that renders to os.Stderr every 100ms with no synchronization against warnf. The walk phase is both a spinner bar and the phase that emits per-path warnings (scan.go:234) — and those warnings are what tell the operator which records are about to be deleted, so garbling them matters. The progress doc comment at progress.go:41-42 already claims single-goroutine ownership of stderr; today that is not true.
Definition of done
newProgress emits one zero-state plain line immediately on the non-TTY path, before any work begins.
stderrIsTTY uses a real terminal test. github.com/mattn/go-isatty and golang.org/x/term are both already in the module graph as indirect dependencies of progressbar; promote whichever is used to a direct dependency and update README §Constraints, which currently lists the permitted third-party dependencies.
The spinner's background render goroutine is disabled (OptionSetSpinnerChangeInterval(0), so the spinner advances only on Add) or warnings are routed through the bar's own writer — either way stderr has one writer, matching the doc comment.
Tests: a non-TTY writer receives a line before the first increment(); a pipe, a regular file and /dev/null all report not-a-TTY; interleaved warnings and increments each land on their own line.
make check green.
Three defects in `progress.go`, all small, all in the same file.
**1. Non-TTY prints nothing until the first item completes.** `newProgress` returns immediately without printing when stderr is not a TTY (`progress.go:54-58`); the first plain line only appears from `increment()` (`progress.go:102-106`). README §Progress requires that each phase's display be "rendered the moment the phase starts — a scan must never look hung." Under cron, `tee` or CI, stderr is a pipe. If the walk's first directory read blocks for minutes on a busy pool, the log is completely silent — precisely the failure the requirement exists to prevent. The hash phase can emit nothing at all if its first read is slow.
**2. `stderrIsTTY` treats any character device as a terminal.** `progress.go:26-33` tests `fi.Mode()&os.ModeCharDevice != 0`. `/dev/null` and `/dev/zero` are character devices, so `sfdupes scan ... 2>/dev/null` takes the full ANSI redraw path and runs the library's 100ms spinner goroutine, writing escape sequences into the void.
**3. Warnings race the spinner's background renderer.** `warnf` (`progress.go:109-119`) calls `p.bar.Clear()` and then writes to `os.Stderr` from the main goroutine. For the indeterminate (`-1`) bars, `progressbar.NewOptions64` starts an internal goroutine that renders to `os.Stderr` every 100ms with no synchronization against `warnf`. The walk phase is both a spinner bar and the phase that emits per-path warnings (`scan.go:234`) — and those warnings are what tell the operator which records are about to be deleted, so garbling them matters. The `progress` doc comment at `progress.go:41-42` already claims single-goroutine ownership of stderr; today that is not true.
## Definition of done
1. `newProgress` emits one zero-state plain line immediately on the non-TTY path, before any work begins.
2. `stderrIsTTY` uses a real terminal test. `github.com/mattn/go-isatty` and `golang.org/x/term` are both already in the module graph as indirect dependencies of `progressbar`; promote whichever is used to a direct dependency and update README §Constraints, which currently lists the permitted third-party dependencies.
3. The spinner's background render goroutine is disabled (`OptionSetSpinnerChangeInterval(0)`, so the spinner advances only on `Add`) or warnings are routed through the bar's own writer — either way stderr has one writer, matching the doc comment.
4. Tests: a non-TTY writer receives a line before the first `increment()`; a pipe, a regular file and `/dev/null` all report not-a-TTY; interleaved warnings and increments each land on their own line.
5. `make check` green.
clawbot
added this to the 1.0.0 milestone 2026-08-09 03:45:37 +02:00
Item 1: the non-TTY path prints its zero-state line inside newProgress, before any work.
Item 2: use golang.org/x/term's IsTerminal (stdlib-adjacent; already in the module graph), promote it to a direct dependency, and list it in README.md §Constraints.
Item 3: take the first option, the spinner advances only on Add (no background render goroutine), so stderr has one writer and the doc comment becomes true. If the library cannot do that for an indeterminate bar, say so in the PR and route warnings through the bar's writer instead.
Tests per the definition of done. For the TTY test use files the test creates (a pipe, a regular file) plus /dev/null; do not depend on the test having a terminal.
warnf is also being edited for #7 (escaping); keep your change to it small. #5 (interrupts) follows this unit and builds on it.
Model: opus-5-5
Plan (implementer's brief).
- Item 1: the non-TTY path prints its zero-state line inside `newProgress`, before any work.
- Item 2: use `golang.org/x/term`'s `IsTerminal` (stdlib-adjacent; already in the module graph), promote it to a direct dependency, and list it in `README.md` §Constraints.
- Item 3: take the first option, the spinner advances only on `Add` (no background render goroutine), so stderr has one writer and the doc comment becomes true. If the library cannot do that for an indeterminate bar, say so in the PR and route warnings through the bar's writer instead.
- Tests per the definition of done. For the TTY test use files the test creates (a pipe, a regular file) plus `/dev/null`; do not depend on the test having a terminal.
- `warnf` is also being edited for https://git.eeqj.de/sneak/sfdupes/issues/7 (escaping); keep your change to it small. https://git.eeqj.de/sneak/sfdupes/issues/5 (interrupts) follows this unit and builds on it.
Model: opus-5-5
clawbot
self-assigned this 2026-10-03 14:09:46 +02:00
Implemented in #73 as the plan describes: the zero-state line is printed inside newProgress, golang.org/x/term is a direct dependency, and the spinner advances only on Add (the library supports that for a bar without a total, so warnings were not rerouted).
Model: opus-5-5
Implemented in https://git.eeqj.de/sneak/sfdupes/pulls/73 as the plan describes: the zero-state line is printed inside `newProgress`, `golang.org/x/term` is a direct dependency, and the spinner advances only on `Add` (the library supports that for a bar without a total, so warnings were not rerouted).
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Three defects in
progress.go, all small, all in the same file.1. Non-TTY prints nothing until the first item completes.
newProgressreturns immediately without printing when stderr is not a TTY (progress.go:54-58); the first plain line only appears fromincrement()(progress.go:102-106). README §Progress requires that each phase's display be "rendered the moment the phase starts — a scan must never look hung." Under cron,teeor CI, stderr is a pipe. If the walk's first directory read blocks for minutes on a busy pool, the log is completely silent — precisely the failure the requirement exists to prevent. The hash phase can emit nothing at all if its first read is slow.2.
stderrIsTTYtreats any character device as a terminal.progress.go:26-33testsfi.Mode()&os.ModeCharDevice != 0./dev/nulland/dev/zeroare character devices, sosfdupes scan ... 2>/dev/nulltakes the full ANSI redraw path and runs the library's 100ms spinner goroutine, writing escape sequences into the void.3. Warnings race the spinner's background renderer.
warnf(progress.go:109-119) callsp.bar.Clear()and then writes toos.Stderrfrom the main goroutine. For the indeterminate (-1) bars,progressbar.NewOptions64starts an internal goroutine that renders toos.Stderrevery 100ms with no synchronization againstwarnf. The walk phase is both a spinner bar and the phase that emits per-path warnings (scan.go:234) — and those warnings are what tell the operator which records are about to be deleted, so garbling them matters. Theprogressdoc comment atprogress.go:41-42already claims single-goroutine ownership of stderr; today that is not true.Definition of done
newProgressemits one zero-state plain line immediately on the non-TTY path, before any work begins.stderrIsTTYuses a real terminal test.github.com/mattn/go-isattyandgolang.org/x/termare both already in the module graph as indirect dependencies ofprogressbar; promote whichever is used to a direct dependency and update README §Constraints, which currently lists the permitted third-party dependencies.OptionSetSpinnerChangeInterval(0), so the spinner advances only onAdd) or warnings are routed through the bar's own writer — either way stderr has one writer, matching the doc comment.increment(); a pipe, a regular file and/dev/nullall report not-a-TTY; interleaved warnings and increments each land on their own line.make checkgreen.Plan (implementer's brief).
newProgress, before any work.golang.org/x/term'sIsTerminal(stdlib-adjacent; already in the module graph), promote it to a direct dependency, and list it inREADME.md§Constraints.Add(no background render goroutine), so stderr has one writer and the doc comment becomes true. If the library cannot do that for an indeterminate bar, say so in the PR and route warnings through the bar's writer instead./dev/null; do not depend on the test having a terminal.warnfis also being edited for #7 (escaping); keep your change to it small. #5 (interrupts) follows this unit and builds on it.Model: opus-5-5
Implemented in #73 as the plan describes: the zero-state line is printed inside
newProgress,golang.org/x/termis a direct dependency, and the spinner advances only onAdd(the library supports that for a bar without a total, so warnings were not rerouted).Model: opus-5-5