From b05a03a1f4b3c1445a31a68ee44ad77014bfeadc Mon Sep 17 00:00:00 2001 From: John Bolliger Date: Sat, 22 Aug 2026 23:19:28 -0700 Subject: [PATCH 1/2] Drive deletes through the live path and cap git concurrency MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three pieces of work that ended up sharing the same test file. Test the delete path users actually run. `performDeletions` was test-only by its own comment, so the async completion logic — branchDeletedMsg → deletesDone → the stateForcePrompt/stateResult transition — was never fed through Update. All 18 delete call sites now drive the real path, the force prompt is answered with a keystroke rather than by calling forceDeleteUnmerged directly, and `performDeletions` is gone. Five new tests cover what only the async path can get wrong; each was verified to fail against a deliberately broken Update before being kept. Measure risk concurrently. Every `git cherry` is its own subprocess and refreshMergeInfo ran one per gone branch in sequence — on a repo where 100 branches are gone that is 1.14s of startup, again on every fetch and every prune. The cost is process spawn, not git work, so the calls now run 8 at a time: startup 1.14s → 0.56s, reload 1.12s → 0.33s, the confirm screen 1.09s → 0.25s. Bound how much git runs at once. tea.Batch gives every selected branch its own goroutine, so arming remotes on 100 branches meant 100 simultaneous `push --delete` — connections a remote would throttle or refuse, and a failed push there leaves a branch deleted locally whose only other copy is still out on the remote. runGit is the single door every invocation passes through, so the cap lives there: 8 local processes, 3 network ones, with networkBound() picking which applies. The cap is measured on the subprocesses rather than on itself, so removing it fails the test instead of silently muting the counter. Also recorded in docs/improvements.md: rows wrap on terminals narrower than 68 + the name column, so an 80-column terminal wraps every row once branch names reach 13 cells; and two dead ends, so they are not re-investigated. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0157b1zyhs2FVFBmaNWS6wjS --- docs/improvements.md | 103 +++++++++-- main.go | 143 +++++++++++---- main_test.go | 417 ++++++++++++++++++++++++++++++++++++++----- 3 files changed, 570 insertions(+), 93 deletions(-) diff --git a/docs/improvements.md b/docs/improvements.md index 214b2c4..212d279 100644 --- a/docs/improvements.md +++ b/docs/improvements.md @@ -78,6 +78,71 @@ to fail against the old code (`date column at cell 31, want 33`) before being ke silently vanished — no error, the safety signal simply was not there. `remotes()` now tries every configured remote with `origin` ordered first. +### Tier 2 + +**7. The tested delete path was not the one users run.** `performDeletions` was test-only by its +own comment, so the live async path's completion logic — `branchDeletedMsg` → `deletesDone` → the +`stateForcePrompt` / `stateResult` transition — was never fed through `Update`. The riskiest state +machine in the program was the untested one. + +All 18 delete call sites now drive the real path through a `drainDeletions` helper, which runs the +`tea.Batch` cmds concurrently (as the runtime does) and feeds every `branchDeletedMsg` back through +`Update`. The force prompt is answered with a keystroke rather than by calling +`forceDeleteUnmerged` directly, and the four tests that hand-set `state = stateForcePrompt` now +assert the machine put them there. `performDeletions` has been deleted. + +Five tests cover what only the async path can get wrong; each was verified to fail against a +deliberately broken `Update` before being kept: + +| test | mutation it catches | +| ---- | ------------------- | +| `TestDeletingWaitsForEveryResult` | completing the run on the first result (`> 0` instead of `>= len(m.results)`) — and reloading the branch list mid-run, which would renumber the indices outstanding messages still write to | +| `TestStrayDeleteResultIsIgnored` | dropping the `msg.idx` bounds check — panics with `index out of range [7]` | +| `TestSpinnerTickStopsAfterDeleting` | dropping the `state == stateDeleting` guard, leaving the tick re-arming forever | +| `TestForcePromptDeclineKeepsBranch` | `n` at the prompt discarding the branch it exists to spare | +| `TestDeletingIgnoresKeysExceptCtrlC` | a stray keystroke dismissing a run whose results have not landed | + +**14. Startup and refresh spent over a second measuring risk, one branch at a time.** Every +`git cherry` is its own subprocess, and `refreshMergeInfo` ran one per gone branch in sequence — +on the tool's headline case, a repo full of gone branches, that is the whole list on the clock +before the first paint, again after every fetch, and again after every prune. + +Measured on a 101-branch repo where 100 branches are gone: + +| path | before | after | +| ---- | ------ | ----- | +| `initialModel` (startup) | 1.14s | 0.56s | +| `refreshMergeInfo` (fetch / reload) | 1.12s | 0.33s | +| `measureSelectedRisk` (pressing `d`)| 1.09s | 0.25s | + +The cost is process spawn, not git work: 100 × `git rev-parse HEAD` takes 0.63s against 100 × +`git cherry` at 0.74s, so ~6ms per subprocess is the floor and no cheaper git command would have +helped. `measureRisk` now takes a predicate and runs the calls concurrently, one goroutine per +branch writing its own slice element. A cap sweep on the same repo put the knee at 4–8 workers +(1 → 1056ms, 4 → 364ms, 8 → 330ms, 32 → 331ms), so `maxLocalGit` is 8; past that the limit is +elsewhere. + +**15. A wide prune opened one connection per branch.** `tea.Batch` gives every selected branch its +own goroutine (`bubbletea/tea.go:545`), so arming remotes on 100 branches meant 100 simultaneous +`git push --delete` — 100 connections a remote would throttle or refuse. A failed push there is +the worst shape the tool has: the local branch is already gone, and the copy that was meant to +outlive it is still sitting on the remote. + +Local and network work are now capped separately — `maxLocalGit = 8`, `maxRemotePush = 3` — so a +wide local fan-out never widens the network one. The cap sits inside `runGit`, the single door +every git invocation already passes through, and `networkBound(args)` picks which one applies. Put +at the call sites instead it held only for the callers that remembered to ask: `fetchPruneCmd` and +`forceDeleteUnmerged` both escaped it. + +The cap is measured on the subprocesses themselves, not on the limiter: `runGit` feeds two +`gauge`s, one for all git processes and one for the network-bound ones. Instrumenting the limiter +would have stopped reporting along with it — verified by deleting the limiter, which took the +observed peak from 3 to 17 and failed `TestWideDeleteStaysWithinCaps`. The test asserts literal +bounds rather than the constants, so raising a cap breaks it instead of moving with it. + +The earlier "60 parallel deletes succeeded" note below still stands, but it only ever covered +local ref locking. It says nothing about the network. + ### Also completed - `LICENSE` (MIT) @@ -90,26 +155,31 @@ configured remote with `origin` ordered first. ### Tier 2 — robustness -**5. Blocking git calls inside `Update`.** `loadDiff` (`v`), `refreshMergeInfo`, and -`reloadBranches` run synchronously in the update loop. `git branch -r --merged` is -O(remote refs × history) and runs on *every* reload; on a repo with thousands of remote branches -the UI freezes. The `tea.Cmd` pattern already works for fetch — reuse it. Note that -`refreshMergeInfo` now also issues one `git cherry` per gone branch, which raises the stakes. +**5. Blocking git calls inside `Update`.** `loadDiff` (`v`), `refreshMergeInfo` and +`reloadBranches` still run synchronously in the update loop, so what remains of their cost is +still a freeze. The per-branch measurement that dominated them has been parallelised (see item 14 +under Completed); the residue is ~370ms on a 100-gone-branch repo. Moving them onto the `tea.Cmd` +pattern already used for fetch would take that to zero. + +Do not reach for `git branch -r --merged` first: it was measured at 100ms with 2000 remote +refs, roughly a tenth of what the `git cherry` loop beside it cost. **6. `runGit` has no timeout and does not disable terminal prompts.** `fetch --all --prune` and `push --delete` are network-bound; a credential or SSH prompt hangs the TUI with no recovery. Set `GIT_TERMINAL_PROMPT=0` and attach a `context.WithTimeout` so it fails fast instead. -**7. The tested delete path is not the one users run.** `performDeletions` is test-only by its own -comment; the live async path's completion logic — `branchDeletedMsg` → `deletesDone` → the -`stateForcePrompt` / `stateResult` transition — is never fed through `Update` in any test. The -riskiest state machine in the program is the untested one. Port the tests to the async path and -delete `performDeletions`. - **8. Smaller items.** +- Rows are wider than the terminal whenever `subjectWidth` hits its `max(10, …)` floor, and each + wrapped row eats two screen lines while `visibleRows` still counts it as one — so the list + overruns and the footer scrolls away. Measured threshold: rows wrap below `68 + nameW` columns, + which at a classic 80-column terminal means any branch name of 13 cells or more wraps *every* + row. `renderRow` needs to fit `m.width` rather than assume it. - `listView` runs one line over terminal height when `status` and `err` are both set - (`visibleRows` is `height-5`; actual emission is `height+1`). + (`visibleRows` is `height-5`; actual emission is `height+1`) — confirmed at 25 lines for a + height of 24. - ANSI and control characters in commit subjects and branch names render raw into the terminal. + Confirmed: a `\x1b[31m` in a subject reaches the row intact (a bare `BEL` is stripped by + `ansi.Truncate`). The text comes from fetched branches, so it is not the author's to trust. - `applyBranches` silently discards the user's existing selections on `p`. - The cursor starts on an arbitrary row. `sortBranches` preserves the cursor by name unconditionally, but at startup `cursor` is 0 and `branches` is still in `for-each-ref` @@ -144,6 +214,15 @@ naturally with finding 1. ## Investigated and rejected +**A commit subject spanning several lines breaking the `for-each-ref` parse.** `loadBranches` +splits output on newlines and needs 8 NUL-separated fields per branch, so an embedded newline +would silently drop branches from the list. It cannot happen: `%(contents:subject)` folds the +subject's newlines to spaces. Verified against a commit whose subject wraps before the blank line. + +**A duplicate or stray `branchDeletedMsg` ending a run early.** `deletesDone` counts `done` flags +on the results, not messages received, so a repeat cannot over-count; an out-of-range index is +dropped by the bounds check. Both are covered by `TestStrayDeleteResultIsIgnored`. + **Concurrent `git branch -d` racing on `packed-refs.lock`.** `tea.Batch` runs deletions concurrently, which looked like it should collide on the packed-refs lock. Tested with 60 parallel deletes against a freshly packed repo: **all 60 succeeded.** Git's ref-lock retry handles it. No diff --git a/main.go b/main.go index d7e490e..bd71f5d 100644 --- a/main.go +++ b/main.go @@ -9,6 +9,8 @@ import ( "sort" "strconv" "strings" + "sync" + "sync/atomic" "time" tea "github.com/charmbracelet/bubbletea" @@ -174,7 +176,72 @@ var ( // ---- git I/O ---- +// Concurrency caps. Local git work is subprocess-bound — roughly 6ms of spawn +// cost each — so running it a few at a time is what makes a repo full of gone +// branches load quickly. Remote work is not: every push or fetch opens its own +// connection, and a prune of a hundred armed branches would open a hundred at +// once, which remotes throttle or refuse. The two are capped separately so a +// wide local fan-out never widens the network one. +const ( + maxLocalGit = 8 + maxRemotePush = 3 +) + +var ( + localSlots = make(chan struct{}, maxLocalGit) + remoteSlots = make(chan struct{}, maxRemotePush) +) + +// networkBound reports whether a git invocation opens a connection to a remote. +// It is what picks the cap, so a new network subcommand belongs here rather than +// at its call site. +func networkBound(args []string) bool { + return len(args) > 0 && (args[0] == "push" || args[0] == "fetch") +} + +// gauge records the highest number of concurrent holders it has seen. It sits on +// the subprocesses rather than on the slots, so a test can tell a cap that works +// from one that was removed — instrumenting the cap would stop reporting along +// with it. +type gauge struct{ inFlight, peak atomic.Int64 } + +func (g *gauge) enter() { + n := g.inFlight.Add(1) + for { + peak := g.peak.Load() + if n <= peak || g.peak.CompareAndSwap(peak, n) { + return + } + } +} + +func (g *gauge) leave() { g.inFlight.Add(-1) } + +// gitProcs counts every git subprocess; netProcs counts the ones that talk to a +// remote, which is what a remote host actually feels. +var gitProcs, netProcs gauge + +// runGit is the single door every git invocation passes through, which is what +// makes it the place to bound them: a cap at the call sites would only hold for +// the callers that remembered to ask. func runGit(args ...string) (string, error) { + net := networkBound(args) + slots := localSlots + if net { + slots = remoteSlots + } + slots <- struct{}{} + defer func() { <-slots }() + + // Counted after the slot is held, so the gauges measure what is running + // rather than what is queued. + gitProcs.enter() + defer gitProcs.leave() + if net { + netProcs.enter() + defer netProcs.leave() + } + cmd := exec.Command("git", args...) // Pin the locale: deleteBranch classifies failures by matching git's own // error text, which gettext would otherwise translate. Everything else we @@ -857,8 +924,8 @@ func (b branch) deleteFlag(force bool) string { } // deleteBranch runs one branch's local delete and, when wantRemote is set, its -// remote-branch push --delete. It is the single worker shared by the synchronous -// performDeletions path and the asynchronous deleteBranchCmd path. +// remote-branch push --delete. It is the worker deleteBranchCmd runs off the +// update loop, one cmd per branch. func deleteBranch(b branch, flag string, wantRemote bool) deleteResult { res := deleteResult{br: b, done: true} if _, err := runGit("branch", flag, b.name); err != nil { @@ -898,19 +965,6 @@ func pushRemoteDelete(res *deleteResult) { } } -// performDeletions deletes the selected branches synchronously. The interactive -// UI uses the async startDeletions path instead; this remains for tests and as -// the straightforward equivalent. -func (m *model) performDeletions() { - m.measureSelectedRisk() - m.results = nil - for _, b := range m.selectedBranches() { - wantRemote := b.deleteRemote && b.upstream != "" - m.results = append(m.results, deleteBranch(b, b.deleteFlag(m.force), wantRemote)) - } - m.reloadBranches() -} - // carryMarks copies the user's pending selections from old onto a freshly loaded // branch set, matching by name. Used on the fetch path, which reloads every // branch struct but changes nothing the marks were made about. An armed remote @@ -955,9 +1009,17 @@ func (m *model) reloadBranches() { // upstream is merged into it or whose tip is merged into HEAD, and measures what // deleting it would cost. Call after every branch (re)load. func (m *model) refreshMergeInfo() { + // localMergedSet needs nothing from the remote resolution, so let the two + // runs of subprocesses overlap rather than queue. + var headMerged map[string]bool + local := make(chan struct{}) + go func() { + headMerged = localMergedSet() + close(local) + }() defRef := remoteDefault() merged := remoteMergedSet(defRef) - headMerged := localMergedSet() + <-local m.riskBaseRef = defRef if m.riskBaseRef == "" { @@ -972,35 +1034,42 @@ func (m *model) refreshMergeInfo() { b.remoteMerged = b.upstream != "" && merged[b.upstream] b.headMerged = headMerged[b.name] b.riskMeasured = false // the branch was just reloaded; any old count is stale - // Only gone branches are measured up front, because `p` consults the count - // to decide what it may auto-select. The rest wait for measureSelectedRisk: - // riskCommitCount is a subprocess per branch, and running it for every - // unmergeable branch here cost a second of startup on a repo with dozens. - if b.gone { - m.measureRisk(b) - } } -} - -// measureRisk fills in b's cost-of-deletion count, once per (re)load. -func (m *model) measureRisk(b *branch) { - if b.riskMeasured { - return + // Only gone branches are measured up front, because `p` consults the count to + // decide what it may auto-select. The rest wait for measureSelectedRisk: + // riskCommitCount is a subprocess per branch, and measuring every unmergeable + // branch here would put a network-free repo's whole branch list on the clock. + m.measureRisk(func(b branch) bool { return b.gone }) +} + +// measureRisk fills in the cost-of-deletion count for every not-yet-measured +// branch that want accepts. The counts are independent `git cherry` subprocesses +// whose cost is dominated by process spawn, so they run concurrently: measured +// one at a time, a repo with a hundred gone branches spent 1.1s here on every +// load, fetch and prune. +func (m *model) measureRisk(want func(branch) bool) { + base := m.riskBaseRef // read once: the goroutines must not touch the model + var wg sync.WaitGroup + for i := range m.branches { + if m.branches[i].riskMeasured || !want(m.branches[i]) { + continue + } + wg.Add(1) + go func() { + defer wg.Done() + // Each goroutine owns one slice element, so no two write the same branch. + m.branches[i].riskCommits = riskCommitCount(m.branches[i].name, base) + m.branches[i].riskMeasured = true + }() } - b.riskCommits = riskCommitCount(b.name, m.riskBaseRef) - b.riskMeasured = true + wg.Wait() } // measureSelectedRisk measures what deleting each selected branch would cost. // Call before any view that reports the cost: only branches a safe delete would // refuse are measured, since those are the ones deleted with -D. func (m *model) measureSelectedRisk() { - for i := range m.branches { - b := &m.branches[i] - if b.selected && (b.gone || !b.safeDeletable()) { - m.measureRisk(b) - } - } + m.measureRisk(func(b branch) bool { return b.selected && (b.gone || !b.safeDeletable()) }) } // forceDeleteUnmerged re-runs the deletions that a safe (-d) delete refused, diff --git a/main_test.go b/main_test.go index f72dace..de071d9 100644 --- a/main_test.go +++ b/main_test.go @@ -6,7 +6,9 @@ import ( "os/exec" "regexp" "strings" + "sync" "testing" + "time" "unicode/utf8" tea "github.com/charmbracelet/bubbletea" @@ -16,17 +18,80 @@ import ( // key builds a rune KeyMsg (e.g. "y", "R") for driving update handlers in tests. func key(s string) tea.KeyMsg { return tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune(s)} } -// runCmd executes a tea.Cmd to completion, recursing into batched cmds, so tests -// can drive the async deletion path synchronously. -func runCmd(t *testing.T, cmd tea.Cmd) { +// deleteMsgs runs every cmd in a deletion batch concurrently — as the tea +// runtime does — and returns the branchDeletedMsgs they produce. The batch is +// one cmd per branch plus the spinner tick, which is dropped: it only animates, +// and waiting on its 120ms timer would slow every caller. +func deleteMsgs(t *testing.T, cmd tea.Cmd) []branchDeletedMsg { t.Helper() if cmd == nil { - return + t.Fatal("want a deletion batch cmd, got nil") } - if batch, ok := cmd().(tea.BatchMsg); ok { - for _, c := range batch { - runCmd(t, c) + msg := cmd() + batch, ok := msg.(tea.BatchMsg) + if !ok { + t.Fatalf("want a tea.BatchMsg, got %T", msg) + } + msgs := make(chan tea.Msg, len(batch)) + for _, c := range batch { + go func() { msgs <- c() }() + } + want := len(batch) - 1 + timeout := time.NewTimer(30 * time.Second) + defer timeout.Stop() + var out []branchDeletedMsg + for len(out) < want { + select { + case msg := <-msgs: + if dm, ok := msg.(branchDeletedMsg); ok { + out = append(out, dm) + } + case <-timeout.C: + t.Fatalf("timed out after %d of %d deletions reported", len(out), want) + } + } + return out +} + +// drainDeletions runs a deletion batch and feeds each result back through +// Update, so tests exercise the path users actually run: the completion logic +// (reload, then force-prompt vs result) decides the end state rather than the +// test asserting it into place. +func drainDeletions(t *testing.T, m model, cmd tea.Cmd) model { + t.Helper() + for _, dm := range deleteMsgs(t, cmd) { + nm, _ := m.Update(dm) + m = nm.(model) + } + return m +} + +// startAndDrain deletes m's selected branches and drives the run to completion. +// Starting the batch inside the call keeps m's copy and startDeletions' writes +// to it from being operands of one expression, where Go does not define which +// happens first. +func startAndDrain(t *testing.T, m model, includeRemote bool) model { + t.Helper() + return drainDeletions(t, m, m.startDeletions(includeRemote)) +} + +// selectAll marks every branch but the current one, arming the remote delete too +// when arm is set. +func selectAll(m *model, arm bool) { + for i := range m.branches { + if m.branches[i].isCurrent { + continue } + m.branches[i].selected = true + m.branches[i].deleteRemote = arm + } +} + +// wantState asserts the state the machine landed in, with why it had to. +func wantState(t *testing.T, m model, want viewState, why string) { + t.Helper() + if m.state != want { + t.Fatalf("%s: state is %v, want %v", why, m.state, want) } } @@ -95,6 +160,23 @@ func setupRepo(t *testing.T) string { return tmp } +// setupManyTracked builds a repo whose n feature branches each track origin and +// hold a commit of their own, so a prune of the lot fans out widely. +func setupManyTracked(t *testing.T, n int) string { + t.Helper() + tmp := initRepo(t, "main") + commitFile(t, tmp, "a", "a") + addOrigin(t, tmp, "main") + for i := 0; i < n; i++ { + name := fmt.Sprintf("feature/%d", i) + git(t, tmp, "checkout", "-q", "-b", name, "main") + commitFile(t, tmp, fmt.Sprintf("f%d", i), name) + } + git(t, tmp, "checkout", "-q", "main") + git(t, tmp, "push", "-q", "-u", "origin", "--all") // one connection, not n + return tmp +} + func chdir(t *testing.T, dir string) { t.Helper() old, _ := os.Getwd() @@ -204,7 +286,7 @@ func TestPruneGoneBranch(t *testing.T) { } find(m.branches, "feature/tracked").selected = true m.force = false - m.performDeletions() + m = startAndDrain(t, m, true) if len(m.results) != 1 || !m.results[0].localOK { t.Fatalf("gone branch should force-delete: %+v", m.results) @@ -239,7 +321,7 @@ func TestSafeDeleteRefusesUnmerged(t *testing.T) { find(m.branches, "feature/merged").selected = true find(m.branches, "feature/unmerged").selected = true m.force = false // safe -d - m.performDeletions() + m = startAndDrain(t, m, true) var merged, unmerged *deleteResult for i := range m.results { @@ -275,10 +357,11 @@ func TestForceDeleteUnmergedRetry(t *testing.T) { } find(m.branches, "feature/unmerged").selected = true m.force = false // safe -d, which will be refused - m.performDeletions() + m = startAndDrain(t, m, true) // The refused unmerged branch should be surfaced for a force prompt, with // its ahead count copied onto the result (0 here: it has no upstream). + wantState(t, m, stateForcePrompt, "a refused -d must raise the force prompt") failures := m.forceableFailures() if len(failures) != 1 || failures[0].br.name != "feature/unmerged" { t.Fatalf("want feature/unmerged in forceableFailures, got %+v", failures) @@ -290,8 +373,10 @@ func TestForceDeleteUnmergedRetry(t *testing.T) { t.Fatal("feature/unmerged should still exist before force retry") } - // Answering yes retries with -D and clears the branch. - m.forceDeleteUnmerged() + // Answering yes at the prompt retries with -D and clears the branch. + nm, _ := m.Update(key("y")) + m = nm.(model) + wantState(t, m, stateResult, "answering the prompt must land on the results screen") if len(m.forceableFailures()) != 0 { t.Fatalf("no failures should remain after force retry: %+v", m.results) } @@ -317,7 +402,7 @@ func TestNoForcePromptForCleanDelete(t *testing.T) { } find(m.branches, "feature/merged").selected = true m.force = false - m.performDeletions() + m = startAndDrain(t, m, true) if len(m.results) != 1 || !m.results[0].localOK { t.Fatalf("merged branch should delete cleanly: %+v", m.results) @@ -349,7 +434,7 @@ func TestGoneFailureNotForceable(t *testing.T) { } tb.selected = true m.force = false // gone branches still use -D - m.performDeletions() + m = startAndDrain(t, m, true) if len(m.results) != 1 || !m.results[0].localOK { t.Fatalf("gone branch should force-delete: %+v", m.results) @@ -371,8 +456,9 @@ func TestForceDeleteUnmergedNoop(t *testing.T) { } find(m.branches, "feature/merged").selected = true m.force = false - m.performDeletions() + m = startAndDrain(t, m, true) before := len(m.branches) + wantState(t, m, stateResult, "a clean delete must skip the force prompt") m.forceDeleteUnmerged() // no forceable failures — should change nothing if len(m.forceableFailures()) != 0 { @@ -398,7 +484,7 @@ func TestForceDeleteAndRemote(t *testing.T) { tb.selected = true tb.deleteRemote = true m.force = true // -D, also needed since tracked has its own commit - m.performDeletions() + m = startAndDrain(t, m, true) if len(m.results) != 1 { t.Fatalf("want 1 result, got %d", len(m.results)) @@ -447,6 +533,254 @@ func TestDeleteBranchCmd(t *testing.T) { } } +// runGit is where the cap lives, so callers cannot escape it however many pile +// in at once — and it still runs them in parallel rather than one at a time. +func TestGitCallsAreCapped(t *testing.T) { + chdir(t, setupLocalRepo(t)) + + const callers = 40 + gitProcs.peak.Store(0) + var wg sync.WaitGroup + for i := 0; i < callers; i++ { + wg.Add(1) + go func() { + defer wg.Done() + runGit("rev-parse", "--git-dir") + }() + } + wg.Wait() + + peak := gitProcs.peak.Load() + if peak > maxLocalGit { + t.Fatalf("%d callers ran %d git processes at once, cap is %d", callers, peak, maxLocalGit) + } + if peak < 2 { + t.Fatalf("the cap serialized everything: peak %d", peak) + } + if n := gitProcs.inFlight.Load(); n != 0 { + t.Fatalf("every slot should be released, %d still held", n) + } +} + +// Pruning a wide selection must not open a connection per branch: remotes +// throttle or refuse a burst, and a failed push is a branch deleted locally +// whose only other copy is still out there. +func TestWideDeleteStaysWithinCaps(t *testing.T) { + const n = 20 + repo := setupManyTracked(t, n) + chdir(t, repo) + + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + selectAll(&m, true) + m.force = true + + gitProcs.peak.Store(0) + netProcs.peak.Store(0) + m = startAndDrain(t, m, true) + + // Literal bounds, not the constants under test: raising a cap must break this + // test rather than move the goalposts with it. + const fewConnections, fewProcesses = 4, 12 + if maxRemotePush > fewConnections { + t.Fatalf("maxRemotePush is %d; a prune should never ask a remote for more than %d connections", maxRemotePush, fewConnections) + } + if got := netProcs.peak.Load(); got > fewConnections { + t.Fatalf("%d branches opened %d simultaneous connections to the remote", n, got) + } + if got := gitProcs.peak.Load(); got > fewProcesses { + t.Fatalf("%d branches ran %d git processes at once", n, got) + } + if netProcs.peak.Load() == 0 { + t.Fatal("no pushes were observed; the test is not measuring the remote path") + } + // The cap must not cost any of them their deletion. + for _, r := range m.results { + if !r.localOK || !r.remoteOK { + t.Fatalf("throttled delete did not complete: %+v", r) + } + } + if len(m.branches) != 1 { + t.Fatalf("only the current branch should remain, got %v", branchNames(m.branches)) + } +} + +// Risk is measured concurrently, one goroutine per branch writing its own slice +// element. Each branch must end up with its own count, not a neighbour's. +func TestConcurrentRiskMeasurementKeepsCountsWithBranch(t *testing.T) { + repo := initRepo(t, "main") + chdir(t, repo) + commitFile(t, repo, "a", "a") + + // feature/i carries i commits of its own. Branching off one chain costs n + // commits for the n distinct counts; a fresh branch each time costs n(n+1)/2. + const n = 12 + git(t, repo, "checkout", "-q", "-b", "chain", "main") + for i := 1; i <= n; i++ { + commitFile(t, repo, fmt.Sprintf("f%d", i), "x") + git(t, repo, "branch", fmt.Sprintf("feature/%d", i)) + } + git(t, repo, "checkout", "-q", "main") + git(t, repo, "branch", "-D", "chain") + + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + selectAll(&m, false) + m.measureSelectedRisk() + + for i := 1; i <= n; i++ { + name := fmt.Sprintf("feature/%d", i) + b := find(m.branches, name) + if b == nil || !b.riskMeasured { + t.Fatalf("%s was not measured: %+v", name, b) + } + if b.riskCommits != i { + t.Fatalf("%s should hold %d commits, measured %d", name, i, b.riskCommits) + } + } +} + +// The live path only leaves stateDeleting once every branch has reported, and +// the branch list is not reloaded before then — a mid-run reload would renumber +// the results the outstanding messages are still indexing into. +func TestDeletingWaitsForEveryResult(t *testing.T) { + repo := setupRepo(t) + chdir(t, repo) + + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + find(m.branches, "feature/merged").selected = true // -d succeeds + find(m.branches, "feature/unmerged").selected = true // -d is refused + m.force = false + + msgs := deleteMsgs(t, m.startDeletions(false)) + if len(msgs) != 2 { + t.Fatalf("want 2 results, got %d", len(msgs)) + } + for i, dm := range msgs { + nm, _ := m.Update(dm) + m = nm.(model) + if !m.results[dm.idx].done { + t.Fatalf("result %d should be marked done: %+v", dm.idx, m.results[dm.idx]) + } + if i == len(msgs)-1 { + break + } + wantState(t, m, stateDeleting, "state must hold at stateDeleting until the last result") + if find(m.branches, "feature/merged") == nil { + t.Fatal("the branch list must not be reloaded mid-run") + } + } + wantState(t, m, stateForcePrompt, "the refused -d should raise the force prompt") + if find(m.branches, "feature/merged") != nil { + t.Fatal("the final result should have reloaded the branch list") + } +} + +// A result carrying an index outside the current run must be dropped rather +// than panic: the async cmds outlive nothing here, but the bounds check is the +// only thing standing between a stale message and an out-of-range write. +func TestStrayDeleteResultIsIgnored(t *testing.T) { + repo := setupRepo(t) + chdir(t, repo) + + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + find(m.branches, "feature/merged").selected = true + m.startDeletions(false) + + nm, _ := m.Update(branchDeletedMsg{idx: 7, res: deleteResult{done: true}}) + m = nm.(model) + wantState(t, m, stateDeleting, "a stray result must not complete the run") + if m.deletesDone() != 0 { + t.Fatalf("a stray result must not be counted, got %d", m.deletesDone()) + } +} + +// Declining the force prompt leaves the refused branch — and its commits — in +// place. This is the escape hatch the -d/-D split exists for. +func TestForcePromptDeclineKeepsBranch(t *testing.T) { + repo := setupRepo(t) + chdir(t, repo) + + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + find(m.branches, "feature/unmerged").selected = true + m.force = false + m = startAndDrain(t, m, true) + wantState(t, m, stateForcePrompt, "a refused -d must raise the force prompt") + + nm, _ := m.Update(key("n")) + m = nm.(model) + wantState(t, m, stateResult, "declining must land on the results screen") + if find(m.branches, "feature/unmerged") == nil { + t.Fatal("declining the force prompt must keep the branch") + } + if len(m.forceableFailures()) != 1 { + t.Fatalf("the refusal must still be reported: %+v", m.results) + } +} + +// While deletions are in flight the keyboard is inert except for ctrl+c, so a +// stray keystroke cannot dismiss a run whose results have not landed yet. +func TestDeletingIgnoresKeysExceptCtrlC(t *testing.T) { + repo := setupRepo(t) + chdir(t, repo) + + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + m.state = stateDeleting + + for _, k := range []tea.KeyMsg{key("q"), key("y"), key("d")} { + nm, cmd := m.Update(k) + if got := nm.(model).state; got != stateDeleting { + t.Fatalf("%v must not change state, got %v", k, got) + } + if cmd != nil { + t.Fatalf("%v must not issue a cmd", k) + } + } + + _, cmd := m.Update(tea.KeyMsg{Type: tea.KeyCtrlC}) + if cmd == nil { + t.Fatal("ctrl+c must abort") + } + if _, ok := cmd().(tea.QuitMsg); !ok { + t.Fatalf("ctrl+c should quit, got %T", cmd()) + } +} + +// The spinner re-arms itself only while deletions are running; once the run has +// landed the tick must die out rather than loop forever. +func TestSpinnerTickStopsAfterDeleting(t *testing.T) { + m := model{state: stateDeleting} + + nm, cmd := m.Update(spinnerTickMsg{}) + if nm.(model).spinnerFrame != 1 { + t.Fatalf("frame should advance, got %d", nm.(model).spinnerFrame) + } + if cmd == nil { + t.Fatal("the tick must re-arm while deleting") + } + + m.state = stateResult + if _, cmd := m.Update(spinnerTickMsg{}); cmd != nil { + t.Fatal("the tick must not re-arm once the run has landed") + } +} + // #4: on the confirm screen, 'y' deletes locals only while 'R' also deletes the // armed remote. func TestConfirmRemoteConfirmationSplit(t *testing.T) { @@ -465,13 +799,9 @@ func TestConfirmRemoteConfirmationSplit(t *testing.T) { m.state = stateConfirm nm, cmd := m.updateConfirm(key("y")) - if nm.(model).state != stateDeleting { - t.Fatalf("state should be stateDeleting, got %v", nm.(model).state) - } - if cmd == nil { - t.Fatal("expected a deletion batch cmd") - } - runCmd(t, cmd) + wantState(t, nm.(model), stateDeleting, "state should be stateDeleting") + m = drainDeletions(t, nm.(model), cmd) + wantState(t, m, stateResult, "a clean -D run should land on the results screen") if !remoteHasBranch(t, repo, "feature/tracked") { t.Fatal("'y' must not delete the remote branch") } @@ -492,10 +822,9 @@ func TestConfirmRemoteConfirmationSplit(t *testing.T) { m.state = stateConfirm nm, cmd := m.updateConfirm(key("R")) - if nm.(model).state != stateDeleting { - t.Fatalf("state should be stateDeleting, got %v", nm.(model).state) - } - runCmd(t, cmd) + wantState(t, nm.(model), stateDeleting, "state should be stateDeleting") + m = drainDeletions(t, nm.(model), cmd) + wantState(t, m, stateResult, "a clean -D run should land on the results screen") if remoteHasBranch(t, repo, "feature/tracked") { t.Fatal("'R' should delete the remote branch") } @@ -905,9 +1234,7 @@ func TestUnmergedBranchWithoutUpstreamIsWarned(t *testing.T) { b.selected = true nm, _ := m.updateList(key("d")) m = nm.(model) - if m.state != stateConfirm { - t.Fatalf("'d' should open the confirm screen, got %v", m.state) - } + wantState(t, m, stateConfirm, "'d' should open the confirm screen") b = find(m.branches, "feature/unmerged") if b.riskCommits != 1 { t.Fatalf("want the branch's 1 unique commit measured, got %d", b.riskCommits) @@ -967,7 +1294,7 @@ func TestForcePromptStatesCommitCount(t *testing.T) { } find(m.branches, "feature/unmerged").selected = true m.force = false - m.performDeletions() + m = startAndDrain(t, m, true) failures := m.forceableFailures() if len(failures) != 1 { @@ -977,7 +1304,7 @@ func TestForcePromptStatesCommitCount(t *testing.T) { t.Fatalf("the refused result must carry its measured cost, got %+v", failures[0]) } - m.state = stateForcePrompt + wantState(t, m, stateForcePrompt, "a refused -d must raise the force prompt") out := stripANSI(m.forcePromptView()) if !strings.Contains(out, fmt.Sprintf("1 commit(s) not in %s will be lost", m.riskBase)) { t.Fatalf("force prompt must state the commit count:\n%s", out) @@ -1007,7 +1334,7 @@ func TestRemoteDeleteDeferredUntilLocalSucceeds(t *testing.T) { tb.selected = true tb.deleteRemote = true m.force = false // safe delete, which git will refuse - m.performDeletions() + m = startAndDrain(t, m, true) r := m.results[0] if r.localOK { @@ -1027,7 +1354,9 @@ func TestRemoteDeleteDeferredUntilLocalSucceeds(t *testing.T) { } // The force retry clears the branch, so the arming is finally honoured. - m.forceDeleteUnmerged() + wantState(t, m, stateForcePrompt, "a refused -d must raise the force prompt") + nm, _ := m.Update(key("y")) + m = nm.(model) r = m.results[0] if !r.localOK || !r.remoteTried || !r.remoteOK || r.remoteSkipped { t.Fatalf("force retry should complete both deletes: %+v", r) @@ -1122,7 +1451,7 @@ func TestTagShadowingBranchName(t *testing.T) { // And the delete must land on the branch, leaving the tag alone. m.force = true - m.performDeletions() + m = startAndDrain(t, m, true) if !m.results[0].localOK { t.Fatalf("delete failed: %s", m.results[0].localErr) } @@ -1183,7 +1512,7 @@ func TestTagShadowingBranchName(t *testing.T) { tb.selected = true tb.deleteRemote = true m.force = true - m.performDeletions() + m = startAndDrain(t, m, true) r := m.results[0] if !r.localOK { @@ -1343,7 +1672,7 @@ func TestGoneCurrentBranchIsNotPruned(t *testing.T) { // Selected by hand, git refuses — a failure -D cannot rescue, so it must not // raise the force prompt offering a retry that fails identically. b.selected = true - m.performDeletions() + m = startAndDrain(t, m, true) if len(m.results) != 1 || m.results[0].localOK { t.Fatalf("deleting the checked-out branch must fail: %+v", m.results) } @@ -1418,7 +1747,7 @@ func TestLocalOnlyRepo(t *testing.T) { // above (it survives the fetch), so clear it first. find(m.branches, "feature/unmerged").selected = false find(m.branches, "feature/merged").selected = true - m.performDeletions() + m = startAndDrain(t, m, true) if len(m.results) != 1 || !m.results[0].localOK { t.Fatalf("merged branch should delete cleanly: %+v", m.results) } @@ -1508,7 +1837,7 @@ func TestDetachedHead(t *testing.T) { nm, _ = m.updateList(key("n")) m = nm.(model) find(m.branches, "main").selected = true - m.performDeletions() + m = startAndDrain(t, m, true) if len(m.results) != 1 || !m.results[0].localOK { t.Fatalf("main should delete cleanly while detached: %+v", m.results) } @@ -1555,11 +1884,11 @@ func TestNonStandardDefaultBranch(t *testing.T) { // The same holds on the force prompt reached after a refused safe delete. m.force = false - m.performDeletions() + m = startAndDrain(t, m, true) if len(m.forceableFailures()) != 1 { t.Fatalf("the unmerged branch should be refused and forceable: %+v", m.results) } - m.state = stateForcePrompt + wantState(t, m, stateForcePrompt, "a refused -d must raise the force prompt") if out := stripANSI(m.forcePromptView()); !strings.Contains(out, "no base branch to compare against") { t.Fatalf("force prompt must carry the warning:\n%s", out) } @@ -1623,7 +1952,7 @@ func TestUnreachableRemote(t *testing.T) { tb = find(m.branches, "feature/tracked") tb.selected = true tb.deleteRemote = true - m.performDeletions() + m = startAndDrain(t, m, true) r := m.results[0] if !r.localOK { @@ -1723,7 +2052,7 @@ func TestRemoteDeleteRace(t *testing.T) { } tb.selected = true tb.deleteRemote = true - m.performDeletions() + m = startAndDrain(t, m, true) r := m.results[0] if !r.localOK { @@ -1845,7 +2174,7 @@ func TestUnicodeNameAndEmptySubject(t *testing.T) { // And it deletes end to end, the name surviving the round trip to git. b.selected = true m.force = true - m.performDeletions() + m = startAndDrain(t, m, true) if !m.results[0].localOK { t.Fatalf("delete failed: %s", m.results[0].localErr) } From 43db7f92d38ec425c75be84fa205c96fe3a9bc28 Mon Sep 17 00:00:00 2001 From: John Bolliger Date: Sat, 22 Aug 2026 23:50:43 -0700 Subject: [PATCH 2/2] Answer risk from one query and load the repo in two rounds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three performance findings, all of them measured before and after. Stop spawning a `git cherry` per merged branch. A branch whose tip is already an ancestor of the base has an empty base..branch range, so `git cherry` can only report nothing — and on this tool's headline case, a repo full of branches that were merged and then pruned, that is nearly every one of them. refreshMergeInfo now runs one `branch --merged ` into m.baseMerged and measureRisk reads the set. On a 101-branch repo where 100 are gone, startup went 529ms → 45ms and peak concurrent git processes 8 → 2. TestMergedBranchesCostNoSubprocess asserts a literal process bound rather than a time; it fails at 39 processes when the shortcut is removed. Window the screens that ask a question. confirmView wrote every selected branch in full: 40 selected produced 206 rows into a 24-row terminal with "Delete these branches?" on row 205, so the user answered a prompt they could not read. The confirm, force-prompt, deleting and results screens now go through page(header, body, footer), which renders a window and a position line, with scrollKeys adding ↑/↓, space, ctrl+d/u, pgup/pgdn and g/G plus the mouse wheel. Each screen answers its own keys first, so y, R and n are never swallowed by the scroll handler. Take startup from six serial subprocesses to two rounds. On this repo startup was 34ms and all of it was process-start latency — one git costs ~6.5ms to spawn and the chain ran rev-parse → for-each-ref → remote → symbolic-ref → branch -r --merged → branch --merged in sequence. loadRemoteRefs reads refs/remotes once with %(refname)%00%(symref), replacing `git remote`, a symbolic-ref per remote and a rev-parse per candidate; %(symref) yields the full ref, so the tag-shadowing guarantee survives where symbolic-ref --short would break it. localDefaultBranch takes a has(name) predicate, so refreshMergeInfo answers from the branch list it already holds. loadRepo then starts loadBranches, localMergedSet and loadRemoteRefs together, initialModel runs the repo check beside them rather than ahead of them, and the two --merged queries form round two. Startup 34ms → 19ms here, and 529ms → 17ms on the 100-gone fixture. Remote names now come out of refs/remotes rather than `git remote`. A remote with no fetched refs holds no ref a default branch could resolve to, so nothing a caller could use is lost. Recorded as items 16-18 in docs/improvements.md, with the numbers and the 501-branch case where for-each-ref's own commit-object reads, not chain depth, are the floor. Item 5's stale 370ms residue is corrected to 16ms, and a note about applyBranches dropping selections is removed — carryMarks fixed that. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01WnKYkFZWMaTCoYumjAkTGD --- README.md | 4 + docs/improvements.md | 62 ++++- docs/test-coverage-gaps.md | 3 +- main.go | 478 +++++++++++++++++++++++++++---------- main_test.go | 207 +++++++++++++++- 5 files changed, 613 insertions(+), 141 deletions(-) diff --git a/README.md b/README.md index 0e11c92..c8aafc2 100644 --- a/README.md +++ b/README.md @@ -69,6 +69,10 @@ checklist), so the UI stays responsive while remote pushes complete. In the diff view: `↑`/`↓` scroll, `space`/`ctrl+d` page down, `ctrl+u`/`pgup` page up, `g`/`G` jump to top/bottom, and `q`/`esc`/`v` return to the list. +The confirmation, force-delete, progress and results screens scroll with the same keys, so a +wide selection never pushes the prompt off the bottom of the terminal. The answer keys take +precedence, so `y`, `R` and `n` still work while a list is scrolled. + ## Row format ![The branch list: selection, track and merge columns, dates, hashes and subjects](assets/branches.png) diff --git a/docs/improvements.md b/docs/improvements.md index 212d279..e189684 100644 --- a/docs/improvements.md +++ b/docs/improvements.md @@ -143,6 +143,59 @@ bounds rather than the constants, so raising a cap breaks it instead of moving w The earlier "60 parallel deletes succeeded" note below still stands, but it only ever covered local ref locking. It says nothing about the network. +**16. A `git cherry` subprocess per branch, for an answer one query already held.** `measureRisk` +ran `git cherry ` for every gone branch. A branch whose tip is already an ancestor +of the base has an empty `base..branch` range, so `git cherry` can only report nothing — and on +the tool's headline case, a repo full of branches that were merged and then pruned, that is nearly +every one of them. + +`refreshMergeInfo` now runs one `git branch --merged ` and caches the result in +`m.baseMerged`; `measureRisk` reads the set instead of starting a process. Measured on the same +101-branch fixture (100 gone, all merged): startup **529ms → 45ms**, peak concurrent git +processes 8 → 2. One query at 9ms replaced 30 `git cherry` calls at 330ms on a 30-branch probe. + +`TestMergedBranchesCostNoSubprocess` asserts a literal process bound rather than a time, and fails +at 39 processes when the shortcut is removed. + +**17. The confirm screen rendered its whole body, pushing its own question off the terminal.** +`confirmView` wrote every selected branch in full. With 40 selected that is 206 rows into a 24-row +terminal, with `Delete these branches?` on row 205 — the user answered a prompt they could not +read. `forcePromptView`, `resultView` and `deletingView` had the same shape. + +All four now go through `page(header, body, footer)`, which renders the header, a window into the +body, and the footer, plus a position line when the body does not fit. `scrollKeys` adds ↑/↓, +space, ctrl+d/u, pgup/pgdn and g/G, and the mouse wheel scrolls them; each screen answers its own +keys first, so `y`, `R` and `n` are never swallowed. Rendering only the visible rows also takes +the per-frame cost off the length of the list. + +**18. Startup was six git subprocesses deep, and the depth was the whole cost.** On this repo +(3 branches) startup was 34ms, all of it process-start latency — one `git` costs ~6.5ms to spawn, +and the chain ran `rev-parse` → `for-each-ref` → `remote` → `symbolic-ref` → `branch -r --merged` +→ `branch --merged ` in sequence. Only `branch --merged HEAD` overlapped anything. + +Three changes, together taking the chain to two rounds: + +- `loadRemoteRefs` reads `refs/remotes` once with `--format='%(refname)%00%(symref)'`. That single + call replaces `git remote`, a `symbolic-ref` per remote and a `rev-parse --verify` per candidate. + `%(symref)` yields the full ref, so it keeps the tag-shadowing guarantee `symbolic-ref --short` + would break. Remote names now come out of the refs: a remote with no fetched refs holds nothing + a default branch could resolve to, so nothing is lost. +- `localDefaultBranch` takes a `has(name)` predicate. `refreshMergeInfo` passes a lookup into the + branch list it already holds; only `baseBranch`, which has no list, still pays `gitHasBranch`. +- `loadRepo` starts `loadBranches`, `localMergedSet` and `loadRemoteRefs` in one round, and + `initialModel` runs the repo check beside them rather than ahead of them. `refreshMergeInfo` + then runs `branch -r --merged` and `branch --merged ` as a second round. + +| repo | before | after | +| ---- | ------ | ----- | +| this one (3 branches) | 34ms | 19ms | +| 101 branches, 100 gone (with item 16)| 529ms | 17ms | +| 501 branches | 90ms | 77ms | + +The 501-branch case is `for-each-ref`'s own work (42ms of the 77ms), not chain depth — measured at +15ms for `%(refname)` alone against 42ms for the full format, so the commit-object reads behind +`%(committerdate)` are the floor there. + ### Also completed - `LICENSE` (MIT) @@ -157,9 +210,11 @@ local ref locking. It says nothing about the network. **5. Blocking git calls inside `Update`.** `loadDiff` (`v`), `refreshMergeInfo` and `reloadBranches` still run synchronously in the update loop, so what remains of their cost is -still a freeze. The per-branch measurement that dominated them has been parallelised (see item 14 -under Completed); the residue is ~370ms on a 100-gone-branch repo. Moving them onto the `tea.Cmd` -pattern already used for fetch would take that to zero. +still a freeze. Items 14, 16 and 18 under Completed took the residue to ~16ms on a +100-gone-branch repo, so this is no longer a visible freeze — what it would still buy is the +first paint, which currently waits on the whole two-round load. Moving `refreshMergeInfo` onto +the `tea.Cmd` pattern already used for fetch would put the branch list on screen after one +subprocess (~7ms) and fill the ✓ and risk columns in behind it. Do not reach for `git branch -r --merged` first: it was measured at 100ms with 2000 remote refs, roughly a tenth of what the `git cherry` loop beside it cost. @@ -180,7 +235,6 @@ Set `GIT_TERMINAL_PROMPT=0` and attach a `context.WithTimeout` so it fails fast - ANSI and control characters in commit subjects and branch names render raw into the terminal. Confirmed: a `\x1b[31m` in a subject reaches the row intact (a bare `BEL` is stripped by `ansi.Truncate`). The text comes from fetched branches, so it is not the author's to trust. -- `applyBranches` silently discards the user's existing selections on `p`. - The cursor starts on an arbitrary row. `sortBranches` preserves the cursor by name unconditionally, but at startup `cursor` is 0 and `branches` is still in `for-each-ref` (alphabetical) order, so it pins the cursor to wherever the alphabetically-first branch diff --git a/docs/test-coverage-gaps.md b/docs/test-coverage-gaps.md index 9ecba46..d50d499 100644 --- a/docs/test-coverage-gaps.md +++ b/docs/test-coverage-gaps.md @@ -208,7 +208,8 @@ All three tiers are complete (2026-08-17): **14 tests added, 43 total**, passing - Tier 3 pins five everyday operations and corrected one more prediction. - A `/simplify` pass then found **two further live instances of the tag-shadowing bug** that Tier 1 had missed (`localDefaultBranch` and `push --delete`), removed the `qualifyRef` - probe, and brought startup back from 11 git subprocesses to 7 — the pre-change baseline. + probe, and brought startup back from 11 git subprocesses to 7 — the pre-change baseline. It is +6 now, in two concurrent rounds rather than six serial ones (see item 18 in `improvements.md`). Fixtures compose rather than duplicate: `initRepo` (bare init + identity) → `setupLocalRepo` (branch shapes) → `setupRepo` (+ `addOrigin` + a tracking branch), with diff --git a/main.go b/main.go index bd71f5d..dd92706 100644 --- a/main.go +++ b/main.go @@ -136,6 +136,10 @@ type model struct { remoteDefault string // resolved remote default branch, e.g. "origin/main" riskBase string // ref that branch.riskCommits is measured against ("" if unresolved) riskBaseRef string // riskBase fully qualified, so a same-named tag cannot shadow it + // baseMerged holds the local branches whose tip is an ancestor of riskBaseRef. + // Their riskCommits is 0 by definition, so one query here removes a `git + // cherry` subprocess per branch (see measureRisk). + baseMerged map[string]bool spinnerFrame int // animation frame for the deleting spinner (deletion counts derive from results) @@ -144,6 +148,8 @@ type model struct { diffLines []string // raw lines of the diff being viewed diffTop int // scroll offset within diffLines + bodyTop int // scroll offset within the confirm/force/result body (see page) + width, height int err string status string // transient info message (e.g. fetch results) @@ -203,9 +209,12 @@ func networkBound(args []string) bool { // the subprocesses rather than on the slots, so a test can tell a cap that works // from one that was removed — instrumenting the cap would stop reporting along // with it. -type gauge struct{ inFlight, peak atomic.Int64 } +// It also counts every holder, which is what tells a fan-out that was removed +// from one that merely runs fast on a small fixture. +type gauge struct{ inFlight, peak, total atomic.Int64 } func (g *gauge) enter() { + g.total.Add(1) n := g.inFlight.Add(1) for { peak := g.peak.Load() @@ -334,59 +343,92 @@ func loadBranches() ([]branch, error) { return branches, nil } -// remotes lists the configured remotes with "origin" first, so the conventional -// remote wins when several exist while repos whose only remote is named -// something else (upstream, fork, …) still resolve a default branch. -func remotes() []string { - out, err := runGit("remote") +// remoteRefs is one read of refs/remotes: which remote-tracking refs exist, and +// what each symbolic one points at. Reading them together is what replaces `git +// remote`, a `symbolic-ref` per remote and a `rev-parse --verify` per candidate +// — five subprocess starts on an ordinary one-remote repo, against this one. +type remoteRefs struct { + names []string // remote names, "origin" first + exists map[string]bool // fully qualified ref -> present + symref map[string]string // fully qualified symbolic ref -> the ref it names +} + +// loadRemoteRefs reads every remote-tracking ref in one call. The remote names +// come out of the refs rather than out of `git remote`: a remote with no fetched +// refs cannot supply a default branch, so it is nothing the caller could use. +func loadRemoteRefs() remoteRefs { + rr := remoteRefs{exists: map[string]bool{}, symref: map[string]string{}} + out, err := runGit("for-each-ref", "--format=%(refname)%00%(symref)", "refs/remotes") if err != nil { - return nil + return rr } - var names []string + seen := map[string]bool{} for _, line := range strings.Split(out, "\n") { + ref, target := "", "" if s := strings.TrimSpace(line); s != "" { - names = append(names, s) + ref, target, _ = strings.Cut(s, "\x00") + } + if ref == "" { + continue + } + rr.exists[ref] = true + if target != "" { + rr.symref[ref] = target + } + // The first segment after the namespace is the remote's name. + if name, _, ok := strings.Cut(shortRef(ref), "/"); ok && !seen[name] { + seen[name] = true + rr.names = append(rr.names, name) } } - sort.SliceStable(names, func(i, j int) bool { return names[i] == "origin" && names[j] != "origin" }) - return names + // origin first, so the conventional remote wins when several exist while a + // repo whose only remote is named something else (upstream, fork, …) still + // resolves a default branch. + sort.SliceStable(rr.names, func(i, j int) bool { return rr.names[i] == "origin" && rr.names[j] != "origin" }) + return rr } +// defaultBranchNames are the branch names treated as a repo's trunk, in order of +// preference. +var defaultBranchNames = []string{"main", "master"} + // localDefaultBranch returns the ref of a local main/master, skipping exclude (a // short branch name) so a branch is never compared against itself. Returns "" -// when neither exists. +// when neither exists. has answers whether a local branch of that name exists; +// a caller already holding the branch list passes a lookup into it rather than +// paying a subprocess per candidate. // // The resolvers below all return fully qualified refs, and the display layer // shortens them with shortRef. Resolving is the only place the namespace is // known for certain, so carrying it forward from here is what keeps a same-named // tag from being measured in place of the branch further down. -func localDefaultBranch(exclude string) string { - for _, c := range []string{"main", "master"} { - if c == exclude { - continue - } - if ref := branchRef(c); refExists(ref) { - return ref +func localDefaultBranch(exclude string, has func(string) bool) string { + for _, c := range defaultBranchNames { + if c != exclude && has(c) { + return branchRef(c) } } return "" } -// remoteDefault resolves the remote's default branch as a remote-tracking ref -// (e.g. "refs/remotes/origin/main"): /HEAD if set, else /main, -// else /master, trying each remote in turn. Returns "" when none can be -// found. -func remoteDefault() string { - for _, r := range remotes() { - // Deliberately not symbolic-ref --short: it shortens to the shortest - // *unambiguous* name, which a same-named tag turns into "remotes/origin/main". - if out, err := runGit("symbolic-ref", "refs/remotes/"+r+"/HEAD"); err == nil { - if s := strings.TrimSpace(out); s != "" { - return s - } +// gitHasBranch is the localDefaultBranch lookup for callers with no branch list +// to hand — it costs a subprocess per candidate. +func gitHasBranch(name string) bool { return refExists(branchRef(name)) } + +// remoteDefaultFrom resolves the remote's default branch as a remote-tracking +// ref (e.g. "refs/remotes/origin/main"): /HEAD if set, else +// /main, else /master, trying each remote in turn. Returns "" +// when none can be found. +func remoteDefaultFrom(rr remoteRefs) string { + for _, r := range rr.names { + // %(symref) yields the full ref, unlike `symbolic-ref --short`, which + // gives the shortest *unambiguous* name — "remotes/origin/main" as soon + // as a tag shares the name. + if t := rr.symref["refs/remotes/"+r+"/HEAD"]; t != "" { + return t } - for _, c := range []string{r + "/main", r + "/master"} { - if ref := "refs/remotes/" + c; refExists(ref) { + for _, c := range defaultBranchNames { + if ref := "refs/remotes/" + r + "/" + c; rr.exists[ref] { return ref } } @@ -394,13 +436,17 @@ func remoteDefault() string { return "" } +// remoteDefault reads the remote-tracking refs and resolves the default branch +// from them. +func remoteDefault() string { return remoteDefaultFrom(loadRemoteRefs()) } + // baseBranch returns the ref to diff a branch against: the remote default branch, // else a local main/master, excluding name itself. func baseBranch(name string) string { if def := remoteDefault(); def != "" { return def } - return localDefaultBranch(name) + return localDefaultBranch(name, gitHasBranch) } // riskCommitCount counts commits on name whose patch is not already present in @@ -459,6 +505,32 @@ func remoteMergedSet(def string) map[string]bool { // git call covers the whole list, so this costs nothing per branch. func localMergedSet() map[string]bool { return mergedSet("branch", "--merged", "HEAD") } +// repoReads holds the reads that do not depend on the branch list, so they can +// run in the same round as it. +type repoReads struct { + headMerged map[string]bool // local branches merged into HEAD + remotes remoteRefs +} + +// loadRepo reads the branch list and everything independent of it in one round. +// Starting a git subprocess costs about 6ms, and none of these three waits on +// another, so the depth of the chain is what the user waits on — not the work +// inside it. +func loadRepo() ([]branch, repoReads, error) { + var ( + branches []branch + err error + reads repoReads + ) + var wg sync.WaitGroup + wg.Add(3) + go func() { defer wg.Done(); branches, err = loadBranches() }() + go func() { defer wg.Done(); reads.headMerged = localMergedSet() }() + go func() { defer wg.Done(); reads.remotes = loadRemoteRefs() }() + wg.Wait() + return branches, reads, err +} + // fetchDoneMsg reports completion of an async `git fetch --all --prune`. type fetchDoneMsg struct{ err error } @@ -514,17 +586,26 @@ func loadDiff(name string) (diff, base string, err error) { // ---- model ---- func initialModel() (model, error) { - if _, err := runGit("rev-parse", "--is-inside-work-tree"); err != nil { + // The repo check runs beside the reads rather than ahead of them. It is here + // to give a clearer message than git's own, not to gate the work, and a + // serial subprocess start is most of what startup costs. + var repoErr error + checked := make(chan struct{}) + go func() { + _, repoErr = runGit("rev-parse", "--is-inside-work-tree") + close(checked) + }() + branches, reads, err := loadRepo() + <-checked + + if repoErr != nil { return model{}, fmt.Errorf("not a git repository (or git is unavailable)") } - branches, err := loadBranches() if err != nil { return model{}, err } - m := model{branches: branches, field: sortDate, ascending: false, height: 24, width: 100} - m.recomputeNameWidth() - m.refreshMergeInfo() - m.sortBranches() + m := model{field: sortDate, ascending: false, height: 24, width: 100} + m.applyBranches(branches, reads) return m, nil } @@ -574,6 +655,9 @@ func (m *model) scroll(delta int) { case stateDiff: m.diffTop += delta * 3 // 3 lines per wheel notch, like a pager m.clampDiff() + case stateConfirm, stateForcePrompt, stateDeleting, stateResult: + m.bodyTop += delta * 3 + m.clampBody() } } @@ -624,10 +708,10 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.status = "" return m, nil } - if branches, err := loadBranches(); err == nil { + if branches, reads, err := loadRepo(); err == nil { // A fetch is non-destructive, so both the cursor (by name, in // sortBranches) and the user's pending marks survive it. - m.applyBranches(carryMarks(m.branches, branches)) + m.applyBranches(carryMarks(m.branches, branches), reads) } // Auto-select only gone branches that carry nothing missing from the // base. Ones holding unique commits are left unselected so discarding @@ -662,6 +746,7 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } if m.deletesDone() >= len(m.results) { m.reloadBranches() + m.bodyTop = 0 if len(m.forceableFailures()) > 0 { m.state = stateForcePrompt } else { @@ -704,6 +789,8 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { switch msg.String() { case "q", "ctrl+c", "enter", "esc": return m, tea.Quit + default: + m.scrollKeys(msg.String()) } case stateHelp: if msg.String() == "ctrl+c" { @@ -788,7 +875,7 @@ func (m model) updateList(msg tea.KeyMsg) (tea.Model, tea.Cmd) { case "d", "enter": if len(m.selectedBranches()) > 0 { m.measureSelectedRisk() // the confirm screen states what each delete costs - m.state = stateConfirm + m.state, m.bodyTop = stateConfirm, 0 } } return m, nil @@ -808,6 +895,8 @@ func (m model) updateConfirm(msg tea.KeyMsg) (tea.Model, tea.Cmd) { m.state = stateList case "ctrl+c": return m, tea.Quit + default: + m.scrollKeys(msg.String()) } return m, nil } @@ -837,7 +926,7 @@ func (m *model) startDeletions(includeRemote bool) tea.Cmd { sel := m.selectedBranches() m.results = make([]deleteResult, len(sel)) m.spinnerFrame = 0 - m.state = stateDeleting + m.state, m.bodyTop = stateDeleting, 0 cmds := []tea.Cmd{spinnerTickCmd()} for i, b := range sel { @@ -875,11 +964,13 @@ func (m model) updateForcePrompt(msg tea.KeyMsg) (tea.Model, tea.Cmd) { switch msg.String() { case "y", "Y": m.forceDeleteUnmerged() - m.state = stateResult + m.state, m.bodyTop = stateResult, 0 case "n", "N", "esc", "q", "enter": - m.state = stateResult + m.state, m.bodyTop = stateResult, 0 case "ctrl+c": return m, tea.Quit + default: + m.scrollKeys(msg.String()) } return m, nil } @@ -988,51 +1079,70 @@ func carryMarks(old, fresh []branch) []branch { // derived from it (name-width, merge info, sort order). Callers set their own // cursor policy around it. This is the single refresh core shared by // reloadBranches and the fetch handler. -func (m *model) applyBranches(branches []branch) { +func (m *model) applyBranches(branches []branch, reads repoReads) { m.branches = branches m.recomputeNameWidth() - m.refreshMergeInfo() + m.refreshMergeInfo(reads) m.sortBranches() } // reloadBranches refreshes the branch list from git and resets the view to the // top — appropriate after a mutation that may have removed the cursor's branch. func (m *model) reloadBranches() { - if branches, err := loadBranches(); err == nil { + if branches, reads, err := loadRepo(); err == nil { m.cursor = 0 m.top = 0 - m.applyBranches(branches) + m.applyBranches(branches, reads) } } +// hasBranch reports whether the loaded list holds a local branch of that name. +func (m model) hasBranch(name string) bool { + for _, b := range m.branches { + if b.name == name { + return true + } + } + return false +} + // refreshMergeInfo caches the remote default branch, marks each branch whose // upstream is merged into it or whose tip is merged into HEAD, and measures what -// deleting it would cost. Call after every branch (re)load. -func (m *model) refreshMergeInfo() { - // localMergedSet needs nothing from the remote resolution, so let the two - // runs of subprocesses overlap rather than queue. - var headMerged map[string]bool - local := make(chan struct{}) - go func() { - headMerged = localMergedSet() - close(local) - }() - defRef := remoteDefault() - merged := remoteMergedSet(defRef) - <-local +// deleting it would cost. Call after every branch (re)load, passing the reads +// that came back with it. +func (m *model) refreshMergeInfo(reads repoReads) { + defRef := remoteDefaultFrom(reads.remotes) m.riskBaseRef = defRef if m.riskBaseRef == "" { - m.riskBaseRef = localDefaultBranch("") + // The branch list is already loaded, so this candidate check is a lookup + // rather than a subprocess per name. + m.riskBaseRef = localDefaultBranch("", m.hasBranch) } // The refs drive git; the short forms are what the views print. m.remoteDefault = shortRef(defRef) m.riskBase = shortRef(m.riskBaseRef) + // Round two. Neither query needs the other's answer, and each is a + // subprocess start, so they go together. + var merged map[string]bool + remote := make(chan struct{}) + go func() { + merged = remoteMergedSet(defRef) + close(remote) + }() + // One query answers "is this branch's tip already in the base?" for the whole + // list, which is the answer for most branches a prune touches. + m.baseMerged = nil + if m.riskBaseRef != "" { + m.baseMerged = mergedSet("branch", "--merged", m.riskBaseRef) + } + <-remote + for i := range m.branches { b := &m.branches[i] b.remoteMerged = b.upstream != "" && merged[b.upstream] - b.headMerged = headMerged[b.name] + b.headMerged = reads.headMerged[b.name] b.riskMeasured = false // the branch was just reloaded; any old count is stale } // Only gone branches are measured up front, because `p` consults the count to @@ -1054,6 +1164,14 @@ func (m *model) measureRisk(want func(branch) bool) { if m.branches[i].riskMeasured || !want(m.branches[i]) { continue } + // A branch already contained in the base has an empty base..branch range, + // so `git cherry` would report nothing. Answer from the set instead of + // spawning the subprocess. + if m.baseMerged[m.branches[i].name] { + m.branches[i].riskCommits = 0 + m.branches[i].riskMeasured = true + continue + } wg.Add(1) go func() { defer wg.Done() @@ -1117,6 +1235,99 @@ func (m model) View() string { } } +// page renders a screen as fixed header lines, a window into body, and fixed +// footer lines. The prompt on these screens is the whole point of them, and it +// lives in the footer: without a window, a wide selection pushes the question +// past the last row of the terminal, where the user cannot read what they are +// answering. Rendering only the visible rows is also what keeps a long list off +// the cost of every frame. +func (m model) page(header, body, footer []string) string { + rows := m.bodyRows(len(header), len(footer), len(body)) + top := max(0, min(m.bodyTop, len(body)-rows)) + end := min(top+rows, len(body)) + + var b strings.Builder + for _, l := range header { + b.WriteString(l + "\n") + } + for _, l := range body[top:end] { + b.WriteString(l + "\n") + } + if len(body) > rows { + b.WriteString(dimStyle.Render(fmt.Sprintf("[%d-%d / %d] ↑/↓ scroll · space/ctrl+d page · g/G top/bottom", + top+1, end, len(body))) + "\n") + } + for _, l := range footer { + b.WriteString(l + "\n") + } + return b.String() +} + +// bodyRows is how many body lines fit between header and footer. A body that +// does not fit gives up one more row to the position line, so that line never +// pushes the footer off in its turn. +func (m model) bodyRows(header, footer, body int) int { + rows := max(1, m.height-header-footer) + if body > rows { + rows = max(1, rows-1) + } + return rows +} + +// pageParts returns the current state's screen as header, body and footer. The +// scroll keys measure against it too, so the window and the clamp can never +// disagree about how far down the body goes. +func (m model) pageParts() (header, body, footer []string) { + switch m.state { + case stateConfirm: + return m.confirmParts() + case stateForcePrompt: + return m.forcePromptParts() + case stateDeleting: + return m.deletingParts() + case stateResult: + return m.resultParts() + } + return nil, nil, nil +} + +// bodyWindow reports the visible row count and the total body length for the +// current state. +func (m model) bodyWindow() (rows, total int) { + header, body, footer := m.pageParts() + return m.bodyRows(len(header), len(footer), len(body)), len(body) +} + +func (m *model) clampBody() { + rows, total := m.bodyWindow() + m.bodyTop = max(0, min(m.bodyTop, total-rows)) +} + +// scrollKeys applies the shared paging keys to a windowed screen. It reports +// whether the key was one of them, so each screen's own keys stay in charge: +// callers must offer their answers first. +func (m *model) scrollKeys(s string) bool { + rows, total := m.bodyWindow() + switch s { + case "up", "k": + m.bodyTop-- + case "down", "j": + m.bodyTop++ + case "ctrl+u", "pgup": + m.bodyTop -= max(1, rows/2) + case "ctrl+d", "pgdown", " ": + m.bodyTop += max(1, rows/2) + case "g", "home": + m.bodyTop = 0 + case "G", "end": + m.bodyTop = total + default: + return false + } + m.clampBody() + return true +} + func colorizeDiffLine(line string) string { switch { case strings.HasPrefix(line, "+++"), strings.HasPrefix(line, "---"): @@ -1341,6 +1552,7 @@ func (m model) helpView() string { {"d, enter", "delete selected branches (local)"}, {"", "on confirm: y = local only · R = local + remote"}, {"", "(unmerged -d failures prompt to retry with -D)"}, + {"", "long lists scroll: ↑/↓ · space/ctrl+d · g/G"}, {"?", "toggle this help screen"}, {"q, ctrl+c", "quit"}, }) @@ -1368,65 +1580,67 @@ func (m model) helpView() string { return b.String() } -func (m model) confirmView() string { - var b strings.Builder +func (m model) confirmParts() (header, body, footer []string) { sel := m.selectedBranches() - - b.WriteString(headerStyle.Render("Confirm deletion")) - b.WriteString("\n\n") flag := "-d (safe)" if m.force { flag = "-D (force)" } remoteCount := countArmedRemotes(sel) - b.WriteString(fmt.Sprintf("Local delete mode: %s\n", flag)) - b.WriteString(fmt.Sprintf("Deleting %d local branch(es), %d remote branch(es).\n\n", len(sel), remoteCount)) + header = []string{ + headerStyle.Render("Confirm deletion"), + "", + fmt.Sprintf("Local delete mode: %s", flag), + fmt.Sprintf("Deleting %d local branch(es), %d remote branch(es).", len(sel), remoteCount), + "", + } for _, br := range sel { - b.WriteString(" " + cursorStyle.Render("• "+br.name) + "\n") + body = append(body, " "+cursorStyle.Render("• "+br.name)) date := br.committed.Format("2006-Jan-02") if br.committedRel != "" { date += " (" + br.committedRel + ")" } - b.WriteString(" " + dimStyle.Render(fmt.Sprintf("%s %s %s", br.hash, date, truncate(br.subject, 50))) + "\n") + body = append(body, " "+dimStyle.Render(fmt.Sprintf("%s %s %s", br.hash, date, truncate(br.subject, 50)))) switch { case br.gone: - b.WriteString(" " + goneStyle.Render("upstream gone: "+br.upstream+" — will prune with -D (force)") + "\n") + body = append(body, " "+goneStyle.Render("upstream gone: "+br.upstream+" — will prune with -D (force)")) case br.upstream != "": - b.WriteString(" " + dimStyle.Render("upstream: "+br.upstream) + " " + m.trackStr(br) + "\n") + body = append(body, " "+dimStyle.Render("upstream: "+br.upstream)+" "+m.trackStr(br)) default: - b.WriteString(" " + dimStyle.Render("no upstream") + "\n") + body = append(body, " "+dimStyle.Render("no upstream")) } if br.upstream != "" && m.remoteDefault != "" { if br.remoteMerged { - b.WriteString(" " + okStyle.Render("✓ merged into "+m.remoteDefault) + "\n") + body = append(body, " "+okStyle.Render("✓ merged into "+m.remoteDefault)) } else { - b.WriteString(" " + goneStyle.Render("⚠ not merged into "+m.remoteDefault) + "\n") + body = append(body, " "+goneStyle.Render("⚠ not merged into "+m.remoteDefault)) } } if br.deleteRemote && br.upstream != "" { - b.WriteString(" " + errStyle.Render(fmt.Sprintf("+ delete remote %s/%s", br.remoteName(), br.remoteBranch())) + "\n") + body = append(body, " "+errStyle.Render(fmt.Sprintf("+ delete remote %s/%s", br.remoteName(), br.remoteBranch()))) } if w := m.riskWarning(br); w != "" { - b.WriteString(" " + errStyle.Render(w) + "\n") + body = append(body, " "+errStyle.Render(w)) } - b.WriteString("\n") + body = append(body, "") } - b.WriteString(headerStyle.Render("Delete these branches? ")) + prompt := headerStyle.Render("Delete these branches? ") if remoteCount > 0 { - b.WriteString(dimStyle.Render(fmt.Sprintf("(y = local only · R = local + remote (%d) · n/esc = cancel)", remoteCount))) + prompt += dimStyle.Render(fmt.Sprintf("(y = local only · R = local + remote (%d) · n/esc = cancel)", remoteCount)) } else { - b.WriteString(dimStyle.Render("(y = yes · n/esc = cancel)")) + prompt += dimStyle.Render("(y = yes · n/esc = cancel)") } - b.WriteString("\n") - return b.String() + return header, body, []string{prompt} } +func (m model) confirmView() string { return m.page(m.confirmParts()) } + // riskWarning states the cost of deleting br, or "" when the delete is clean. // It covers every branch git's safe delete would refuse plus gone branches, // which take the -D path regardless: under -D the unmerged commits are @@ -1451,92 +1665,92 @@ func (m model) riskWarning(br branch) string { return "⚠ not fully merged — safe delete (-d) will fail; use force (f)" } -func (m model) forcePromptView() string { - var b strings.Builder +func (m model) forcePromptParts() (header, body, footer []string) { failures := m.forceableFailures() - b.WriteString(headerStyle.Render("Force delete unmerged branches?")) - b.WriteString("\n\n") - b.WriteString(fmt.Sprintf("%d branch(es) were refused by safe delete (-d) because they are not\n", len(failures))) - b.WriteString("fully merged. Force deleting (-D) will ") - b.WriteString(errStyle.Render("permanently discard their unmerged commits")) - b.WriteString(".\n\n") + header = []string{ + headerStyle.Render("Force delete unmerged branches?"), + "", + fmt.Sprintf("%d branch(es) were refused by safe delete (-d) because they are not", len(failures)), + "fully merged. Force deleting (-D) will " + errStyle.Render("permanently discard their unmerged commits") + ".", + "", + } for _, r := range failures { - b.WriteString(" " + cursorStyle.Render("• "+r.br.name) + "\n") + body = append(body, " "+cursorStyle.Render("• "+r.br.name)) // Every branch here failed -d, so its risk was measured before the delete // ran: riskCommits == 0 means either nothing is missing from the base or // there was no base to measure against. switch { case r.br.riskCommits > 0: - b.WriteString(" " + errStyle.Render(fmt.Sprintf("⚠ %d commit(s) not in %s will be lost", r.br.riskCommits, m.riskBase)) + "\n") + body = append(body, " "+errStyle.Render(fmt.Sprintf("⚠ %d commit(s) not in %s will be lost", r.br.riskCommits, m.riskBase))) case m.riskBase == "": - b.WriteString(" " + errStyle.Render("⚠ no base branch to compare against — unmerged commits may be lost") + "\n") + body = append(body, " "+errStyle.Render("⚠ no base branch to compare against — unmerged commits may be lost")) default: - b.WriteString(" " + dimStyle.Render("no commits missing from "+m.riskBase) + "\n") + body = append(body, " "+dimStyle.Render("no commits missing from "+m.riskBase)) } if r.remoteSkipped { - b.WriteString(" " + errStyle.Render(fmt.Sprintf("+ remote %s/%s will be deleted once the branch is gone", r.br.remoteName(), r.br.remoteBranch())) + "\n") + body = append(body, " "+errStyle.Render(fmt.Sprintf("+ remote %s/%s will be deleted once the branch is gone", r.br.remoteName(), r.br.remoteBranch()))) } } - b.WriteString("\n") - b.WriteString(headerStyle.Render("Force delete (-D) these branches? ")) - b.WriteString(dimStyle.Render("(y = yes, discard · n/esc = keep them)")) - b.WriteString("\n") - return b.String() + footer = []string{ + "", + headerStyle.Render("Force delete (-D) these branches? ") + dimStyle.Render("(y = yes, discard · n/esc = keep them)"), + } + return header, body, footer } -// writeResultLines renders one completed deletion result (local, then remote if -// tried) into b. Shared by the results screen and the live deleting screen. -func writeResultLines(b *strings.Builder, r deleteResult) { +func (m model) forcePromptView() string { return m.page(m.forcePromptParts()) } + +// appendResultLines adds one completed deletion result (local, then remote if +// tried) to dst. Shared by the results screen and the live deleting screen. +func appendResultLines(dst []string, r deleteResult) []string { if r.localOK { - b.WriteString(okStyle.Render(" ✓ ") + "deleted local " + r.br.name + "\n") + dst = append(dst, okStyle.Render(" ✓ ")+"deleted local "+r.br.name) } else { - b.WriteString(errStyle.Render(" ✗ ") + "local " + r.br.name + ": " + r.localErr + "\n") + dst = append(dst, errStyle.Render(" ✗ ")+"local "+r.br.name+": "+r.localErr) } switch { case r.remoteSkipped: // Say why the armed remote survived, or it reads as a silent failure. - b.WriteString(errStyle.Render(" ! ") + "kept remote " + r.br.remoteName() + "/" + r.br.remoteBranch() + ": local delete failed\n") + dst = append(dst, errStyle.Render(" ! ")+"kept remote "+r.br.remoteName()+"/"+r.br.remoteBranch()+": local delete failed") case r.remoteTried && r.remoteOK: - b.WriteString(okStyle.Render(" ✓ ") + "deleted remote " + r.br.name + "\n") + dst = append(dst, okStyle.Render(" ✓ ")+"deleted remote "+r.br.name) case r.remoteTried: - b.WriteString(errStyle.Render(" ✗ ") + "remote " + r.br.name + ": " + r.remoteErr + "\n") + dst = append(dst, errStyle.Render(" ✗ ")+"remote "+r.br.name+": "+r.remoteErr) } + return dst } -func (m model) deletingView() string { - var b strings.Builder +func (m model) deletingParts() (header, body, footer []string) { spin := spinnerFrames[m.spinnerFrame%len(spinnerFrames)] - b.WriteString(headerStyle.Render(fmt.Sprintf("%s Deleting… (%d/%d)", spin, m.deletesDone(), len(m.results)))) - b.WriteString("\n\n") + header = []string{ + headerStyle.Render(fmt.Sprintf("%s Deleting… (%d/%d)", spin, m.deletesDone(), len(m.results))), + "", + } for _, r := range m.results { if r.done { - writeResultLines(&b, r) + body = appendResultLines(body, r) } else { - b.WriteString(dimStyle.Render(" "+spin+" deleting "+r.br.name+"…") + "\n") + body = append(body, dimStyle.Render(" "+spin+" deleting "+r.br.name+"…")) } } - b.WriteString("\n") - b.WriteString(dimStyle.Render("working — ctrl+c to abort")) - b.WriteString("\n") - return b.String() + return header, body, []string{"", dimStyle.Render("working — ctrl+c to abort")} } -func (m model) resultView() string { - var b strings.Builder - b.WriteString(headerStyle.Render("Results")) - b.WriteString("\n\n") +func (m model) deletingView() string { return m.page(m.deletingParts()) } + +func (m model) resultParts() (header, body, footer []string) { + header = []string{headerStyle.Render("Results"), ""} for _, r := range m.results { - writeResultLines(&b, r) + body = appendResultLines(body, r) } - b.WriteString("\n") - b.WriteString(dimStyle.Render("press q/enter to quit")) - b.WriteString("\n") - return b.String() + return header, body, []string{"", dimStyle.Render("press q/enter to quit")} } +func (m model) resultView() string { return m.page(m.resultParts()) } + // versionString reports the build's commit and date using Go's automatic VCS // stamping (populated when built with `go build` inside the repo). Fields fall // back to "unknown" when build info is unavailable (e.g. `go run`). diff --git a/main_test.go b/main_test.go index de071d9..d80b00c 100644 --- a/main_test.go +++ b/main_test.go @@ -4,7 +4,9 @@ import ( "fmt" "os" "os/exec" + "path/filepath" "regexp" + "strconv" "strings" "sync" "testing" @@ -1103,13 +1105,16 @@ func TestNonOriginRemoteResolves(t *testing.T) { } } -// origin is preferred when several remotes are configured. +// origin is preferred when several remotes are configured. The names come out +// of refs/remotes now, so the second remote has to be fetched to count — one +// that has never been fetched holds no ref a default branch could resolve to. func TestRemotesPrefersOrigin(t *testing.T) { repo := setupRepo(t) chdir(t, repo) git(t, repo, "remote", "add", "aaa-fork", repo) - got := remotes() + git(t, repo, "fetch", "-q", "aaa-fork") + got := loadRemoteRefs().names if len(got) == 0 || got[0] != "origin" { t.Fatalf("origin should sort first, got %v", got) } @@ -1477,7 +1482,7 @@ func TestTagShadowingBranchName(t *testing.T) { git(t, repo, "tag", "main", "feature") git(t, repo, "checkout", "-q", "trunk") - if got := localDefaultBranch(""); got != "" { + if got := localDefaultBranch("", gitHasBranch); got != "" { t.Fatalf("a tag must not pose as the local default branch, got %q", got) } m, err := initialModel() @@ -1704,7 +1709,7 @@ func TestLocalOnlyRepo(t *testing.T) { repo := setupLocalRepo(t) chdir(t, repo) - if got := remotes(); len(got) != 0 { + if got := loadRemoteRefs().names; len(got) != 0 { t.Fatalf("no remotes should be configured, got %v", got) } if got := remoteDefault(); got != "" { @@ -2192,3 +2197,197 @@ func TestVersionString(t *testing.T) { } } } + +// setupGoneMerged builds n branches that are merged into main and report their +// upstream as gone. The upstream is configured rather than pushed and pruned: +// git reads "gone" straight out of the config when the remote-tracking ref is +// absent, which keeps the fixture free of network work. +func setupGoneMerged(t *testing.T, n int) string { + t.Helper() + tmp := initRepo(t, "main") + commitFile(t, tmp, "a", "a") + git(t, tmp, "remote", "add", "origin", filepath.Join(t.TempDir(), "absent.git")) + for i := 0; i < n; i++ { + name := fmt.Sprintf("feature/%d", i) + git(t, tmp, "checkout", "-q", "-b", name, "main") + commitFile(t, tmp, fmt.Sprintf("f%d", i), name) + git(t, tmp, "checkout", "-q", "main") + git(t, tmp, "merge", "-q", "--no-ff", name, "-m", "m"+name) + git(t, tmp, "config", "branch."+name+".remote", "origin") + git(t, tmp, "config", "branch."+name+".merge", "refs/heads/"+name) + } + return tmp +} + +// A branch whose tip is already in the base has an empty base..branch range, so +// `git cherry` can only report nothing. One `branch --merged` query answers that +// for the whole list, and it has to stay one query: a subprocess per branch put +// half a second on the load of every repo full of pruned branches. +func TestMergedBranchesCostNoSubprocess(t *testing.T) { + const n = 30 + chdir(t, setupGoneMerged(t, n)) + + before := gitProcs.total.Load() + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + spawned := gitProcs.total.Load() - before + + // A literal bound, not a formula over n: a fan-out that comes back must fail + // this rather than scale along with it. + const fewProcesses = 12 + if spawned > fewProcesses { + t.Fatalf("loading %d merged branches ran %d git processes; it should not grow with the branch count", n, spawned) + } + + measured := 0 + for _, b := range m.branches { + if !b.gone { + continue + } + if !b.riskMeasured { + t.Fatalf("%s was never measured, so p cannot tell whether it is safe to select", b.name) + } + if b.riskCommits != 0 { + t.Fatalf("%s is merged into the base; the shortcut must agree with git cherry, got %d", b.name, b.riskCommits) + } + measured++ + } + if measured != n { + t.Fatalf("expected %d gone branches, saw %d", n, measured) + } +} + +// The shortcut must not swallow the branches it cannot answer for: one that is +// merged and one that is not have to come back with the counts git cherry gives. +func TestUnmergedBranchIsStillMeasured(t *testing.T) { + repo := setupGoneMerged(t, 1) // feature/0: merged, gone + chdir(t, repo) + + git(t, repo, "checkout", "-q", "-b", "feature/kept", "main") + commitFile(t, repo, "kept", "kept") + git(t, repo, "checkout", "-q", "main") + git(t, repo, "config", "branch.feature/kept.remote", "origin") + git(t, repo, "config", "branch.feature/kept.merge", "refs/heads/feature/kept") + + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + if b := find(m.branches, "feature/0"); b == nil || b.riskCommits != 0 { + t.Fatalf("the merged branch should cost nothing: %+v", b) + } + kept := find(m.branches, "feature/kept") + if kept == nil || !kept.riskMeasured || kept.riskCommits != 1 { + t.Fatalf("the unmerged branch still needs its real count: %+v", kept) + } +} + +// screenRows counts the rows a rendered view occupies. +func screenRows(view string) int { return len(strings.Split(strings.TrimRight(view, "\n"), "\n")) } + +// The confirm screen asks a question whose answer deletes branches. A wide +// selection used to render every branch in full, pushing that question past the +// last row of the terminal — the user answered a prompt they could not read. +func TestConfirmPromptStaysOnScreen(t *testing.T) { + const n = 30 + chdir(t, setupGoneMerged(t, n)) + + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + selectAll(&m, false) + next, _ := m.Update(key("d")) + m = next.(model) + wantState(t, m, stateConfirm, "d with a selection opens the confirm screen") + + view := m.confirmView() + if rows := screenRows(view); rows > m.height { + t.Fatalf("%d selected branches rendered %d rows into a %d-row terminal", n, rows, m.height) + } + if !strings.Contains(stripANSI(view), "Delete these branches?") { + t.Fatalf("the prompt must be on the screen:\n%s", stripANSI(view)) + } +} + +// Windowing must not hide a branch from the user: every one of them has to be +// reachable by scrolling, and the prompt has to stay put while they do it. +func TestConfirmScrollReachesTheLastBranch(t *testing.T) { + const n = 30 + chdir(t, setupGoneMerged(t, n)) + + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + selectAll(&m, false) + m.state = stateConfirm + + sel := m.selectedBranches() + last := sel[len(sel)-1].name // the confirm list follows the sort order + if strings.Contains(stripANSI(m.confirmView()), last) { + t.Fatalf("%s should start below the window; the fixture proves nothing", last) + } + + next, _ := m.Update(key("G")) + m = next.(model) + view := stripANSI(m.confirmView()) + if !strings.Contains(view, last) { + t.Fatalf("G must reach the last branch:\n%s", view) + } + if !strings.Contains(view, "Delete these branches?") { + t.Fatalf("the prompt must survive scrolling:\n%s", view) + } + if rows := screenRows(m.confirmView()); rows > m.height { + t.Fatalf("the scrolled screen is %d rows in a %d-row terminal", rows, m.height) + } + + // The answer keys still answer; they are not swallowed by the scroll handler. + next, _ = m.Update(key("n")) + wantState(t, next.(model), stateList, "n cancels from a scrolled confirm screen") +} + +// The results screen has the same shape and the same failure: a wide prune used +// to bury "press q/enter to quit" under its own output. +func TestResultViewWindowsItsBody(t *testing.T) { + m := model{height: 24, width: 100, state: stateResult} + for i := 0; i < 40; i++ { + m.results = append(m.results, deleteResult{ + br: branch{name: "feature/" + strconv.Itoa(i)}, done: true, localOK: true, + }) + } + if rows := screenRows(m.resultView()); rows > m.height { + t.Fatalf("40 results rendered %d rows into a %d-row terminal", rows, m.height) + } + if !strings.Contains(stripANSI(m.resultView()), "press q/enter to quit") { + t.Fatal("the footer must stay on the screen") + } + + next, _ := m.Update(key("G")) + m = next.(model) + if o := stripANSI(m.resultView()); !strings.Contains(o, "feature/39") { + t.Fatalf("G must reach the last result:\n%s", o) + } +} + +// A body that fits needs no position line and no scrolling — the common case +// must not grow furniture it does not need. +func TestShortBodyRendersWhole(t *testing.T) { + m := model{height: 24, width: 100, state: stateResult} + for i := 0; i < 3; i++ { + m.results = append(m.results, deleteResult{ + br: branch{name: "feature/" + strconv.Itoa(i)}, done: true, localOK: true, + }) + } + out := stripANSI(m.resultView()) + if strings.Contains(out, "scroll") { + t.Fatalf("a body that fits should show no position line:\n%s", out) + } + for i := 0; i < 3; i++ { + if !strings.Contains(out, "feature/"+strconv.Itoa(i)) { + t.Fatalf("every result must be shown:\n%s", out) + } + } +}