Drive deletes through the live path and cap git concurrency - #4
Merged
Conversation
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0157b1zyhs2FVFBmaNWS6wjS
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 <base>` 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 <base> 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WnKYkFZWMaTCoYumjAkTGD
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three pieces of work that ended up sharing the same test file.
Test the delete path users actually run.
performDeletionswas 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, andperformDeletionsis 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 cherryis 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.