diff --git a/README.md b/README.md index 69e622d..0fbdead 100644 --- a/README.md +++ b/README.md @@ -51,7 +51,7 @@ git_pruner version # also --version, -v | `tab` | Switch between local branches and [remote-only branches](#remote-only-branches) | | `c` | Checkout the branch under the cursor (`git switch`); on a remote-only row, create a local branch that tracks it | | `/` | Filter branches by name; `enter` keeps it, `esc` clears it | -| `r` | Toggle "also delete remote" for the row (needs an upstream on a remote that is not [protected](#protected-branches)) | +| `r` | Toggle "also delete remote" for the row (needs an upstream branch on a remote that is not [protected](#protected-branches)) | | `v` | View the branch's diff (through [delta](https://github.com/dandavison/delta) when installed) | | `x` | Select gone branches that hold no unique work (no fetch) | | `p` | Fetch `--all --prune`, then select gone branches that hold no unique work | @@ -170,8 +170,9 @@ claiming the work is unrecoverable. - Remote: when armed with `r`, runs `git push --delete ` on the branch's upstream, as git records it. That upstream need not share the branch's name: a branch made with `git switch -c feat origin/main` tracks `main`. So `r` refuses when the upstream is the - default branch or protected, and when the upstream is a local branch (`--track main`), which - has no remote branch to delete. Because this affects shared history, remote deletion + default branch or protected, when the upstream is a local branch (`--track main`), and when + it is not a branch at all (a pull request head such as `refs/pull/1/head`). None of these + has a remote branch to delete. Because this affects shared history, remote deletion requires the explicit `R` key on the confirmation screen — plain `y` deletes locals only. The confirmation screen also shows, per branch, whether the upstream is merged into the remote default (`✓ merged` / `⚠ not merged`) to help you judge whether the remote is safe to delete. @@ -221,13 +222,16 @@ Deleting one is a push, so only `R` does it; `y` never touches a remote. The exi prints the `git push` command that puts a deleted remote branch back. Each row is mapped back to its branch on the remote through the remote's fetch refspecs -(`remote..fetch`), so a refspec that renames branches deletes the right one. A row that +(`remote..fetch`), so a refspec that renames branches deletes the right one. The +confirmation screen names that branch when it differs from the row. A row that maps to something other than one remote branch is not listed: pull request refs fetched with `+refs/pull/*/head:refs/remotes/origin/pr/*` are not branches a push can delete. ## Protected branches -The default branch is always protected. Add your own with name globs: +The default branch is always protected, and so is the default branch of each remote whose +`/HEAD` is set (`git remote set-head --auto` sets it). Add your own with name +globs: ```sh git config --add pruner.protect 'release/*' @@ -235,8 +239,9 @@ git config --add pruner.protect develop ``` A protected branch shows `P` and cannot be selected by any key. Remote rows are matched by the -branch part of their name, so `release/*` covers `origin/release/1.0` too. The same check -applies to the remote branch a local branch tracks: `r` cannot arm the delete of +branch part of their name, so `release/*` covers `origin/release/1.0` too. When a fetch +refspec renames branches, the glob is also matched against the name on the remote. The same +check applies to the remote branch a local branch tracks: `r` cannot arm the delete of `origin/release/1.0` from a local `hotfix` made from it. ## Script mode @@ -263,7 +268,10 @@ login. In a partial clone (`git clone --filter=blob:none`), git fetches file contents when it needs them, so `git cherry` (the commit count) and `git diff` (`v`) can also reach the remote. They get the same limits there. A commit count that fails is shown as unknown, never as zero, so `x` and -`p` leave that branch unselected. +`p` leave that branch unselected. Commit counts run in the background, so the screen keeps +drawing while they wait; `d` opens the confirmation screen when they finish. `git switch` (`c`) +can fetch too. It runs without a terminal, but with no time limit, because stopping a checkout +partway leaves the repository locked. ## Settings diff --git a/bugs.md b/bugs.md index 383cd8b..26708f1 100644 --- a/bugs.md +++ b/bugs.md @@ -6,3 +6,17 @@ - [x] `r` could delete the wrong remote branch: a protected or default upstream, or `origin`'s branch for a local upstream - [x] Remote-only rows ignored fetch refspecs and could delete a teammate's branch of the same short name - [x] Partial clones: `git cherry` failing offline counted as zero risky commits, so `x` selected unmerged gone branches + +### October 10 +- [x] `r` on a branch made from a pull request (`merge = refs/pull/1/head`) pushed the delete of a teammate's branch with the tracking name +- [x] A fetch refspec that renames branches (`refs/heads/jb/*:refs/remotes/origin/*`) stopped `pruner.protect` globs from matching the name shown +- [x] Only the default remote's trunk was protected; `upstream/master` next to `origin/main` was not +- [x] The confirm screen did not name the remote branch a renamed remote row deletes +- [x] A refusal from `r` stayed on screen after `r` armed another row +- [x] A repo path with `[`, `*` or `?` hid linked worktrees from the held-branch check +- [x] A `BISECT_START` left behind without `BISECT_LOG` locked a branch git would delete +- [x] git older than 2.31 turned off the held-branch check with no error +- [x] `c` on a remote row whose remote name holds `/` (`team/a/x`) made the local branch `a/x` +- [x] Partial clones: `git switch` could show an ssh prompt over the screen +- [x] Partial clones: commit counts ran 3 at a time (the push cap), not 8 +- [x] Commit counts ran inside the screen's update loop, so a slow partial clone froze the screen diff --git a/docs/improvements.md b/docs/improvements.md index bb12c4e..da7d4e8 100644 --- a/docs/improvements.md +++ b/docs/improvements.md @@ -130,7 +130,7 @@ 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 +every git invocation already passes through, and `networkBound(args)` (now `networkUse`) 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. diff --git a/docs/test-coverage-gaps.md b/docs/test-coverage-gaps.md index 000af8a..1086a46 100644 --- a/docs/test-coverage-gaps.md +++ b/docs/test-coverage-gaps.md @@ -204,15 +204,16 @@ and on every branch a `rebase --update-refs` will move. The worktree's HEAD is d `origin`'s `main`, a remote the branch never used. Fixed by `remoteProtected` (trunk and protect checks on `remoteBranch()`) and -`remoteDeletable()`, which also refuses a local upstream. *(`TestRemoteDeleteRefusesWrongTargets`)* +`remoteDeletable()` (now `remoteDelete()`, see 31), which also refuses a local upstream. +*(`TestRemoteDeleteRefusesWrongTargets`)* ### 17. Remote rows ignored fetch refspecs *(confirmed — fixed)* With `+refs/heads/jb/*:refs/remotes/origin/*`, the row `origin/old` is `jb/old` on the remote, but the delete pushed `refs/heads/old` and removed a teammate's branch. `mapRemoteRows` now maps each row back through `remote..fetch`. Rows that map to no branch, or to more than one -source (the usual pull request refspec next to the default one), are dropped. Every remote's -trunk is now protected in the remote view, not only the default remote's. +source (the usual pull request refspec next to the default one), are dropped. This fix claimed +that every remote's trunk was protected in the remote view. It was not until item 21. *(`TestRemoteRowsFollowFetchRefspecs`)* ### 18. Partial clones reach the network from `cherry` and `diff` *(confirmed — fixed)* @@ -236,6 +237,137 @@ kill a checkout partway and leave `index.lock` behind. - `git am` in progress: HEAD stays on the branch, so it already shows as current. - Locked worktree: the remove is refused as designed. +## Tier 5 — review of the Tier 4 fixes — **DONE 2026-10-10** (one new item, 34, open) + +A `/code-review` of the Tier 4 work found these on 2026-10-09, before its commit. The review +reproduced 19, 21 and 22 in throwaway repos. A read of the code confirmed 19 and 20. Reproduce +each other one before the fix. Every fix gets a test that fails with the fix taken out, then +`go test -race`, `gofmt` and `go vet`, and README and `bugs.md` updates. + +### Order + +1. **Done.** Commit the Tier 4 work first, so the review fixes have their own diff: `c4846f2`, + merged in PR #7. +2. **Done.** 31, then 19 inside it: one function, one place for the `r` rules. +3. **Done.** 20, 21, 26, 25: what `r` and the confirm screen protect and show. +4. **Done.** 22, 23, 24: all in `loadHeldBranches`. +5. **Done.** 28, 27: small, separate. +6. **Done.** 29 and 30 together: both split `networkBound`. +7. **Done.** 32 and 33 last: larger, and not wrong today. 33 went first, so 32 carries a + plain count. + +### Correctness + +**19. `r` can delete a branch that is not the upstream** — `git.go` `remoteDelete` — **DONE**. +With the pull request refspec listed first, `git switch --track origin/pr/1` gives +`merge = refs/pull/1/head`. `remoteBranch()` then falls back to the tracking name, and the push +deletes `refs/heads/pr/1`, a teammate's branch. Fixed: `r` arms only when `upMerge` starts with +`refs/heads/`, and the refusal names the ref. Reproduced first with git 2.55. Covered by the +`pr/1` case in `TestRemoteDeleteRefusesWrongTargets`: `r` refuses, and the bare remote still has +`refs/heads/pr/1`. + +**20. A renaming refspec drops protection** *(regression from 17)* — `model.go` `isProtected` — +**DONE**. `mapRemoteRows` sets `upMerge`, so `remoteBranch()` of the row `origin/release/1.0` +was `jb/release/1.0`, and `release/*` no longer matched. The README says protect globs match +the name shown. Fixed: `protectsRemote` matches the globs against both the shown name and the +remote's name, for remote rows and for `remoteProtected`. Covered in +`TestRemoteRowsFollowFetchRefspecs`: the row `origin/release/1.0` is protected, and `r` refuses +on a local `hotfix` made from `origin/release/2.0`. + +**21. Only the default remote's trunk is protected** — `model.go` `protectsName` — **DONE**. +`upstream/master` next to `origin/main` was not protected. `TestRemoteRowsFollowFetchRefspecs` +passed only because `fork`'s trunk was the default remote. Fixed: `resolveBases` keeps the +target of each `refs/remotes//HEAD` in `m.remoteTrunks`, and `protectsRemote` protects those +refs in both checks. A remote with no `/HEAD` (added with `remote add`, never `set-head`) has +no known trunk. The test now gives `fork` the trunk `master` with `fork/HEAD` set, and gives +`origin` a `main` so `fork` is not the default remote. Item 17's text is corrected. + +**22. A repo path with `[`, `*` or `?` hides linked worktrees** — `git.go` `loadHeldBranches` — +**DONE**. `filepath.Glob` read the path as a pattern. Fixed: `os.ReadDir(/worktrees)`. +Covered: `TestHeldBranchesAreLocked` now runs in a folder named `a[1]`. + +**23. Left-over bisect state locks a branch git would delete** — `git.go` `readHeld` — **DONE**. +git's `wt_status_check_bisect` reads `BISECT_START` only when `BISECT_LOG` exists. Reproduced +with git 2.55: without `BISECT_LOG`, `git branch -D` succeeds. Fixed: the same check. +*(`TestLeftoverBisectStartDoesNotLock`)* + +**24. git older than 2.31 turns off held-branch checks with no error** — `loadHeldBranches` — +**DONE**. Old `rev-parse` prints an unknown `--path-format=absolute` back as output, so `common` +was wrong and every read missed. Checked with git 2.55: `rev-parse` echoes unknown flags, and +prints `--git-common-dir` relative to the current folder (`../../.git` from a subfolder), +or absolute from a linked worktree. Fixed: the flag is gone. `filepath.Abs` is not needed: the +reads resolve the relative path from the same folder. No test can fail without this fix on a +current git. `TestHeldBranchesAreLocked` now also reads the held branches from a subfolder and +from a linked worktree, which guards the relative path. + +**25. A stale refusal stays on screen** — `model.go`, the `r` case — **DONE**. +Fixed: `r` clears `m.status` when it toggles. Covered in `TestRemoteDeleteRefusesWrongTargets`: +`r` on a refused row, then on a good row, leaves the status empty. + +**26. The confirm screen hides a renamed remote target** — `view.go` `confirmParts` — **DONE**. +For the row `origin/old` mapped to `jb/old`, the confirm screen said only "remote branch — no +local copy", but the result line said `origin/jb/old`. Fixed: it adds +`delete remote /` when that differs from the row name. Covered in +`TestRemoteRowsFollowFetchRefspecs`: the confirm view contains `origin/jb/old`. + +**27. A test check that cannot fail** — `main_test.go`, end of `TestHeldBranchesAreLocked` — +**DONE**. `selectedBranches()` already drops locked rows. Fixed: after `a`, the test checks +`.selected` on each held branch. Mutation check passed: with the locked guard in `a` removed, +the test fails. + +**28. A remote name with `/` gives the wrong local name on `c`** — `model.go` `switchCmd` — +**DONE**. Remote `team/a`, row `team/a/x`: the local branch became `a/x`. Fixed: +`strings.TrimPrefix(b.name, b.upRemote+"/")`. *(`TestCheckoutRemoteWithSlashInName`)* + +### Design + +**29. `git switch` in a partial clone can show an ssh prompt** — `git.go` `networkBound` — +**DONE**. It stays without a timeout on purpose (see 18), so it got no `detachTTY` either. +Fixed: `networkBound` is now `networkUse`, which returns a `gitNet` with three flags: +`connects` (remote slot), `timeout` and `noTTY`. `switch` in a partial clone gets `noTTY` only. + +**30. Partial clones run risk counts 3 at a time, not 8** — `networkBound` — **DONE**. +`cherry` and `diff` took the remote slots even when the files were local. Fixed with 29: they +get `timeout` and `noTTY`, and keep the local slots. Both covered in +`TestPartialCloneRiskIsNotGuessed`. Mutation checks passed: `cherry` with `connects`, or +`switch` without `noTTY`, fails it. + +**31. `remoteDeletable` and `remoteRefusal` repeat the same checks** — `git.go` — **DONE**. +Fixed: one `remoteDelete() (ok bool, why string)`, done before 19, so 19 added its check once. + +**32. Risk counts can freeze the screen** — `model.go` `countRisk` callers — **DONE**. +They ran inside `Update`: in `d`, and in every reload (a fetch for `p`, the end of a delete +run, a switch, an undo, the remote view's load). `x` itself counted nothing. With the 60 s +network timeout from 18, a slow partial clone could block the screen for minutes. Fixed: +- Every reload installs the list and returns `mergeCmd`, the same `tea.Cmd` startup already + used. `installBranches` sets `m.merging`, so `x` and `d` wait until it lands. +- `p` sets `selectOnMerge`, and `selectAfterFetch` selects the gone branches once the counts + land. +- `d` calls `confirmAfterRisk`, which counts in a `tea.Cmd` and sets `m.measuring`. The + `riskMsg` opens the confirm screen. A count for an older load is dropped, with a status + line that says to press `d` again. +- Script mode and `startDeletions` keep the waiting `measureSelectedRisk`. In the TUI it + counts nothing, because `d` measured the selection first. + +Tests drive the new cmds with a `settle` helper in `press`, `drainDeletions`, `doSwitch` and +`openRemoteView`. *(`TestRiskCountsRunOffTheUpdateLoop`: no git process runs inside `Update` +for a reload or for `d`.)* Mutation checks passed: either path made synchronous again fails it. + +**33. `riskUnknown = -1` lives in a count field** — `git.go` — **DONE**. +Any new `> 0` or `== 0` test misread it. Fixed: `riskFailed` sits next to `riskMeasured`, and +`riskCommits` is a plain count. `riskCount{commits, failed}` carries both. Every "commits at +risk" check asks `atRisk()`, which is true for a failed count. Covered in +`TestPartialCloneRiskIsNotGuessed`. Mutation check passed: a plain `riskCommits == 0` on the +confirm screen shows a failed count as clean, and fails the test. + +### Found while fixing + +**34. A forced retry runs deletes and pushes inside `Update`** — `model.go` +`forceDeleteUnmerged` — **TODO**. `y` on the force prompt runs `git branch -D` and +`pushRemoteDelete` for each refused branch inside `Update`. A push can wait out the 60 s +timeout, so a slow remote freezes the screen, as in 32. Fix: run the retry as a `tea.Cmd`, like +`startDeletions`, and apply the results from a message. + ## Ref handling — the rule this work established **Never use git's `%(refname:short)`, `%(upstream:short)`, or `symbolic-ref --short`.** They @@ -259,17 +391,29 @@ open in `localDefaultBranch`. It has been deleted. ## Status -All three tiers are complete (2026-08-17): **14 tests added, 43 total**, passing under -`-race`, with `gofmt` and `go vet` clean. +Tiers 1 to 5 are done. One new item, 34, is open. + +The suite has **99 tests** (81 in `main_test.go`, 18 in `features_test.go`), passing under +`-race`, with `gofmt` and `go vet` clean (2026-10-10). Tiers 1 to 3 ended at 43; Tier 4 added 4; +Tier 5 added 3. -- Tier 1 fixed two real bugs — tag shadowing on the risk path, and `p` wiping selections. +- Tier 1 (2026-08-17) fixed two real bugs — tag shadowing on the risk path, and `p` wiping selections. - Tier 2 found no new bugs; it pins five repo environments that had no coverage, and corrected two wrong predictions in this document against real git behavior. - 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. It is -6 now, in two concurrent rounds rather than six serial ones (see item 18 in `improvements.md`). + probe, and brought startup back from 11 git subprocesses to 7 — the pre-change baseline. + Item 18 in `improvements.md` then took it to 6, in two concurrent rounds rather than six + serial ones. +- Tier 4 (2026-10-09) fixed four bugs where git state outside the branch list changed what a + delete does. It is in `c4846f2`, merged in PR #7. + +Startup is now 9 git subprocesses in `setupRepo`'s shape, still in two rounds. Later work added +three reads to the first round: the config, the worktree list and the held branches. The first +round is the repo check plus the six reads in `loadRepo`. The second is the two merge queries +against the base. Gone branches add their risk counts on top. `TestMergedBranchesCostNoSubprocess` +caps a load at 12. Fixtures compose rather than duplicate: `initRepo` (bare init + identity) → `setupLocalRepo` (branch shapes) → `setupRepo` (+ `addOrigin` + a tracking branch), with diff --git a/features_test.go b/features_test.go index 4a81865..65b8772 100644 --- a/features_test.go +++ b/features_test.go @@ -100,8 +100,8 @@ func TestUndoRestoresDeletedBranches(t *testing.T) { if cmd == nil { t.Fatal("u must start the restore") } - nm, _ = m.Update(cmd()) - m = nm.(model) + nm, next := m.Update(cmd()) + m = settle(m, nm.(model), next) if got := git(t, repo, "rev-parse", "refs/heads/feature/tracked"); got != sha { t.Fatalf("restored at %s, want %s", got, sha) } @@ -403,8 +403,8 @@ func TestRemoteOnlyView(t *testing.T) { if cmd == nil || !m.showRemote { t.Fatal("tab must switch view and load the remote rows") } - nm, _ = m.Update(cmd()) - m = nm.(model) + nm, next := m.Update(cmd()) + m = settle(m, nm.(model), next) var names []string for _, i := range m.viewIdx() { names = append(names, m.branches[i].name) @@ -446,9 +446,7 @@ func TestCheckoutRemoteOnly(t *testing.T) { if err != nil { t.Fatal(err) } - nm, cmd := m.Update(special(tea.KeyTab)) - nm, _ = nm.(model).Update(cmd()) - m = nm.(model) + m = openRemoteView(t, m) cursorTo(t, &m, "origin/orphan") m = doSwitch(t, m) if m.err != "" || currentBranch(t, repo) != "orphan" { @@ -462,6 +460,28 @@ func TestCheckoutRemoteOnly(t *testing.T) { } } +// A remote's name can hold a slash. The local copy of team/a/x is x, as git's +// own checkout names it, not a/x. +func TestCheckoutRemoteWithSlashInName(t *testing.T) { + repo := initRepo(t, "main") + commitFile(t, repo, "a", "a") + remote := t.TempDir() + git(t, remote, "init", "--bare", "-q", "-b", "main") + git(t, repo, "remote", "add", "team/a", remote) + git(t, repo, "push", "-q", "team/a", "main:x") + chdir(t, repo) + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + m = openRemoteView(t, m) + cursorTo(t, &m, "team/a/x") + m = doSwitch(t, m) + if m.err != "" || currentBranch(t, repo) != "x" { + t.Fatalf("c must make x, got %q (err %q)", currentBranch(t, repo), m.err) + } +} + // Script mode lists without --yes, deletes with it, and keeps the same safety // rules as the TUI. func TestScriptMode(t *testing.T) { diff --git a/git.go b/git.go index 2a06d0f..953b56a 100644 --- a/git.go +++ b/git.go @@ -35,8 +35,9 @@ type branch struct { gone bool // upstream was configured but no longer exists remoteMerged bool // upstream is merged into the remote default branch (safe to delete) headMerged bool // branch tip is merged into HEAD (git's -d criterion when there is no upstream) - riskCommits int // commits whose patch is not in the base branch; -D discards them (riskUnknown: not countable) + riskCommits int // commits whose patch is not in the base branch; -D discards them riskMeasured bool // riskCommits has been computed (0 is a meaningful value) + riskFailed bool // git cherry failed, so the count is unknown and riskCommits says nothing isCurrent bool worktree string // path of another worktree that has the branch checked out; "" when none does mainWorktree bool // that worktree is the main one, which git cannot remove @@ -54,30 +55,40 @@ type branch struct { // a remoteOnly row arms it: deleting the remote branch is all it offers. func (b branch) remoteArmed() bool { return (b.deleteRemote || b.remoteOnly) && b.upstream != "" } +// setRisk records b's measured cost of deletion. +func (b *branch) setRisk(r riskCount) { + b.riskCommits, b.riskFailed, b.riskMeasured = r.commits, r.failed, true +} + +// atRisk reports whether -D can discard commits of b: some are not in the base, +// or git cherry could not tell. Every check for commits at risk asks this, so an +// unknown count is never read as clean. +func (b branch) atRisk() bool { return b.riskFailed || b.riskCommits > 0 } + // locked reports whether b can never be marked for deletion. A branch in a // linked worktree is not: the delete removes the worktree first. The main // worktree cannot be removed, so its branch stays locked. func (b branch) locked() bool { return b.isCurrent || b.mainWorktree || b.heldBy != "" || b.protected } -// remoteDeletable reports whether r may arm the delete of b's remote branch. An +// remoteDelete reports whether r may arm the delete of b's remote branch. why +// says what stops it, and is "" when the row already shows the reason. An // upstream that is a local branch (remote ".") has no remote branch at all, and -// remoteName would guess a remote b never used. -func (b branch) remoteDeletable() bool { - return b.upstream != "" && b.upRemote != "." && !b.remoteOnly && !b.locked() && !b.remoteProtected -} - -// remoteRefusal says why r cannot arm b's remote delete, for the cases the row -// does not already show. -func (b branch) remoteRefusal() string { +// remoteName would guess a remote b never used. An upstream outside refs/heads/, +// such as a pull request head, is not a branch either, and remoteBranch would +// fall back to the tracking name: origin/pr/1 would push the delete of a +// teammate's pr/1. +func (b branch) remoteDelete() (ok bool, why string) { switch { case b.remoteOnly || b.locked() || b.upstream == "": - return "" + return false, "" case b.upRemote == ".": - return b.name + " tracks local branch " + b.upstream + ": there is no remote branch to delete" + return false, b.name + " tracks local branch " + b.upstream + ": there is no remote branch to delete" + case !strings.HasPrefix(b.upMerge, "refs/heads/"): + return false, b.name + " tracks " + b.upMerge + " on " + b.remoteName() + ", which is not a branch: there is no remote branch to delete" case b.remoteProtected: - return "remote branch " + b.remoteName() + "/" + b.remoteBranch() + " is protected" + return false, "remote branch " + b.remoteName() + "/" + b.remoteBranch() + " is protected" } - return "" + return true, "" } // ref is b's fully qualified ref (see branchRef). @@ -100,6 +111,11 @@ func (b branch) remoteName() string { return "origin" } +// shownBranch is the branch part of b's tracking name: origin/release/1.0 gives +// release/1.0. A renaming fetch refspec can make it differ from remoteBranch. +// The remote's name can hold a slash, so it is trimmed whole. +func (b branch) shownBranch() string { return strings.TrimPrefix(b.upstream, b.remoteName()+"/") } + func (b branch) remoteBranch() string { if s, ok := strings.CutPrefix(b.upMerge, "refs/heads/"); ok { return s @@ -152,22 +168,36 @@ var ( 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 { +// gitNet says how one git invocation may use the network. +type gitNet struct { + connects bool // opens a connection on every call: takes a remote slot + timeout bool // can wait on a remote: bounded by netTimeout + noTTY bool // can reach a remote, where ssh may ask for a passphrase: run with no terminal +} + +// networkUse reports how a git invocation may use the network. It is what picks +// the cap, the timeout and the terminal, so a new network subcommand belongs +// here rather than at its call site. +func networkUse(args []string) gitNet { if len(args) == 0 { - return false + return gitNet{} } + lazy := lazyFetch.Load() switch args[0] { case "push", "fetch": - return true + return gitNet{connects: true, timeout: true, noTTY: true} case "cherry", "diff": // Both read file contents, which a partial clone fetches on demand. - // Killing either on the timeout is safe: they change nothing. - return lazyFetch.Load() + // Killing either on the timeout is safe: they change nothing. They keep + // the local cap: it already bounds them, and the remote cap would run + // a risk count 3 at a time. + return gitNet{timeout: lazy, noTTY: lazy} + case "switch": + // A checkout in a partial clone fetches too, but gets no timeout: + // killing it partway leaves index.lock behind. + return gitNet{noTTY: lazy} } - return false + return gitNet{} } // lazyFetch records that the repository is a partial clone (git clone @@ -204,9 +234,9 @@ var gitProcs, netProcs gauge // 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) + use := networkUse(args) slots := localSlots - if net { + if use.connects { slots = remoteSlots } slots <- struct{}{} @@ -216,13 +246,13 @@ func runGit(args ...string) (string, error) { // rather than what is queued. gitProcs.enter() defer gitProcs.leave() - if net { + if use.connects { netProcs.enter() defer netProcs.leave() } ctx := context.Background() - if net { + if use.timeout { // A remote that stops answering would otherwise hold the slot, and the // screen waiting on it, forever. var cancel context.CancelFunc @@ -236,7 +266,7 @@ func runGit(args ...string) (string, error) { // GIT_TERMINAL_PROMPT=0 makes git fail instead of asking for a password: the // TUI owns the terminal, so a prompt there can never be answered. cmd.Env = append(os.Environ(), "LC_ALL=C", "GIT_TERMINAL_PROMPT=0") - if net { + if use.noTTY { // ssh asks for passphrases on /dev/tty, past GIT_TERMINAL_PROMPT. With no // controlling terminal it fails fast instead of fighting the TUI for input. detachTTY(cmd) @@ -495,22 +525,26 @@ func baseBranch(name string) string { // squashed as a group still count, since no equivalent single patch exists; // the warning is therefore worded as "not in ", not "will be lost". // Returns 0 when there is nothing to compare against. -func riskCommitCount(name, base string) int { return riskCommitCountRef(branchRef(name), base) } +func riskCommitCount(name, base string) riskCount { return riskCommitCountRef(branchRef(name), base) } -// riskUnknown is the count of a branch git cherry could not measure. A partial -// clone that cannot reach its remote fails there, and reading the failure as 0 -// would let x and p select a branch whose commits -D then discards. -const riskUnknown = -1 +// riskCount is one branch's measured cost of deletion. +type riskCount struct { + commits int + // failed records that git cherry failed. A partial clone that cannot reach + // its remote fails there, and reading the failure as 0 would let x and p + // select a branch whose commits -D then discards. + failed bool +} // riskCommitCountRef is riskCommitCount for any fully qualified ref, which is // what lets a remote branch be measured the same way. -func riskCommitCountRef(ref, base string) int { +func riskCommitCountRef(ref, base string) riskCount { if base == "" || base == ref { - return 0 + return riskCount{} } out, err := runGit("cherry", base, ref) if err != nil { - return riskUnknown + return riskCount{failed: true} } n := 0 for _, line := range strings.Split(out, "\n") { @@ -518,7 +552,7 @@ func riskCommitCountRef(ref, base string) int { n++ } } - return n + return riskCount{commits: n} } // mergedSet runs a `git branch --merged` query and collects the short ref names @@ -719,14 +753,22 @@ func mainWorktreePath() string { // worktree keeps this state in its own git dir: the common dir for the main // worktree, and worktrees/ under it for each linked one. func loadHeldBranches() map[string]string { - out, err := runGit("rev-parse", "--path-format=absolute", "--git-common-dir") + // The path may be relative to the current folder, which the reads below + // resolve it from too. --path-format=absolute would need git 2.31; older git + // prints the unknown flag back as output. + out, err := runGit("rev-parse", "--git-common-dir") if err != nil { return nil } common := strings.TrimSpace(out) - linked, _ := filepath.Glob(filepath.Join(common, "worktrees", "*")) + dirs := []string{common} + // Listed, not globbed: a repo path can hold [, * or ?. + linked, _ := os.ReadDir(filepath.Join(common, "worktrees")) + for _, e := range linked { + dirs = append(dirs, filepath.Join(common, "worktrees", e.Name())) + } held := map[string]string{} - for _, dir := range append([]string{common}, linked...) { + for _, dir := range dirs { readHeld(dir, held) } return held @@ -754,7 +796,12 @@ func readHeld(dir string, held map[string]string) { } } // BISECT_START names the branch bisect started from, or holds a commit ID - // when it started from a detached HEAD. + // when it started from a detached HEAD. Like git (wt_status_check_bisect), + // trust it only while BISECT_LOG exists: a bisect cleaned up by hand can + // leave it behind. + if _, err := os.Stat(filepath.Join(dir, "BISECT_LOG")); err != nil { + return + } if start := read("BISECT_START"); start != "" && !isObjectID(start) { held[branchRef(start)] = "bisect" } diff --git a/main_test.go b/main_test.go index 6001305..9a31cba 100644 --- a/main_test.go +++ b/main_test.go @@ -64,8 +64,8 @@ func deleteMsgs(t *testing.T, cmd tea.Cmd) []branchDeletedMsg { 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) + nm, next := m.Update(dm) + m = settle(m, nm.(model), next) } return m } @@ -124,11 +124,16 @@ func git(t *testing.T, dir string, args ...string) string { // identity configured. Callers add the commits, remotes and branches they need. func initRepo(t *testing.T, branch string) string { t.Helper() - tmp := t.TempDir() - git(t, tmp, "init", "-q", "-b", branch) - git(t, tmp, "config", "user.email", "t@t.t") - git(t, tmp, "config", "user.name", "t") - return tmp + return initRepoIn(t, t.TempDir(), branch) +} + +// initRepoIn is initRepo in a folder of the caller's choice, which git makes. +func initRepoIn(t *testing.T, dir, branch string) string { + t.Helper() + git(t, filepath.Dir(dir), "init", "-q", "-b", branch, dir) + git(t, dir, "config", "user.email", "t@t.t") + git(t, dir, "config", "user.name", "t") + return dir } // addOrigin gives dir a bare origin remote and pushes trunk to it. @@ -1019,8 +1024,8 @@ func TestGoneBranchWithUnpushedCommitsIsGuarded(t *testing.T) { for i := range m.branches { m.branches[i].selected = false } - nm, _ := m.Update(fetchDoneMsg{}) - after := nm.(model) + nm, cmd := m.Update(fetchDoneMsg{}) + after := settle(m, nm.(model), cmd) if b := find(after.branches, "feature/tracked"); b == nil || b.selected { t.Fatalf("gone branch holding unique commits must not be auto-selected: %+v", b) } @@ -1054,8 +1059,8 @@ func TestGoneMergedBranchStillAutoSelected(t *testing.T) { if w := m.riskWarning(*b); w != "" { t.Fatalf("a gone branch holding nothing unique must not warn: %q", w) } - nm, _ := m.Update(fetchDoneMsg{}) - after := nm.(model) + nm, cmd := m.Update(fetchDoneMsg{}) + after := settle(m, nm.(model), cmd) if b := find(after.branches, "feature/tracked"); b == nil || !b.selected { t.Fatalf("risk-free gone branch should still be auto-selected: %+v", b) } @@ -1075,14 +1080,14 @@ func TestRiskCommitCountIgnoresCherryPicked(t *testing.T) { git(t, repo, "checkout", "-q", "main") git(t, repo, "cherry-pick", "picked") - if n := riskCommitCount("picked", "main"); n != 0 { + if n := riskCommitCount("picked", "main").commits; n != 0 { t.Fatalf("cherry-picked commit should not count as at risk, got %d", n) } - if n := riskCommitCount("feature/unmerged", "main"); n != 1 { + if n := riskCommitCount("feature/unmerged", "main").commits; n != 1 { t.Fatalf("genuinely unmerged commit should count, got %d", n) } // No base to compare against means nothing can be asserted about risk. - if n := riskCommitCount("picked", ""); n != 0 { + if n := riskCommitCount("picked", "").commits; n != 0 { t.Fatalf("empty base should yield 0, got %d", n) } } @@ -1248,8 +1253,7 @@ func TestUnmergedBranchWithoutUpstreamIsWarned(t *testing.T) { // Drive the real path: 'd' measures the selection's risk, then confirms. b.selected = true - nm, _ := m.updateList(key("d")) - m = nm.(model) + m = press(t, m, key("d")) wantState(t, m, stateConfirm, "'d' should open the confirm screen") b = find(m.branches, "feature/unmerged") if b.riskCommits != 1 { @@ -1429,7 +1433,7 @@ func TestTagShadowingBranchName(t *testing.T) { // Tag the base commit with the name of a branch that is one commit ahead. git(t, repo, "tag", "feature/unmerged", "main") - if n := riskCommitCount("feature/unmerged", "origin/main"); n != 1 { + if n := riskCommitCount("feature/unmerged", "origin/main").commits; n != 1 { t.Fatalf("want the branch's 1 unique commit measured, got %d (the tag was measured)", n) } diff, _, err := loadDiff("feature/unmerged") @@ -1454,8 +1458,7 @@ func TestTagShadowingBranchName(t *testing.T) { // End to end: the confirm screen must still state what the delete costs. b.selected = true - nm, _ := m.updateList(key("d")) - m = nm.(model) + m = press(t, m, key("d")) out := stripANSI(m.confirmView()) if !strings.Contains(out, fmt.Sprintf("1 commit(s) not in %s", m.riskBase)) { t.Fatalf("confirm view must state the shadowed branch's cost:\n%s", out) @@ -1584,7 +1587,7 @@ func TestTagShadowingBranchName(t *testing.T) { if m.riskBase != "origin/main" { t.Fatalf("precondition: base should be origin/main, got %q", m.riskBase) } - if n := riskCommitCount("feature/unmerged", m.riskBaseRef); n != 1 { + if n := riskCommitCount("feature/unmerged", m.riskBaseRef).commits; n != 1 { t.Fatalf("want 1 commit at risk, got %d (the tag was used as the base)", n) } }) @@ -1605,8 +1608,8 @@ func TestFetchPreservesSelections(t *testing.T) { tb.selected = true tb.deleteRemote = true - nm, _ := m.Update(fetchPruneCmd()()) - after := nm.(model) + nm, cmd := m.Update(fetchPruneCmd()()) + after := settle(m, nm.(model), cmd) if b := find(after.branches, "feature/unmerged"); b == nil || !b.selected { t.Fatalf("a manual selection must survive a fetch: %+v", b) @@ -1637,8 +1640,8 @@ func TestFetchDisarmsRemoteForGoneBranch(t *testing.T) { // Someone else deletes the remote branch before this fetch lands. git(t, repo, "push", "-q", "origin", "--delete", "feature/tracked") - nm, _ := m.Update(fetchPruneCmd()()) - after := nm.(model) + nm, cmd := m.Update(fetchPruneCmd()()) + after := settle(m, nm.(model), cmd) b := find(after.branches, "feature/tracked") if b == nil || !b.gone { @@ -1666,8 +1669,8 @@ func TestGoneCurrentBranchIsNotPruned(t *testing.T) { if err != nil { t.Fatal(err) } - nm, _ := m.Update(fetchPruneCmd()()) - m = nm.(model) + nm, cmd := m.Update(fetchPruneCmd()()) + m = settle(m, nm.(model), cmd) b := find(m.branches, "feature/tracked") if b == nil || !b.gone || !b.isCurrent { @@ -1746,8 +1749,8 @@ func TestLocalOnlyRepo(t *testing.T) { } // `p` is a harmless no-op rather than an error. - nm, _ := m.Update(fetchPruneCmd()()) - m = nm.(model) + nm, cmd := m.Update(fetchPruneCmd()()) + m = settle(m, nm.(model), cmd) if m.err != "" { t.Fatalf("fetch must succeed with no remotes: %q", m.err) } @@ -2438,12 +2441,37 @@ func special(k tea.KeyType) tea.KeyMsg { return tea.KeyMsg{Type: k} } func press(t *testing.T, m model, keys ...tea.KeyMsg) model { t.Helper() for _, k := range keys { - nm, _ := m.Update(k) + nm, cmd := m.Update(k) + m = settle(m, nm.(model), cmd) + } + return m +} + +// settle feeds back the message of a cmd the model started for itself, as the +// tea runtime would: the merge queries a reload starts, and the risk count d +// starts. was is the model before the Update that returned cmd, so work it +// already had running stays the test's to drive. +func settle(was, m model, cmd tea.Cmd) model { + if cmd != nil && (m.merging && !was.merging || m.measuring && !was.measuring) { + nm, _ := m.Update(cmd()) m = nm.(model) } return m } +// openRemoteView presses tab and lands the remote rows' load and the merge +// queries that follow it. +func openRemoteView(t *testing.T, m model) model { + t.Helper() + nm, cmd := m.Update(special(tea.KeyTab)) + m = nm.(model) + if cmd == nil { + t.Fatal("tab must load the remote rows") + } + nm, next := m.Update(cmd()) + return settle(m, nm.(model), next) +} + // cursorTo puts the cursor on the named branch's view row, failing if the // branch is not visible. func cursorTo(t *testing.T, m *model, name string) { @@ -2463,8 +2491,8 @@ func doSwitch(t *testing.T, m model) model { if cmd == nil { return m // guarded no-op: nothing was started } - nm, _ = m.Update(cmd()) - return nm.(model) + nm, next := m.Update(cmd()) + return settle(m, nm.(model), next) } // currentBranch reports the checked-out branch of dir. @@ -3003,9 +3031,10 @@ func gitStops(t *testing.T, dir string, args ...string) { // A rebase or bisect holds the branch it started from, and a rebase // --update-refs holds every branch in its stack. git refuses to delete them in // any worktree, under -d and -D alike, yet the worktree's HEAD is detached, so -// neither %(HEAD) nor %(worktreepath) reports them. +// neither %(HEAD) nor %(worktreepath) reports them. The repo's folder name holds +// glob characters, which must not hide its linked worktrees. func TestHeldBranchesAreLocked(t *testing.T) { - repo := initRepo(t, "main") + repo := initRepoIn(t, filepath.Join(t.TempDir(), "a[1]"), "main") commitFile(t, repo, "f", "a") for _, name := range []string{"stack/base", "stack/top", "bisected", "applied"} { git(t, repo, "branch", name) @@ -3057,28 +3086,70 @@ func TestHeldBranchesAreLocked(t *testing.T) { t.Fatalf("the row must show the ~ marker and the reason: %q", row) } } + // Read .selected itself: selectedBranches already drops locked rows, so it + // cannot show a held branch that a marked. m = press(t, m, key("a")) - for _, b := range m.selectedBranches() { - if want[b.name] != "" { - t.Fatalf("a must skip held branch %s", b.name) + for name := range want { + if find(m.branches, name).selected { + t.Fatalf("a must skip held branch %s", name) } } + + // git prints the common dir relative to the current folder, or absolute from + // a linked worktree, and the reads resolve it from that same folder. + sub := filepath.Join(repo, "sub") + if err := os.Mkdir(sub, 0o755); err != nil { + t.Fatal(err) + } + for _, dir := range []string{sub, bisectWT} { + chdir(t, dir) + if held := loadHeldBranches(); len(held) != len(want) { + t.Fatalf("from %s: held %v, want the %d branches", dir, held, len(want)) + } + } +} + +// A bisect cleaned up by hand can leave BISECT_START behind. git reads it only +// while BISECT_LOG exists, so the branch it names is free to delete. +func TestLeftoverBisectStartDoesNotLock(t *testing.T) { + repo := initRepo(t, "main") + commitFile(t, repo, "f", "a") + git(t, repo, "branch", "stale") + if err := os.WriteFile(filepath.Join(repo, ".git", "BISECT_START"), []byte("stale\n"), 0o644); err != nil { + t.Fatal(err) + } + chdir(t, repo) + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + if b := find(m.branches, "stale"); b == nil || b.heldBy != "" || b.locked() { + t.Fatalf("stale must not be held: %+v", b) + } + git(t, repo, "branch", "-D", "stale") // ground truth: git agrees } // r deletes the remote branch a branch tracks, which need not share its name. // A branch made from origin/main tracks main, one made from a protected branch // tracks that branch, and one made with --track main tracks no remote branch at -// all. None of these may be armed. +// all. A branch made from a pull request tracks refs/pull/1/head, which is not a +// branch: a delete pushed by its tracking name would remove a teammate's pr/1. +// None of these may be armed. func TestRemoteDeleteRefusesWrongTargets(t *testing.T) { repo := initRepo(t, "main") commitFile(t, repo, "a", "a") addOrigin(t, repo, "main") - git(t, repo, "push", "-q", "origin", "main:release/1.0") + git(t, repo, "push", "-q", "origin", "main:release/1.0", "main:pr/1", "main:refs/pull/1/head") git(t, repo, "fetch", "-q") git(t, repo, "config", "pruner.protect", "release/*") git(t, repo, "branch", "hotfix", "origin/release/1.0") git(t, repo, "branch", "feat", "origin/main") git(t, repo, "branch", "--track", "stacked", "main") + // With the pull request refspec first, git maps origin/pr/1 back to the pull + // request, not to the branch pr/1. + git(t, repo, "config", "--replace-all", "remote.origin.fetch", "+refs/pull/*/head:refs/remotes/origin/pr/*") + git(t, repo, "config", "--add", "remote.origin.fetch", "+refs/heads/*:refs/remotes/origin/*") + git(t, repo, "branch", "pr/1", "origin/pr/1") git(t, repo, "switch", "-q", "-c", "feature/ok") git(t, repo, "push", "-q", "-u", "origin", "feature/ok") git(t, repo, "switch", "-q", "main") @@ -3092,6 +3163,7 @@ func TestRemoteDeleteRefusesWrongTargets(t *testing.T) { "hotfix": "remote branch origin/release/1.0 is protected", "feat": "remote branch origin/main is protected", "stacked": "stacked tracks local branch main", + "pr/1": "pr/1 tracks refs/pull/1/head on origin", } { cursorTo(t, &m, name) m.status = "" @@ -3107,7 +3179,11 @@ func TestRemoteDeleteRefusesWrongTargets(t *testing.T) { if m = press(t, m, key("r")); !find(m.branches, "feature/ok").deleteRemote { t.Fatal("r must still arm a branch whose remote branch is its own") } - if !remoteHasBranch(t, repo, "refs/heads/release/1.0") || !remoteHasBranch(t, repo, "refs/heads/main") { + if m.status != "" { + t.Fatalf("the last row's refusal must not stay on screen once r arms another: %q", m.status) + } + if !remoteHasBranch(t, repo, "refs/heads/release/1.0") || !remoteHasBranch(t, repo, "refs/heads/main") || + !remoteHasBranch(t, repo, "refs/heads/pr/1") { t.Fatal("no remote branch should have been touched") } } @@ -3116,33 +3192,44 @@ func TestRemoteDeleteRefusesWrongTargets(t *testing.T) { // fetch refspecs: with refs/heads/jb/*:refs/remotes/origin/*, origin/old is // jb/old, and a teammate's branch named old must survive its delete. Refs // fetched from outside refs/heads/ are not branches, so they are not listed. -// Every remote's trunk is protected, not only the default remote's. +// A protect glob matches the name a row shows, which here is not the name on the +// remote. Each remote's trunk, as its /HEAD names it, is protected, not +// only the default remote's. func TestRemoteRowsFollowFetchRefspecs(t *testing.T) { repo := initRepo(t, "main") commitFile(t, repo, "a", "a") addOrigin(t, repo, "main") - git(t, repo, "push", "-q", "origin", "main:jb/old", "main:old", "main:refs/pull/1/head") + git(t, repo, "push", "-q", "origin", "main:jb/old", "main:old", "main:refs/pull/1/head", + "main:jb/main", "main:jb/release/1.0", "main:jb/release/2.0") git(t, repo, "config", "--replace-all", "remote.origin.fetch", "+refs/heads/jb/*:refs/remotes/origin/*") git(t, repo, "config", "--add", "remote.origin.fetch", "+refs/pull/*/head:refs/remotes/origin/pr/*") + git(t, repo, "config", "pruner.protect", "release/*") git(t, repo, "fetch", "-q", "--prune", "origin") + git(t, repo, "branch", "hotfix", "origin/release/2.0") fork := t.TempDir() - git(t, fork, "init", "--bare", "-q", "-b", "main") + git(t, fork, "init", "--bare", "-q", "-b", "master") git(t, repo, "remote", "add", "fork", fork) - git(t, repo, "push", "-q", "fork", "main", "main:topic") + git(t, repo, "push", "-q", "fork", "main:master", "main:topic") git(t, repo, "fetch", "-q", "fork") + git(t, repo, "remote", "set-head", "fork", "master") chdir(t, repo) m, err := initialModel() if err != nil { t.Fatal(err) } - nm, cmd := m.Update(special(tea.KeyTab)) - nm, _ = nm.(model).Update(cmd()) - m = nm.(model) + cursorTo(t, &m, "hotfix") + if m = press(t, m, key("r")); find(m.branches, "hotfix").deleteRemote { + t.Fatal("r must refuse: origin/release/2.0 matches release/* by the name it shows") + } + m = openRemoteView(t, m) if find(m.branches, "origin/pr/1") != nil { t.Fatal("a pull request ref is not a remote branch and must not be listed") } - if b := find(m.branches, "fork/main"); b == nil || !b.protected { + if b := find(m.branches, "origin/release/1.0"); b == nil || !b.protected { + t.Fatalf("release/* must protect the row by the name it shows: %+v", b) + } + if b := find(m.branches, "fork/master"); b == nil || !b.protected { t.Fatalf("another remote's trunk must be protected: %+v", b) } if b := find(m.branches, "fork/topic"); b == nil || b.locked() || b.remoteBranch() != "topic" { @@ -3156,7 +3243,10 @@ func TestRemoteRowsFollowFetchRefspecs(t *testing.T) { cursorTo(t, &m, "origin/old") m = press(t, m, key(" "), key("d")) wantState(t, m, stateConfirm, "d opens the confirm screen") - nm, cmd = m.Update(key("R")) + if v := stripANSI(m.confirmView()); !strings.Contains(v, "origin/jb/old") { + t.Fatalf("the confirm screen must name the branch the push deletes:\n%s", v) + } + nm, cmd := m.Update(key("R")) m = drainDeletions(t, nm.(model), cmd) if r := result(t, m, "origin/old"); !r.remoteOK { t.Fatalf("the delete should succeed: %+v", r) @@ -3196,11 +3286,22 @@ func TestPartialCloneRiskIsNotGuessed(t *testing.T) { if err != nil { t.Fatal(err) } - if !networkBound([]string{"cherry"}) || !networkBound([]string{"diff"}) { - t.Fatal("cherry and diff can reach the remote in a partial clone") + // cherry and diff can reach the remote here, so they get the timeout and no + // terminal, but they keep the local cap. switch can reach it too, but a + // timeout would kill a checkout partway. + for _, cmd := range []string{"cherry", "diff"} { + if use := networkUse([]string{cmd}); use != (gitNet{timeout: true, noTTY: true}) { + t.Fatalf("%s in a partial clone: %+v", cmd, use) + } + } + if use := networkUse([]string{"switch"}); use != (gitNet{noTTY: true}) { + t.Fatalf("switch in a partial clone: %+v", use) + } + if use := networkUse([]string{"push"}); use != (gitNet{connects: true, timeout: true, noTTY: true}) { + t.Fatalf("push: %+v", use) } b := find(m.branches, "feat") - if !b.gone || !b.riskMeasured || b.riskCommits != riskUnknown { + if !b.gone || !b.riskMeasured || !b.riskFailed || b.riskCommits != 0 { t.Fatalf("want a gone branch with an unknown risk count: %+v", b) } if status := m.selectGone(); find(m.branches, "feat").selected || !strings.Contains(status, "1 could not be checked") { @@ -3210,9 +3311,58 @@ func TestPartialCloneRiskIsNotGuessed(t *testing.T) { if w := m.riskWarning(*b); !strings.Contains(w, "an unknown number of commits") { t.Fatalf("the confirm screen must say the count is unknown: %q", w) } + // The count is 0 when git cherry fails, which must not read as clean. + if v := stripANSI(m.confirmView()); strings.Contains(v, "no commits missing") { + t.Fatalf("a failed count must not show as clean:\n%s", v) + } lazyFetch.Store(false) - if networkBound([]string{"cherry"}) { - t.Fatal("outside a partial clone, cherry is local") + for _, cmd := range []string{"cherry", "diff", "switch"} { + if use := networkUse([]string{cmd}); use != (gitNet{}) { + t.Fatalf("outside a partial clone, %s is local: %+v", cmd, use) + } + } +} + +// git cherry can reach the network in a partial clone, where each count may +// wait out netTimeout. So no risk count runs inside Update: a reload installs +// the new list before its merge queries, gone-branch counts and all, finish, +// and d opens the confirm screen when its counts land. +func TestRiskCountsRunOffTheUpdateLoop(t *testing.T) { + chdir(t, setupRepo(t)) + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + + branches, reads, err := loadRepo(false) + if err != nil { + t.Fatal(err) + } + before := gitProcs.total.Load() + nm, cmd := m.Update(repoLoadedMsg{branches: branches, reads: reads}) + m = nm.(model) + if n := gitProcs.total.Load() - before; n != 0 || cmd == nil || !m.merging { + t.Fatalf("a reload must run its merge queries off the update loop: %d git processes ran inside Update", n) + } + nm, _ = m.Update(cmd()) + if m = nm.(model); m.merging { + t.Fatal("the merge queries must land") + } + + cursorTo(t, &m, "feature/unmerged") + m = press(t, m, key(" ")) + before = gitProcs.total.Load() + nm, cmd = m.Update(key("d")) + m = nm.(model) + if n := gitProcs.total.Load() - before; n != 0 || cmd == nil || !m.measuring { + t.Fatalf("d must count off the update loop: %d git processes ran inside Update", n) + } + wantState(t, m, stateList, "the confirm screen waits for the counts") + nm, _ = m.Update(cmd()) + m = nm.(model) + wantState(t, m, stateConfirm, "the confirm screen opens when the counts land") + if b := find(m.branches, "feature/unmerged"); !b.riskMeasured || b.riskCommits != 1 || m.measuring { + t.Fatalf("the count must land: %+v", b) } } diff --git a/model.go b/model.go index cae13c9..4f07324 100644 --- a/model.go +++ b/model.go @@ -105,15 +105,24 @@ type model struct { remoteDefaultRef string // remoteDefault fully qualified riskBase string // ref that branch.riskCommits is measured against ("" if unresolved) riskBaseRef string // riskBase fully qualified, so a same-named tag cannot shadow it + // remoteTrunks holds the ref each /HEAD names. remoteDefault is only + // the first remote's trunk. + remoteTrunks map[string]bool // 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 - // merging is set while the startup merge queries run in the background. - // Until they land, ✓ marks and risk counts are unknown, so the keys that - // select by them wait. + // merging is set while a load's merge queries run in the background (see + // mergeCmd). Until they land, ✓ marks and risk counts are unknown, so the + // keys that select by them wait. merging bool loadGen int // bumped per branch load, so a late merge result for an old list is dropped + // selectOnMerge is set by a finished fetch: p selects the gone branches + // once the merge queries have counted what each one holds. + selectOnMerge bool + // measuring is set while d counts what the selected deletes cost (see + // confirmAfterRisk). The confirm screen opens when the counts land. + measuring bool spinnerFrame int // animation frame for the deleting spinner (deletion counts derive from results) @@ -170,8 +179,7 @@ func switchCmd(b branch, withRemote bool) tea.Cmd { if b.remoteOnly { // Name the local copy after the tracking ref, as git's own checkout // does: a fetch refspec can give the remote's branch another name. - _, local, _ := strings.Cut(b.name, "/") - msg.name, msg.fromRemote = local, true + msg.name, msg.fromRemote = b.shownBranch(), true args = []string{"switch", "-c", msg.name, "--track", b.ref()} } return func() tea.Msg { @@ -202,17 +210,24 @@ func reloadCmd(withRemote bool) tea.Cmd { // mergeInfo is the result of the merge queries (see queryMergeInfo). type mergeInfo struct { - remoteMerged map[string]bool // remote-tracking names merged into the remote default - baseMerged map[string]bool // local branches whose tip is in the risk base - risk map[string]int // commits not in the base, by ref, for gone branches + remoteMerged map[string]bool // remote-tracking names merged into the remote default + baseMerged map[string]bool // local branches whose tip is in the risk base + risk map[string]riskCount // commits not in the base, by ref, for gone branches } -// mergeInfoMsg carries the startup merge queries; gen ties it to one load. +// mergeInfoMsg carries a load's merge queries; gen ties it to one load. type mergeInfoMsg struct { gen int info mergeInfo } +// riskMsg carries the risk counts d started; gen ties them to one load. +type riskMsg struct { + gen int + refs []string + counts []riskCount +} + // diffLoadedMsg carries a diff made in the background; seq ties it to one request. type diffLoadedMsg struct { seq int @@ -314,7 +329,6 @@ func startupModel() (model, error) { m := model{field: sortDate, ascending: false, height: 24, width: 100} loadSettings(&m) // a no-op unless main set settingsPath m.installBranches(branches, reads) - m.merging = true return m, nil } @@ -456,6 +470,13 @@ func (m model) Init() tea.Cmd { if !m.merging { return nil } + return m.mergeCmd() +} + +// mergeCmd runs the merge queries for the loaded list off the update loop. Their +// risk counts can reach the network in a partial clone (see networkUse), and +// the screen must keep drawing while they wait. +func (m model) mergeCmd() tea.Cmd { gen, query := m.loadGen, m.mergeQuery() return func() tea.Msg { return mergeInfoMsg{gen: gen, info: query()} } } @@ -483,8 +504,11 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { case mergeInfoMsg: if msg.gen == m.loadGen { m.applyMergeInfo(msg.info) + m.selectAfterFetch() } return m, nil + case riskMsg: + return m.landRisk(msg) case diffLoadedMsg: if msg.seq != m.diffSeq || m.state != stateDiff { return m, nil // the user asked for another diff, or left @@ -503,16 +527,14 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.clampDiff() return m, nil case undoDoneMsg: - m.applyUndo(msg) - return m, nil + return m, m.applyUndo(msg) case repoLoadedMsg: if msg.err != nil { m.err = msg.err.Error() return m, nil } m.status = "" - m.applyKeepMarks(msg.branches, msg.reads) - return m, nil + return m, m.applyKeepMarks(msg.branches, msg.reads) case fetchDoneMsg: m.fetching = false if msg.err != nil { @@ -520,14 +542,16 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.status = "" return m, nil } + var cmd tea.Cmd if branches, reads, err := loadRepo(m.remoteLoaded); err == nil { // A fetch is non-destructive, so both the cursor (by name, in // applyBranches) and the user's pending marks survive it. - m.applyKeepMarks(branches, reads) + cmd = m.applyKeepMarks(branches, reads) } m.err = "" - m.status = "fetched & pruned — " + m.selectGone() - return m, nil + m.selectOnMerge = true + m.selectAfterFetch() + return m, cmd case switchDoneMsg: m.switching = false if msg.err != nil { @@ -542,27 +566,29 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { if msg.fromRemote { m.showRemote = false // the new local branch is where the user is now } + var cmd tea.Cmd if msg.loadErr == nil { // A switch deletes nothing, so pending marks survive it; carryMarks // drops the ones now on the current branch. - m.applyKeepMarks(msg.branches, msg.reads) + cmd = m.applyKeepMarks(msg.branches, msg.reads) } m.focusBranch(msg.name) - return m, nil + return m, cmd case branchDeletedMsg: if msg.idx >= 0 && msg.idx < len(m.results) { m.results[msg.idx] = msg.res } - if m.deletesDone() >= len(m.results) { - m.reloadBranches() - m.bodyTop, m.abortArmed = 0, false - if len(m.forceableFailures()) > 0 { - m.state = stateForcePrompt - } else { - m.state = stateResult - } + if m.deletesDone() < len(m.results) { + return m, nil } - return m, nil + cmd := m.reloadBranches() + m.bodyTop, m.abortArmed = 0, false + if len(m.forceableFailures()) > 0 { + m.state = stateForcePrompt + } else { + m.state = stateResult + } + return m, cmd case spinnerTickMsg: if m.state == stateDeleting { m.spinnerFrame++ @@ -708,10 +734,11 @@ func (m model) updateList(msg tea.KeyMsg) (tea.Model, tea.Cmd) { b.selected = !b.selected } case "r": - if b := m.cur(); b != nil && b.remoteDeletable() { - b.deleteRemote = !b.deleteRemote - } else if b != nil { - if why := b.remoteRefusal(); why != "" { + if b := m.cur(); b != nil { + if ok, why := b.remoteDelete(); ok { + b.deleteRemote = !b.deleteRemote + m.status = "" // a refusal shown for another row + } else if why != "" { m.status = why } } @@ -790,9 +817,8 @@ func (m model) updateList(msg tea.KeyMsg) (tea.Model, tea.Cmd) { case "?": m.state = stateHelp case "d", "enter": - if len(m.selectedBranches()) > 0 && !m.waitForMerge() { - m.measureSelectedRisk() // the confirm screen states what each delete costs - m.state, m.bodyTop = stateConfirm, 0 + if !m.measuring && len(m.selectedBranches()) > 0 && !m.waitForMerge() { + return m, m.confirmAfterRisk() } } return m, nil @@ -853,7 +879,9 @@ func (m model) localSelectedCount() int { // pushed --delete only when includeRemote is set (see updateConfirm), so a // remoteOnly row is left out of a local-only run. func (m *model) startDeletions(includeRemote bool) tea.Cmd { - m.measureSelectedRisk() // results carry the cost through to the force prompt + // Results carry the cost through to the force prompt. d has counted it + // already; this is for callers that start a run without d. + m.measureSelectedRisk() var sel []branch for _, b := range m.selectedBranches() { if includeRemote || !b.remoteOnly { @@ -900,8 +928,9 @@ func (m model) forceableFailures() []deleteResult { func (m model) updateForcePrompt(msg tea.KeyMsg) (tea.Model, tea.Cmd) { switch msg.String() { case "y", "Y": - m.forceDeleteUnmerged() + cmd := m.forceDeleteUnmerged() m.state, m.bodyTop = stateResult, 0 + return m, cmd case "n", "N", "esc", "q", "enter": m.state, m.bodyTop = stateResult, 0 case "ctrl+c": @@ -967,21 +996,22 @@ func carryMarks(old, fresh []branch) []branch { } // applyBranches installs a freshly-loaded branch set and recomputes everything -// derived from it (name-width, merge info, sort order). The cursor stays on the -// branch it was on. This is the single refresh core shared by every reload. -func (m *model) applyBranches(branches []branch, reads repoReads) { +// derived from it (name-width, sort order), then returns the cmd that runs its +// merge queries. The cursor stays on the branch it was on. This is the single +// refresh core shared by every reload. +func (m *model) applyBranches(branches []branch, reads repoReads) tea.Cmd { m.installBranches(branches, reads) - m.applyMergeInfo(m.mergeQuery()()) + return m.mergeCmd() } // applyKeepMarks is applyBranches for a reload that deleted nothing, so the // user's pending marks carry over by ref. -func (m *model) applyKeepMarks(branches []branch, reads repoReads) { - m.applyBranches(carryMarks(m.branches, branches), reads) +func (m *model) applyKeepMarks(branches []branch, reads repoReads) tea.Cmd { + return m.applyBranches(carryMarks(m.branches, branches), reads) } // installBranches is applyBranches without the merge queries, which cost a -// round of git. startupModel runs them in the background instead. +// round of git. Until they land, m.merging holds the keys that need them. func (m *model) installBranches(branches []branch, reads repoReads) { // Read the cursor's branch from the old list, before it is replaced: after, // the cursor index points into an unsorted new list. @@ -990,7 +1020,7 @@ func (m *model) installBranches(branches []branch, reads repoReads) { focus = b.name } m.loadGen++ - m.merging = false + m.merging = true // until this list's merge queries land m.config = reads.config m.branches = append(branches, remoteOnlyRows(branches, reads.remotes.rows)...) m.recomputeNameWidth() @@ -1001,11 +1031,11 @@ func (m *model) installBranches(branches []branch, reads repoReads) { b.protected = m.isProtected(*b) // The trunk and protect checks apply to the remote branch r would delete, // not only to the local name: a branch made from origin/main tracks main. - b.remoteProtected = !b.remoteOnly && b.upstream != "" && m.protectsName(b.remoteBranch()) + b.remoteProtected = !b.remoteOnly && b.upstream != "" && m.protectsRemote(*b) if b.locked() { b.selected = false } - if !b.remoteDeletable() { + if ok, _ := b.remoteDelete(); !ok { b.deleteRemote = false } } @@ -1034,11 +1064,19 @@ func remoteOnlyRows(local, remote []branch) []branch { // A remote row is matched by its branch part, so "release/*" covers both views. func (m model) isProtected(b branch) bool { if b.remoteOnly { - return b.name == m.remoteDefault || m.protectsName(b.remoteBranch()) + return b.name == m.remoteDefault || m.protectsRemote(b) } return m.protectsName(b.name) } +// protectsRemote reports whether b's remote branch is a remote's trunk or matches +// a pruner.protect glob. A renaming fetch refspec makes the name the tracking ref +// shows differ from the name on the remote, and both count: the README promises +// the shown name, and the push deletes the remote's. +func (m model) protectsRemote(b branch) bool { + return m.remoteTrunks["refs/remotes/"+b.upstream] || m.protectsName(b.shownBranch()) || m.protectsName(b.remoteBranch()) +} + // protectsName reports whether a branch of this name, local or on a remote, is // the trunk or matches a pruner.protect glob. func (m model) protectsName(name string) bool { @@ -1055,11 +1093,14 @@ func (m model) protectsName(name string) bool { // 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, reads, err := loadRepo(m.remoteLoaded); err == nil { - m.applyBranches(branches, reads) - m.cursor, m.top = 0, 0 +func (m *model) reloadBranches() tea.Cmd { + branches, reads, err := loadRepo(m.remoteLoaded) + if err != nil { + return nil } + cmd := m.applyBranches(branches, reads) + m.cursor, m.top = 0, 0 + return cmd } // hasBranch reports whether the loaded list holds a local branch of that name. @@ -1083,6 +1124,12 @@ func (m *model) resolveBases(reads repoReads) { } m.remoteDefault = shortRef(m.remoteDefaultRef) m.riskBase = shortRef(m.riskBaseRef) + m.remoteTrunks = map[string]bool{} + for ref, target := range reads.remotes.symref { + if strings.HasSuffix(ref, "/HEAD") { + m.remoteTrunks[target] = true + } + } } // goneRefs lists the refs of the gone branches. Only these are measured up @@ -1126,16 +1173,16 @@ func queryMergeInfo(defRef, baseRef string, gone []string) mergeInfo { <-remote var todo []string - info.risk = map[string]int{} + info.risk = map[string]riskCount{} for _, ref := range gone { if info.baseMerged[shortRef(ref)] { - info.risk[ref] = 0 + info.risk[ref] = riskCount{} } else { todo = append(todo, ref) } } - for i, n := range countRisk(todo, baseRef) { - info.risk[todo[i]] = n + for i, r := range countRisk(todo, baseRef) { + info.risk[todo[i]] = r } return info } @@ -1148,8 +1195,8 @@ func (m *model) applyMergeInfo(info mergeInfo) { b := &m.branches[i] b.remoteMerged = b.upstream != "" && info.remoteMerged[b.upstream] b.riskMeasured = false // the branch was just reloaded; any old count is stale - if n, ok := info.risk[b.ref()]; ok { - b.riskCommits, b.riskMeasured = n, true + if r, ok := info.risk[b.ref()]; ok { + b.setRisk(r) } } } @@ -1167,8 +1214,8 @@ func (m model) inBase(b branch) bool { // `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 countRisk(refs []string, base string) []int { - out := make([]int, len(refs)) +func countRisk(refs []string, base string) []riskCount { + out := make([]riskCount, len(refs)) var wg sync.WaitGroup for i, ref := range refs { // Each goroutine owns one slice element, so no two write the same one. @@ -1178,43 +1225,91 @@ func countRisk(refs []string, base string) []int { return out } -// measureRisk fills in the cost-of-deletion count for every not-yet-measured -// branch that want accepts. -func (m *model) measureRisk(want func(branch) bool) { - var idx []int +// riskTodo finds the selected branches whose cost of deletion is not yet known, +// answers what it can from the merge sets, and returns the refs that still +// need git cherry. Only branches a safe delete would refuse are measured, since +// those are the ones deleted with -D. A remote branch not merged into the +// default is measured too: deleting it can take the only shared copy of its +// commits. +func (m *model) riskTodo() []string { var refs []string for i, b := range m.branches { - if b.riskMeasured || !want(b) { + costly := b.gone || !b.safeDeletable() || (b.remoteOnly && !b.remoteMerged) + if b.riskMeasured || !b.selected || !costly { continue } // Answer from the merge sets instead of spawning the subprocess. if m.inBase(b) { - m.branches[i].riskCommits, m.branches[i].riskMeasured = 0, true + m.branches[i].setRisk(riskCount{}) continue } - idx = append(idx, i) refs = append(refs, b.ref()) } - for k, n := range countRisk(refs, m.riskBaseRef) { - m.branches[idx[k]].riskCommits, m.branches[idx[k]].riskMeasured = n, true + return refs +} + +// applyRisk records the counts taken for refs. It matches by ref, not index: +// the list can be re-sorted while a count runs. +func (m *model) applyRisk(refs []string, counts []riskCount) { + at := make(map[string]riskCount, len(refs)) + for i, ref := range refs { + at[ref] = counts[i] + } + for i := range m.branches { + if r, ok := at[m.branches[i].ref()]; ok { + m.branches[i].setRisk(r) + } } } -// 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. A remote -// branch not merged into the default is measured too: deleting it can take the -// only shared copy of its commits. +// measureSelectedRisk measures what deleting each selected branch would cost, +// and waits for the counts. Script mode has no screen to keep drawing. In the +// TUI, d has measured the selection before the confirm screen opens (see +// confirmAfterRisk), so this counts nothing there. func (m *model) measureSelectedRisk() { - m.measureRisk(func(b branch) bool { - return b.selected && (b.gone || !b.safeDeletable() || (b.remoteOnly && !b.remoteMerged)) - }) + refs := m.riskTodo() + m.applyRisk(refs, countRisk(refs, m.riskBaseRef)) +} + +// confirmAfterRisk opens the confirm screen once it can state what each +// selected delete costs. git cherry can reach the network in a partial clone +// (see networkUse), so the counts the merge sets cannot answer run off the +// update loop. Their riskMsg comes back here: the selection may have grown +// while they ran. +func (m *model) confirmAfterRisk() tea.Cmd { + refs := m.riskTodo() + if len(refs) == 0 { + m.measuring = false + m.state, m.bodyTop = stateConfirm, 0 + return nil + } + m.measuring = true + m.status = "counting commits at risk…" + gen, base := m.loadGen, m.riskBaseRef + return func() tea.Msg { return riskMsg{gen: gen, refs: refs, counts: countRisk(refs, base)} } +} + +// landRisk records the counts d asked for, then opens the confirm screen. Counts +// taken for an older load are dropped: the list they measured is gone. +func (m model) landRisk(msg riskMsg) (tea.Model, tea.Cmd) { + m.status = "" + if msg.gen != m.loadGen { + m.measuring = false + m.status = "the branch list changed — press d again" + return m, nil + } + m.applyRisk(msg.refs, msg.counts) + if m.state != stateList || len(m.selectedBranches()) == 0 { + m.measuring = false // the user moved on + return m, nil + } + return m, m.confirmAfterRisk() } // forceDeleteUnmerged re-runs the deletions that a safe (-d) delete refused, // this time with -D. It updates the matching result in place so the results // screen reflects the retry outcome. -func (m *model) forceDeleteUnmerged() { +func (m *model) forceDeleteUnmerged() tea.Cmd { for i := range m.results { r := &m.results[i] if r.localOK || !r.forceable { @@ -1237,7 +1332,7 @@ func (m *model) forceDeleteUnmerged() { pushRemoteDelete(r) } } - m.reloadBranches() + return m.reloadBranches() } // undoDoneMsg carries a background undo and the reload after it. @@ -1284,7 +1379,7 @@ func (m *model) undo() tea.Cmd { } // applyUndo records a finished undo and installs the reload that came with it. -func (m *model) applyUndo(msg undoDoneMsg) { +func (m *model) applyUndo(msg undoDoneMsg) tea.Cmd { remote := 0 for _, i := range msg.restored { m.results[i].restored = true @@ -1294,14 +1389,30 @@ func (m *model) applyUndo(msg undoDoneMsg) { remote++ } } + var cmd tea.Cmd if msg.loadErr == nil { - m.applyKeepMarks(msg.branches, msg.reads) + cmd = m.applyKeepMarks(msg.branches, msg.reads) } m.err = strings.Join(msg.errs, "; ") m.status = fmt.Sprintf("restored %d branch(es)", len(msg.restored)) if remote > 0 { m.status += fmt.Sprintf(" · %d remote branch(es) not restored — see the commands printed on quit", remote) } + return cmd +} + +// selectAfterFetch finishes p. It selects the gone branches once the merge +// queries have counted what each one holds, and says it is waiting until then. +func (m *model) selectAfterFetch() { + if !m.selectOnMerge { + return + } + if m.merging { + m.status = "fetched & pruned — reading merge status…" + return + } + m.selectOnMerge = false + m.status = "fetched & pruned — " + m.selectGone() } // selectGone selects every gone branch that carries nothing missing from the @@ -1317,12 +1428,12 @@ func (m *model) selectGone() string { } gone++ switch { + case br.riskFailed: + unknown++ + continue case br.riskCommits > 0: risky++ continue - case br.riskCommits == riskUnknown: - unknown++ - continue } br.selected = true } diff --git a/view.go b/view.go index 93a312d..374fe8c 100644 --- a/view.go +++ b/view.go @@ -594,6 +594,10 @@ func (m model) confirmParts() (header, body, footer []string) { switch { case br.remoteOnly: body = append(body, " "+dimStyle.Render("remote branch — no local copy")) + // A renaming fetch refspec gives the remote's branch another name. + if target := br.remoteName() + "/" + br.remoteBranch(); target != br.name { + body = append(body, " "+errStyle.Render("delete remote "+target)) + } case br.gone: body = append(body, " "+goneStyle.Render("upstream gone: "+br.upstream+" — will prune with -D (force)")) case br.upstream != "": @@ -607,7 +611,7 @@ func (m model) confirmParts() (header, body, footer []string) { // A gone branch has no upstream left to test, so remoteMerged is // always false for it. The risk count is the real answer: say so // when it is clean, and riskWarning speaks when it is not. - if br.riskMeasured && br.riskCommits == 0 && m.riskBase != "" { + if br.riskMeasured && !br.atRisk() && m.riskBase != "" { body = append(body, " "+okStyle.Render("✓ no commits missing from "+m.riskBase)) } case br.upstream != "" && m.remoteDefault != "": @@ -665,7 +669,7 @@ func (m model) worktreeWarning(br branch) string { // discarded, under -d the delete simply fails. func (m model) riskWarning(br branch) string { if br.remoteOnly { - if br.riskCommits != 0 { + if br.atRisk() { return fmt.Sprintf("⚠ %s — this may be the only shared copy of them", m.riskCount(br)) } return "" @@ -674,7 +678,7 @@ func (m model) riskWarning(br branch) string { return "" } if br.forcedDelete(m.force) { // -D: the question is what gets discarded - if br.riskCommits != 0 { + if br.atRisk() { return fmt.Sprintf("⚠ %s — force delete (-D) will discard them", m.riskCount(br)) } if m.riskBase == "" { @@ -683,17 +687,17 @@ func (m model) riskWarning(br branch) string { return "" // measured against a real base: nothing here is at risk } // -d will be refused either way; the count is what a force would then cost. - if br.riskCommits != 0 { + if br.atRisk() { return fmt.Sprintf("⚠ not fully merged: %s — safe delete (-d) will fail; use force (f)", m.riskCount(br)) } return "⚠ not fully merged — safe delete (-d) will fail; use force (f)" } // riskCount phrases br's risk count for a warning. A count git cherry could not -// take (riskUnknown) is nonzero on purpose, so every warning that tests for -// commits at risk also speaks for it. +// take is at risk (see atRisk), so every warning that tests for commits at risk +// also speaks for it. func (m model) riskCount(br branch) string { - if br.riskCommits == riskUnknown { + if br.riskFailed { return "an unknown number of commits not in " + m.riskBase } return fmt.Sprintf("%d commit(s) not in %s", br.riskCommits, m.riskBase) @@ -715,10 +719,10 @@ func (m model) forcePromptParts() (header, body, footer []string) { for _, r := range failures { body = append(body, " "+cursorStyle.Render("• "+r.br.name)+dimStyle.Render(" at "+r.br.hash)) // 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. + // ran: a branch not at risk has either nothing missing from the base or + // no base to measure against. switch { - case r.br.riskCommits != 0: + case r.br.atRisk(): body = append(body, " "+errStyle.Render(fmt.Sprintf("⚠ %s will be lost", m.riskCount(r.br)))) case m.riskBase == "": body = append(body, " "+errStyle.Render("⚠ no base branch to compare against — unmerged commits may be lost"))