From 7b09c4a462f6bce5aeee0ef7c1b9eddb8b0473bc Mon Sep 17 00:00:00 2001 From: John Bolliger Date: Mon, 17 Aug 2026 22:30:22 -0700 Subject: [PATCH 1/2] Qualify git refs so a same-named tag cannot shadow a branch A tag sharing a branch's name silently shadowed the branch: git's ref search order puts refs/tags/ ahead of refs/heads/, and %(refname:short) returns the shortest *unambiguous* name, which grows a "heads/" prefix the moment the tag exists. Tagging a release branch with its own name is ordinary practice, so this reached everyday repos. The worst symptom was silent: riskCommitCount measured the tag instead of the branch and reported nothing at risk, so a force delete of a branch holding real work drew no warning at all. Resolve refs at the point where the namespace is actually known and carry them forward, rather than re-deriving later: - loadBranches reads %(refname)/%(upstream); shortRef strips the namespace for display and name-keyed lookups. - remoteDefault, localDefaultBranch and baseBranch return fully qualified refs. localDefaultBranch previously probed a bare "main", which a tag satisfies, leaving the bug live on the no-remote path. - remoteDefault drops symbolic-ref --short, which has the same shortest-unambiguous hazard. - branchRef qualifies local branch names for git; the model keeps riskBase for display alongside riskBaseRef for git. - push --delete takes a qualified refspec. A bare one is rejected as matching more than one ref when the remote also carries the tag, deleting nothing. This also makes deleting an already-absent remote branch idempotent rather than an error, which is the better trade: the race ends in the state the user armed. Also preserve pending selections across a fetch. `p` reloaded every branch struct and silently discarded the user's selections and armed remotes; a fetch mutates nothing local, so carryMarks copies them onto the new set. An armed remote is dropped when the fetch reveals the upstream is gone, since that push could only fail. Startup stays at 7 git subprocesses, verified with a logging shim. Co-Authored-By: Claude Fable 5 --- main.go | 138 ++++++--- main_test.go | 837 ++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 926 insertions(+), 49 deletions(-) diff --git a/main.go b/main.go index 38dfd42..d7e490e 100644 --- a/main.go +++ b/main.go @@ -133,6 +133,7 @@ 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 spinnerFrame int // animation frame for the deleting spinner (deletion counts derive from results) @@ -192,11 +193,39 @@ func runGit(args ...string) (string, error) { return out.String(), nil } +// branchRef fully qualifies a local branch name. git's ref search order puts +// refs/tags/ ahead of refs/heads/, so a tag sharing a branch's name +// silently shadows the branch in any command handed the bare name — and tagging a +// release branch with its own name is ordinary practice. +func branchRef(name string) string { return "refs/heads/" + name } + +// shortRef strips the namespace from a full ref, yielding the plain branch name +// ("feature/x") or remote-tracking name ("origin/feature/x") the rest of the +// program keys on. git's own %(refname:short) cannot be used for this: it yields +// the shortest *unambiguous* name, which grows a "heads/" or "remotes/" prefix +// exactly when a tag shares the name — silently breaking every name-keyed lookup +// and every ref built back up from it. +func shortRef(ref string) string { + for _, prefix := range []string{"refs/heads/", "refs/remotes/"} { + if s, ok := strings.CutPrefix(ref, prefix); ok { + return s + } + } + return ref +} + +// refExists reports whether a ref resolves. Always pass a fully qualified ref: a +// bare name would also match a tag (see branchRef). +func refExists(ref string) bool { + _, err := runGit("rev-parse", "--verify", "--quiet", ref) + return err == nil +} + var trackRe = regexp.MustCompile(`ahead (\d+)|behind (\d+)`) func loadBranches() ([]branch, error) { - const format = "%(refname:short)%00%(objectname:short)%00%(committerdate:iso8601-strict)%00" + - "%(committerdate:relative)%00%(upstream:short)%00%(upstream:track)%00%(HEAD)%00%(contents:subject)" + const format = "%(refname)%00%(objectname:short)%00%(committerdate:iso8601-strict)%00" + + "%(committerdate:relative)%00%(upstream)%00%(upstream:track)%00%(HEAD)%00%(contents:subject)" out, err := runGit("for-each-ref", "--format="+format, "refs/heads") if err != nil { return nil, err @@ -211,10 +240,10 @@ func loadBranches() ([]branch, error) { continue } b := branch{ - name: f[0], + name: shortRef(f[0]), hash: f[1], committedRel: f[3], - upstream: f[4], + upstream: shortRef(f[4]), isCurrent: f[6] == "*", subject: f[7], } @@ -256,43 +285,52 @@ func remotes() []string { return names } -// localDefaultBranch returns a local main/master, skipping exclude so a branch -// is never compared against itself. Returns "" when neither exists. +// 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. +// +// 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 _, err := runGit("rev-parse", "--verify", "--quiet", c); err == nil { - return c + if ref := branchRef(c); refExists(ref) { + return ref } } return "" } // remoteDefault resolves the remote's default branch as a remote-tracking ref -// (e.g. "origin/main"): /HEAD if set, else /main, else -// /master, trying each remote in turn. Returns "" when none can be found. +// (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() { - if out, err := runGit("symbolic-ref", "--short", "refs/remotes/"+r+"/HEAD"); err == nil { + // 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 } } for _, c := range []string{r + "/main", r + "/master"} { - if _, err := runGit("rev-parse", "--verify", "--quiet", "refs/remotes/"+c); err == nil { - return c + if ref := "refs/remotes/" + c; refExists(ref) { + return ref } } } return "" } -// baseBranch returns a reference to diff a branch against: the remote default -// branch, else a local main/master, excluding name itself. +// 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 != "" && def != name { + if def := remoteDefault(); def != "" { return def } return localDefaultBranch(name) @@ -306,10 +344,11 @@ func baseBranch(name string) string { // 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 { - if base == "" || base == name { + ref := branchRef(name) + if base == "" || base == ref { return 0 } - out, err := runGit("cherry", base, name) + out, err := runGit("cherry", base, ref) if err != nil { return 0 } @@ -326,21 +365,21 @@ func riskCommitCount(name, base string) int { // it reports into a set. func mergedSet(args ...string) map[string]bool { set := map[string]bool{} - out, err := runGit(append(args, "--format=%(refname:short)")...) + out, err := runGit(append(args, "--format=%(refname)")...) if err != nil { return set } for _, line := range strings.Split(out, "\n") { if s := strings.TrimSpace(line); s != "" { - set[s] = true + set[shortRef(s)] = true } } return set } // remoteMergedSet returns the set of remote-tracking branches (short names, e.g. -// "origin/feature") whose tip is merged into def. Operates on local -// remote-tracking refs, so it needs no network — it reflects the last fetch. +// "origin/feature") whose tip is merged into def, a qualified ref. Operates on +// local remote-tracking refs, so it needs no network — it reflects the last fetch. func remoteMergedSet(def string) map[string]bool { if def == "" { return map[string]bool{} @@ -394,14 +433,15 @@ func spinnerTickCmd() tea.Cmd { } // loadDiff returns the patch introduced on name relative to its merge-base with -// the repo's default branch — i.e. what the branch contains — and the base ref used. +// the repo's default branch — i.e. what the branch contains — and the base it was +// compared against, shortened for display. func loadDiff(name string) (diff, base string, err error) { - base = baseBranch(name) - if base == "" { - base = "HEAD" + ref := baseBranch(name) + if ref == "" { + ref = "HEAD" } - diff, err = runGit("diff", base+"..."+name) - return diff, base, err + diff, err = runGit("diff", ref+"..."+branchRef(name)) + return diff, shortRef(ref), err } // ---- model ---- @@ -518,7 +558,9 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, nil } if branches, err := loadBranches(); err == nil { - m.applyBranches(branches) // preserves the cursor by name (fetch is non-destructive) + // 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)) } // Auto-select only gone branches that carry nothing missing from the // base. Ones holding unique commits are left unselected so discarding @@ -847,7 +889,9 @@ func deleteBranch(b branch, flag string, wantRemote bool) deleteResult { func pushRemoteDelete(res *deleteResult) { res.remoteSkipped = false res.remoteTried = true - if _, err := runGit("push", res.br.remoteName(), "--delete", res.br.remoteBranch()); err != nil { + // Qualify the remote branch: a remote carrying both a branch and a tag of that + // name rejects a bare refspec as matching more than one ref, deleting nothing. + if _, err := runGit("push", res.br.remoteName(), "--delete", branchRef(res.br.remoteBranch())); err != nil { res.remoteErr = err.Error() } else { res.remoteOK = true @@ -867,6 +911,25 @@ func (m *model) performDeletions() { 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 +// delete is dropped when the fetch reveals the upstream is already gone: the push +// it would run can only fail. +func carryMarks(old, fresh []branch) []branch { + prev := make(map[string]branch, len(old)) + for _, b := range old { + prev[b.name] = b + } + for i := range fresh { + if p, ok := prev[fresh[i].name]; ok { + fresh[i].selected = p.selected + fresh[i].deleteRemote = p.deleteRemote && !fresh[i].gone + } + } + return fresh +} + // applyBranches installs a freshly-loaded branch set and recomputes everything // 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 @@ -892,14 +955,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() { - m.remoteDefault = remoteDefault() - merged := remoteMergedSet(m.remoteDefault) + defRef := remoteDefault() + merged := remoteMergedSet(defRef) headMerged := localMergedSet() - m.riskBase = m.remoteDefault - if m.riskBase == "" { - m.riskBase = localDefaultBranch("") + m.riskBaseRef = defRef + if m.riskBaseRef == "" { + m.riskBaseRef = localDefaultBranch("") } + // The refs drive git; the short forms are what the views print. + m.remoteDefault = shortRef(defRef) + m.riskBase = shortRef(m.riskBaseRef) for i := range m.branches { b := &m.branches[i] @@ -921,7 +987,7 @@ func (m *model) measureRisk(b *branch) { if b.riskMeasured { return } - b.riskCommits = riskCommitCount(b.name, m.riskBase) + b.riskCommits = riskCommitCount(b.name, m.riskBaseRef) b.riskMeasured = true } diff --git a/main_test.go b/main_test.go index 0bf3781..f72dace 100644 --- a/main_test.go +++ b/main_test.go @@ -51,21 +51,45 @@ func git(t *testing.T, dir string, args ...string) { } } -func setupRepo(t *testing.T) string { +// initRepo creates an empty repository on the named initial branch with a commit +// identity configured. Callers add the commits, remotes and branches they need. +func initRepo(t *testing.T, branch string) string { t.Helper() tmp := t.TempDir() - remote := t.TempDir() - git(t, remote, "init", "--bare", "-q") - git(t, tmp, "init", "-q", "-b", "main") + 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 +} + +// addOrigin gives dir a bare origin remote and pushes trunk to it. +func addOrigin(t *testing.T, dir, trunk string) { + t.Helper() + remote := t.TempDir() + git(t, remote, "init", "--bare", "-q", "-b", trunk) + git(t, dir, "remote", "add", "origin", remote) + git(t, dir, "push", "-q", "-u", "origin", trunk) +} + +// setupLocalRepo builds the branch shapes with no remote at all — a scratch +// project, or one that has simply never been pushed. +func setupLocalRepo(t *testing.T) string { + t.Helper() + tmp := initRepo(t, "main") commitFile(t, tmp, "a", "a") - git(t, tmp, "remote", "add", "origin", remote) - git(t, tmp, "push", "-q", "-u", "origin", "main") git(t, tmp, "branch", "feature/merged") // merged into main -> safe delete git(t, tmp, "checkout", "-q", "-b", "feature/unmerged") commitFile(t, tmp, "b", "b") - git(t, tmp, "checkout", "-q", "-b", "feature/tracked") + git(t, tmp, "checkout", "-q", "main") + return tmp +} + +// setupRepo is setupLocalRepo plus an origin and a branch tracking it. +func setupRepo(t *testing.T) string { + t.Helper() + tmp := setupLocalRepo(t) + addOrigin(t, tmp, "main") + git(t, tmp, "checkout", "-q", "-b", "feature/tracked", "feature/unmerged") git(t, tmp, "push", "-q", "-u", "origin", "feature/tracked") git(t, tmp, "checkout", "-q", "main") return tmp @@ -80,6 +104,14 @@ func chdir(t *testing.T, dir string) { t.Cleanup(func() { os.Chdir(old) }) } +func branchNames(bs []branch) []string { + out := make([]string, len(bs)) + for i, b := range bs { + out[i] = b.name + } + return out +} + func find(bs []branch, name string) *branch { for i := range bs { if bs[i].name == name { @@ -114,10 +146,7 @@ func TestLoadAndSort(t *testing.T) { m.field = sortName m.ascending = true m.sortBranches() - got := make([]string, len(m.branches)) - for i, b := range m.branches { - got[i] = b.name - } + got := branchNames(m.branches) want := []string{"feature/merged", "feature/tracked", "feature/unmerged", "main"} for i := range want { if got[i] != want[i] { @@ -726,10 +755,11 @@ func TestNonOriginRemoteResolves(t *testing.T) { git(t, repo, "remote", "rename", "origin", "upstream") git(t, repo, "fetch", "-q", "--all", "--prune") - if got := remoteDefault(); got != "upstream/main" { + // The resolvers hand back qualified refs; the model shortens them for display. + if got := remoteDefault(); got != "refs/remotes/upstream/main" { t.Fatalf("remoteDefault should resolve upstream/main, got %q", got) } - if got := baseBranch("feature/unmerged"); got != "upstream/main" { + if got := baseBranch("feature/unmerged"); got != "refs/remotes/upstream/main" { t.Fatalf("baseBranch should use the non-origin remote, got %q", got) } m, err := initialModel() @@ -739,6 +769,9 @@ func TestNonOriginRemoteResolves(t *testing.T) { if m.remoteDefault != "upstream/main" || m.riskBase != "upstream/main" { t.Fatalf("model should cache the resolved default: %q / %q", m.remoteDefault, m.riskBase) } + if m.riskBaseRef != "refs/remotes/upstream/main" { + t.Fatalf("model should cache the ref form too: %q", m.riskBaseRef) + } } // origin is preferred when several remotes are configured. @@ -1043,6 +1076,784 @@ func TestNonUnmergedFailureIsNotForceable(t *testing.T) { } } +// A tag sharing a branch's name shadows it: git resolves refs/tags/ before +// refs/heads/, so any bare name handed to git measures the tag instead. On +// the risk path that reports a branch's unmerged commits as already safe — +// precisely when -D is about to discard them. Release branches tagged with their +// own name (v1.2, release-3) make this an everyday shape. +func TestTagShadowingBranchName(t *testing.T) { + t.Run("branch_name", func(t *testing.T) { + repo := setupRepo(t) + chdir(t, repo) + // 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 { + t.Fatalf("want the branch's 1 unique commit measured, got %d (the tag was measured)", n) + } + diff, _, err := loadDiff("feature/unmerged") + if err != nil { + t.Fatalf("loadDiff: %v", err) + } + if !strings.Contains(diff, "+++ b/b") { + t.Fatalf("diff must show the branch's content, not the tag's:\n%s", diff) + } + + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + // The branch must load under its plain name. %(refname:short) reports the + // shortest *unambiguous* name, which becomes "heads/feature/unmerged" the + // moment the tag exists — breaking every name-keyed lookup downstream. + b := find(m.branches, "feature/unmerged") + if b == nil { + t.Fatalf("branch must load under its plain name; loaded %v", branchNames(m.branches)) + } + + // End to end: the confirm screen must still state what the delete costs. + b.selected = true + nm, _ := m.updateList(key("d")) + m = nm.(model) + 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) + } + + // And the delete must land on the branch, leaving the tag alone. + m.force = true + m.performDeletions() + if !m.results[0].localOK { + t.Fatalf("delete failed: %s", m.results[0].localErr) + } + if find(m.branches, "feature/unmerged") != nil { + t.Fatal("the branch should be gone") + } + if _, err := runGit("rev-parse", "--verify", "--quiet", "refs/tags/feature/unmerged"); err != nil { + t.Fatal("the tag must survive a branch delete") + } + }) + + // A tag named main/master, with no branch of that name, must not be adopted as + // the comparison base — `rev-parse --verify main` is satisfied by the tag. This + // was the same bug on the no-remote path: measured against the tag, a branch + // holding real work reported nothing at risk, so a -D drew no warning at all. + t.Run("local_default_name", func(t *testing.T) { + repo := initRepo(t, "trunk") + chdir(t, repo) + commitFile(t, repo, "a", "a") + git(t, repo, "checkout", "-q", "-b", "feature", "trunk") + commitFile(t, repo, "b", "b") + // The tag carries the feature tip, so measuring against it reports nothing. + git(t, repo, "tag", "main", "feature") + git(t, repo, "checkout", "-q", "trunk") + + if got := localDefaultBranch(""); got != "" { + t.Fatalf("a tag must not pose as the local default branch, got %q", got) + } + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + if m.riskBase != "" { + t.Fatalf("no honest base exists here, got %q", m.riskBase) + } + b := find(m.branches, "feature") + b.selected = true + m.force = true + m.measureSelectedRisk() + if w := m.riskWarning(*b); !strings.Contains(w, "no base branch to compare against") { + t.Fatalf("a -D with no measurable base must say so rather than stay silent: %q", w) + } + }) + + // A remote carrying both a branch and a tag of one name rejects a bare refspec + // as matching more than one ref, deleting nothing. + t.Run("remote_delete_refspec", func(t *testing.T) { + repo := setupRepo(t) + chdir(t, repo) + git(t, repo, "tag", "feature/tracked", "main") + git(t, repo, "push", "-q", "origin", "refs/tags/feature/tracked") + + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + tb := find(m.branches, "feature/tracked") + tb.selected = true + tb.deleteRemote = true + m.force = true + m.performDeletions() + + r := m.results[0] + if !r.localOK { + t.Fatalf("local delete failed: %s", r.localErr) + } + if !r.remoteTried || !r.remoteOK { + t.Fatalf("the remote branch delete must succeed: tried=%v err=%s", r.remoteTried, r.remoteErr) + } + if remoteHasBranch(t, repo, "feature/tracked") { + t.Fatal("the remote branch should be gone") + } + // Only the branch was asked for; the tag must survive. + out, err := runGit("ls-remote", "--tags", "origin", "feature/tracked") + if err != nil || strings.TrimSpace(out) == "" { + t.Fatalf("the remote tag must survive a branch delete (err=%v out=%q)", err, out) + } + }) + + // The upstream is read as a short name too, so a tag named after a + // remote-tracking ref disambiguates it to "remotes/origin/…" — which would + // then be split into a bogus remote named "remotes". + t.Run("upstream_name", func(t *testing.T) { + repo := setupRepo(t) + chdir(t, repo) + git(t, repo, "tag", "origin/feature/tracked", "main") + + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + b := find(m.branches, "feature/tracked") + if b == nil { + t.Fatalf("feature/tracked missing; loaded %v", branchNames(m.branches)) + } + if b.upstream != "origin/feature/tracked" { + t.Fatalf("upstream must load unqualified, got %q", b.upstream) + } + if b.remoteName() != "origin" || b.remoteBranch() != "feature/tracked" { + t.Fatalf("a shadowed upstream must still split correctly: %s / %s", + b.remoteName(), b.remoteBranch()) + } + }) + + // The base is resolved to a short name too, so a tag can shadow it the same way + // — and a wrong base silently changes every branch's measured risk. + t.Run("base_name", func(t *testing.T) { + repo := setupRepo(t) + chdir(t, repo) + // A tag named after the remote-tracking base, pointing at the work that is + // supposed to be measured as missing from it. + git(t, repo, "tag", "origin/main", "feature/unmerged") + + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + 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 { + t.Fatalf("want 1 commit at risk, got %d (the tag was used as the base)", n) + } + }) +} + +// A fetch mutates nothing local, so pressing `p` must not silently discard the +// selections the user already made — they are the whole reason to press d next. +func TestFetchPreservesSelections(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 + tb := find(m.branches, "feature/tracked") + tb.selected = true + tb.deleteRemote = true + + nm, _ := m.Update(fetchPruneCmd()()) + after := nm.(model) + + if b := find(after.branches, "feature/unmerged"); b == nil || !b.selected { + t.Fatalf("a manual selection must survive a fetch: %+v", b) + } + if b := find(after.branches, "feature/tracked"); b == nil || !b.selected || !b.deleteRemote { + t.Fatalf("an armed remote delete must survive a fetch: %+v", b) + } + if b := find(after.branches, "feature/merged"); b == nil || b.selected { + t.Fatalf("an untouched branch must stay unselected: %+v", b) + } +} + +// The armed remote delete is dropped when the fetch reveals the upstream is +// already gone: the push it would run can only fail, and the results screen would +// report that failure as if the user had asked for something impossible. +func TestFetchDisarmsRemoteForGoneBranch(t *testing.T) { + repo := setupRepo(t) + chdir(t, repo) + + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + tb := find(m.branches, "feature/tracked") + tb.selected = true + tb.deleteRemote = true + + // 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) + + b := find(after.branches, "feature/tracked") + if b == nil || !b.gone { + t.Fatalf("precondition: feature/tracked should be gone after the fetch: %+v", b) + } + if !b.selected { + t.Fatalf("the selection itself must survive: %+v", b) + } + if b.deleteRemote { + t.Fatalf("the armed remote must be disarmed once the upstream is gone: %+v", b) + } +} + +// Everyday shape: the PR merged, the remote branch was deleted, and the user is +// still standing on that branch when they press `p`. The current branch cannot be +// deleted, so it must never be auto-selected — and must survive if it somehow is. +func TestGoneCurrentBranchIsNotPruned(t *testing.T) { + repo := setupRepo(t) + chdir(t, repo) + + git(t, repo, "checkout", "-q", "feature/tracked") + git(t, repo, "push", "-q", "origin", "--delete", "feature/tracked") + + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + nm, _ := m.Update(fetchPruneCmd()()) + m = nm.(model) + + b := find(m.branches, "feature/tracked") + if b == nil || !b.gone || !b.isCurrent { + t.Fatalf("precondition: want the gone current branch: %+v", b) + } + if b.selected { + t.Fatal("the checked-out branch must never be auto-selected for deletion") + } + // It cannot be pruned, so the status must not count it as one waiting to be. + if strings.Contains(m.status, "press d to prune") { + t.Fatalf("status must not offer to prune the current branch: %q", m.status) + } + + // 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() + if len(m.results) != 1 || m.results[0].localOK { + t.Fatalf("deleting the checked-out branch must fail: %+v", m.results) + } + if m.results[0].forceable { + t.Fatalf("a checked-out branch cannot be rescued by -D: %q", m.results[0].localErr) + } + if find(m.branches, "feature/tracked") == nil { + t.Fatal("feature/tracked must survive the refused delete") + } +} + +// setupTrunkRepo builds a repo whose default branch is neither main nor master, +// with origin/HEAD unset — what `git remote add` + push produces, as opposed to a +// clone, which is the only thing that sets origin/HEAD. +func setupTrunkRepo(t *testing.T) string { + t.Helper() + tmp := initRepo(t, "trunk") + commitFile(t, tmp, "a", "a") + addOrigin(t, tmp, "trunk") + git(t, tmp, "checkout", "-q", "-b", "feature/unmerged") + commitFile(t, tmp, "b", "b") + git(t, tmp, "checkout", "-q", "trunk") + return tmp +} + +// A repo with no remote configured: every remote-derived lookup must degrade to +// the local default branch rather than going blank and silencing the warnings. +func TestLocalOnlyRepo(t *testing.T) { + repo := setupLocalRepo(t) + chdir(t, repo) + + if got := remotes(); len(got) != 0 { + t.Fatalf("no remotes should be configured, got %v", got) + } + if got := remoteDefault(); got != "" { + t.Fatalf("remoteDefault should be empty, got %q", got) + } + + m, err := initialModel() + if err != nil { + t.Fatalf("initialModel must work without a remote: %v", err) + } + if m.riskBase != "main" || m.riskBaseRef != "refs/heads/main" { + t.Fatalf("risk should fall back to the local default: %q / %q", m.riskBase, m.riskBaseRef) + } + if _, base, err := loadDiff("feature/unmerged"); err != nil || base != "main" { + t.Fatalf("diff base should fall back to local main, got %q err=%v", base, err) + } + + // The cost of a delete is still measured and still named. + b := find(m.branches, "feature/unmerged") + b.selected = true + m.measureSelectedRisk() + if b.riskCommits != 1 { + t.Fatalf("want 1 commit at risk against local main, got %d", b.riskCommits) + } + if w := m.riskWarning(*b); !strings.Contains(w, "1 commit(s) not in main") { + t.Fatalf("warning must name the local base: %q", w) + } + + // `p` is a harmless no-op rather than an error. + nm, _ := m.Update(fetchPruneCmd()()) + m = nm.(model) + if m.err != "" { + t.Fatalf("fetch must succeed with no remotes: %q", m.err) + } + if !strings.Contains(m.status, "no gone branches") { + t.Fatalf("status should report nothing to prune, got %q", m.status) + } + + // And an ordinary delete still works. feature/unmerged is still selected from + // 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() + if len(m.results) != 1 || !m.results[0].localOK { + t.Fatalf("merged branch should delete cleanly: %+v", m.results) + } +} + +// `git init` with nothing committed: the tool must open on an empty list rather +// than refusing to start or panicking on the missing HEAD. (git itself fails +// `branch --merged HEAD` here, which mergedSet has to absorb.) +func TestUnbornHeadRepo(t *testing.T) { + repo := initRepo(t, "main") + chdir(t, repo) + + m, err := initialModel() + if err != nil { + t.Fatalf("initialModel must succeed in a fresh repo: %v", err) + } + if len(m.branches) != 0 { + t.Fatalf("a repo with no commits has no branches, got %v", branchNames(m.branches)) + } + if m.riskBase != "" || m.riskBaseRef != "" { + t.Fatalf("nothing can serve as a base: %q / %q", m.riskBase, m.riskBaseRef) + } + if m.cur() != nil { + t.Fatal("there is no branch under the cursor") + } + if out := stripANSI(m.listView()); !strings.Contains(out, "no local branches found") { + t.Fatalf("the empty list must say so:\n%s", out) + } + + // Every key that acts on the cursor or the selection must be a no-op here. + for _, k := range []string{"j", "k", "g", "G", " ", "r", "a", "n", "v", "d", "s", "o", "f"} { + nm, _ := m.updateList(key(k)) + m = nm.(model) + if m.state != stateList { + t.Fatalf("%q left the list view (state %v) with no branches", k, m.state) + } + } + if len(m.selectedBranches()) != 0 { + t.Fatal("nothing can be selected") + } + + // Including a fetch, which has no remote to talk to. + nm, _ := m.Update(fetchPruneCmd()()) + if got := nm.(model).err; got != "" { + t.Fatalf("fetch in a fresh repo should not error: %q", got) + } +} + +// Mid-bisect, mid-rebase, or on a checked-out tag, HEAD is on no branch at all. +// Nothing is current, so nothing carries the current-branch protection — and git +// will happily delete the branch HEAD is parked on. +func TestDetachedHead(t *testing.T) { + repo := setupRepo(t) + chdir(t, repo) + git(t, repo, "checkout", "-q", "--detach", "main") + + m, err := initialModel() + if err != nil { + t.Fatalf("initialModel must work on a detached HEAD: %v", err) + } + for _, b := range m.branches { + if b.isCurrent { + t.Fatalf("no branch is current when HEAD is detached: %+v", b) + } + } + // git prints a "(HEAD detached at …)" pseudo-entry in --merged output; it must + // not be mistaken for a branch, and real branches must still be classified. + if len(m.branches) != 4 { + t.Fatalf("want the 4 real branches, got %v", branchNames(m.branches)) + } + if b := find(m.branches, "main"); b == nil || !b.headMerged { + t.Fatalf("main is merged into the detached HEAD: %+v", b) + } + if b := find(m.branches, "feature/unmerged"); b == nil || b.headMerged { + t.Fatalf("feature/unmerged is not merged into the detached HEAD: %+v", b) + } + + // With nothing current, `a` selects everything — there is no branch to spare. + nm, _ := m.updateList(key("a")) + m = nm.(model) + if got := len(m.selectedBranches()); got != len(m.branches) { + t.Fatalf("select-all should take all %d branches, got %d", len(m.branches), got) + } + + // Deleting the branch HEAD is parked on is legal while detached, and safe: + // the commits stay reachable from HEAD. + nm, _ = m.updateList(key("n")) + m = nm.(model) + find(m.branches, "main").selected = true + m.performDeletions() + if len(m.results) != 1 || !m.results[0].localOK { + t.Fatalf("main should delete cleanly while detached: %+v", m.results) + } + if find(m.branches, "main") != nil { + t.Fatal("main should be gone") + } +} + +// A repo whose default branch is neither main nor master. Without origin/HEAD +// there is nothing left to guess from, so no base resolves at all — the path +// every "no base branch to compare against" message hangs off. +func TestNonStandardDefaultBranch(t *testing.T) { + t.Run("unresolvable_base", func(t *testing.T) { + repo := setupTrunkRepo(t) + chdir(t, repo) + + if got := remoteDefault(); got != "" { + t.Fatalf("nothing should resolve without origin/HEAD or main/master, got %q", got) + } + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + if m.riskBase != "" { + t.Fatalf("no base should resolve, got %q", m.riskBase) + } + + b := find(m.branches, "feature/unmerged") + if b == nil { + t.Fatalf("feature/unmerged missing; loaded %v", branchNames(m.branches)) + } + b.selected = true + m.force = true + m.measureSelectedRisk() + if b.riskCommits != 0 { + t.Fatalf("nothing can be measured without a base, got %d", b.riskCommits) + } + + // A -D with nothing to measure against must say so rather than imply safety. + m.state = stateConfirm + if out := stripANSI(m.confirmView()); !strings.Contains(out, "no base branch to compare against") { + t.Fatalf("confirm view must carry the warning:\n%s", out) + } + + // The same holds on the force prompt reached after a refused safe delete. + m.force = false + m.performDeletions() + if len(m.forceableFailures()) != 1 { + t.Fatalf("the unmerged branch should be refused and forceable: %+v", m.results) + } + m.state = stateForcePrompt + 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) + } + }) + + // A clone sets origin/HEAD, which is the only thing that can name a default + // branch the tool would never guess. This is remoteDefault's symbolic-ref path. + t.Run("origin_head_resolves", func(t *testing.T) { + repo := setupTrunkRepo(t) + chdir(t, repo) + git(t, repo, "remote", "set-head", "origin", "trunk") + + if got := remoteDefault(); got != "refs/remotes/origin/trunk" { + t.Fatalf("origin/HEAD should name the default, got %q", got) + } + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + if m.riskBase != "origin/trunk" || m.riskBaseRef != "refs/remotes/origin/trunk" { + t.Fatalf("model should cache both forms: %q / %q", m.riskBase, m.riskBaseRef) + } + b := find(m.branches, "feature/unmerged") + b.selected = true + m.measureSelectedRisk() + if b.riskCommits != 1 { + t.Fatalf("risk should now be measurable against origin/trunk, got %d", b.riskCommits) + } + }) +} + +// The remote is still configured but unreachable — the server moved, the repo was +// deleted, or the laptop is offline. Remote-tracking refs are local, so everything +// on screen still resolves; only the operations that touch the network fail, and +// they must fail loudly without taking the local delete down with them. +func TestUnreachableRemote(t *testing.T) { + repo := setupRepo(t) + chdir(t, repo) + git(t, repo, "remote", "set-url", "origin", t.TempDir()+"/gone.git") + + m, err := initialModel() + if err != nil { + t.Fatalf("initialModel must work against a dead remote: %v", err) + } + tb := find(m.branches, "feature/tracked") + if tb == nil || tb.upstream != "origin/feature/tracked" { + t.Fatalf("the upstream is recorded locally and must survive: %+v", tb) + } + + // A fetch is the first thing to fail, and the error must reach the user. + nm, _ := m.Update(fetchPruneCmd()()) + m = nm.(model) + if m.err == "" { + t.Fatal("a failed fetch must be reported") + } + if m.fetching { + t.Fatal("the fetch flag must clear so p can be pressed again") + } + + // The local delete still succeeds; the armed push fails and is reported. + tb = find(m.branches, "feature/tracked") + tb.selected = true + tb.deleteRemote = true + m.performDeletions() + + r := m.results[0] + if !r.localOK { + t.Fatalf("the local delete must still land: %s", r.localErr) + } + if !r.remoteTried || r.remoteOK || r.remoteErr == "" { + t.Fatalf("the push must have been tried, failed, and captured: %+v", r) + } + if r.remoteSkipped { + t.Fatalf("the push was attempted, not deferred: %+v", r) + } + out := stripANSI(m.resultView()) + if !strings.Contains(out, "deleted local feature/tracked") { + t.Fatalf("results must report the successful local delete:\n%s", out) + } + if !strings.Contains(out, "remote feature/tracked:") { + t.Fatalf("results must report the failed push:\n%s", out) + } +} + +// setupDiverged gives feature/tracked commits its upstream lacks and the upstream +// a commit it lacks, so git reports "[ahead 2, behind 1]" — ahead by the "b" it +// already carried from setupRepo plus "mine", behind by "theirs". +func setupDiverged(t *testing.T, repo string) { + t.Helper() + git(t, repo, "checkout", "-q", "feature/tracked") + commitFile(t, repo, "mine", "mine") + // Publish a different commit as the upstream's tip, from a branch that never + // touches the local one. + git(t, repo, "checkout", "-q", "-b", "theirs", "main") + commitFile(t, repo, "theirs", "theirs") + git(t, repo, "push", "-qf", "origin", "theirs:feature/tracked") + git(t, repo, "checkout", "-q", "main") + git(t, repo, "branch", "-qD", "theirs") + git(t, repo, "fetch", "-q", "--all", "--prune") +} + +// Both halves of the track string come from one git field, but only the ahead +// half has ever been exercised against real git output. +func TestDivergedBranch(t *testing.T) { + repo := setupRepo(t) + chdir(t, repo) + setupDiverged(t, repo) + + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + b := find(m.branches, "feature/tracked") + if b == nil || b.ahead != 2 || b.behind != 1 { + t.Fatalf("want ahead=2 behind=1 parsed from real git: %+v", b) + } + if track := stripANSI(m.trackStr(*b)); !strings.Contains(track, "↑2") || !strings.Contains(track, "↓1") { + t.Fatalf("the track column must show both directions, got %q", track) + } + // Holding a commit its upstream lacks, it is not safely deletable. + if b.safeDeletable() { + t.Fatalf("a branch ahead of its upstream must not be safely deletable: %+v", b) + } + + // sortAheadBehind orders on (ahead - behind), which nets to 0 here. + m.field = sortAheadBehind + m.ascending = true + m.sortBranches() + if len(m.branches) != 4 { + t.Fatalf("the sort must keep every branch: %v", branchNames(m.branches)) + } +} + +// Someone else deleted the remote branch between the last fetch and this delete. +// Nothing local says so, so the push is still attempted — and because the refspec +// is qualified, git treats deleting an already-absent ref as a no-op rather than +// an error. The end state is exactly what the user armed, so this reports success. +// (A bare refspec would fail here with "remote ref does not exist", but it also +// silently deletes nothing when a tag shadows the branch — see +// TestTagShadowingBranchName/remote_delete_refspec. Idempotence is the better +// trade.) +func TestRemoteDeleteRace(t *testing.T) { + repo := setupRepo(t) + chdir(t, repo) + + // Delete it straight on the bare remote, leaving our remote-tracking ref stale + // — `push --delete` from here would prune that ref and give the game away. + out, err := runGit("remote", "get-url", "origin") + if err != nil { + t.Fatal(err) + } + git(t, strings.TrimSpace(out), "branch", "-qD", "feature/tracked") + + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + tb := find(m.branches, "feature/tracked") + if tb == nil || tb.gone || tb.upstream != "origin/feature/tracked" { + t.Fatalf("without a fetch the branch still looks healthy: %+v", tb) + } + tb.selected = true + tb.deleteRemote = true + m.performDeletions() + + r := m.results[0] + if !r.localOK { + t.Fatalf("the local delete must still succeed: %s", r.localErr) + } + if !r.remoteTried || !r.remoteOK { + t.Fatalf("the already-absent remote must resolve cleanly: %+v", r) + } + if remoteHasBranch(t, repo, "feature/tracked") { + t.Fatal("the remote branch must be absent afterwards") + } + if o := stripANSI(m.resultView()); !strings.Contains(o, "deleted remote feature/tracked") { + t.Fatalf("results must report the remote as dealt with:\n%s", o) + } +} + +// The checked-out branch cannot be deleted, so no key may ever mark it. +func TestCurrentBranchCannotBeMarked(t *testing.T) { + repo := setupRepo(t) + chdir(t, repo) + + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + for i := range m.branches { + if m.branches[i].isCurrent { + m.cursor = i + } + } + if m.cur() == nil || !m.cur().isCurrent { + t.Fatal("precondition: the cursor should sit on the current branch") + } + for _, k := range []string{" ", "r"} { + nm, _ := m.updateList(key(k)) + m = nm.(model) + if b := m.cur(); b.selected || b.deleteRemote { + t.Fatalf("%q marked the current branch: %+v", k, b) + } + } + + // Select-all spares it too. + nm, _ := m.updateList(key("a")) + m = nm.(model) + if got, want := len(m.selectedBranches()), len(m.branches)-1; got != want { + t.Fatalf("select-all should take %d of %d branches, got %d", want, len(m.branches), got) + } + for _, b := range m.selectedBranches() { + if b.isCurrent { + t.Fatalf("select-all must spare the current branch: %+v", b) + } + } +} + +// With no remote default and no local main/master, there is nothing left to diff +// against but HEAD. +func TestLoadDiffFallsBackToHead(t *testing.T) { + repo := setupTrunkRepo(t) // default branch "trunk", origin/HEAD unset + chdir(t, repo) + + if got := baseBranch("feature/unmerged"); got != "" { + t.Fatalf("no base should resolve, got %q", got) + } + diff, base, err := loadDiff("feature/unmerged") + if err != nil { + t.Fatalf("loadDiff: %v", err) + } + if base != "HEAD" { + t.Fatalf("the base should fall back to HEAD, got %q", base) + } + if !strings.Contains(diff, "+++ b/b") { + t.Fatalf("the diff against HEAD must still show the branch's work:\n%s", diff) + } + + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + m.diffBranch, m.diffBase = "feature/unmerged", base + m.diffLines = strings.Split(strings.TrimRight(diff, "\n"), "\n") + m.state = stateDiff + if o := stripANSI(m.diffView()); !strings.Contains(o, "vs HEAD") { + t.Fatalf("the diff header must name the base it actually used:\n%s", o) + } +} + +// Legal but unusual content has to survive the NUL-delimited parse: an empty +// commit subject leaves a trailing empty field, which a stricter field count +// would drop the whole branch over, and a multibyte name must reach the renderer +// and the git call intact. +func TestUnicodeNameAndEmptySubject(t *testing.T) { + repo := setupLocalRepo(t) + chdir(t, repo) + + const name = "feature/café-日本語" + git(t, repo, "checkout", "-q", "-b", name) + if err := os.WriteFile(repo+"/c", []byte("c"), 0o644); err != nil { + t.Fatal(err) + } + git(t, repo, "add", "c") + git(t, repo, "commit", "-q", "--allow-empty-message", "-m", "") + git(t, repo, "checkout", "-q", "main") + + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + b := find(m.branches, name) + if b == nil { + t.Fatalf("the branch must survive the parse; loaded %v", branchNames(m.branches)) + } + if b.subject != "" { + t.Fatalf("want an empty subject, got %q", b.subject) + } + if !strings.Contains(stripANSI(m.listView()), "café") { + t.Fatalf("the row must carry the branch name:\n%s", stripANSI(m.listView())) + } + + // And it deletes end to end, the name surviving the round trip to git. + b.selected = true + m.force = true + m.performDeletions() + if !m.results[0].localOK { + t.Fatalf("delete failed: %s", m.results[0].localErr) + } + if find(m.branches, name) != nil { + t.Fatal("the branch should be gone") + } +} + // The version string is well-formed even when VCS build info is absent. func TestVersionString(t *testing.T) { s := versionString() From ff2493d9b6844712acc596949e16ce7b5a761dd5 Mon Sep 17 00:00:00 2001 From: John Bolliger Date: Mon, 17 Aug 2026 22:30:30 -0700 Subject: [PATCH 2/2] Document the test-gap sweep and the ref-handling rule Records the three-tier review of everyday git edge cases the suite did not construct, what each tier found, and the corrections real repos forced on three predictions made before testing them. The "Ref handling" section states the rule the bug fixes established, so the shortest-unambiguous-name trap is not walked into again. Co-Authored-By: Claude Fable 5 --- docs/test-coverage-gaps.md | 215 +++++++++++++++++++++++++++++++++++++ 1 file changed, 215 insertions(+) create mode 100644 docs/test-coverage-gaps.md diff --git a/docs/test-coverage-gaps.md b/docs/test-coverage-gaps.md new file mode 100644 index 0000000..9ecba46 --- /dev/null +++ b/docs/test-coverage-gaps.md @@ -0,0 +1,215 @@ +# git_pruner — test coverage gaps + +An analysis of `main_test.go` (2026-08-17) proposing tests for everyday git edge cases the +suite does not cover. Git-behavior claims below were reproduced in throwaway repositories +before being recorded, following the same rule as `improvements.md`. + +## Current coverage assessment + +The 25 existing tests are strong on the deletion-safety core: safe-vs-force semantics +ground-truthed against real git (`TestSafeDeletableMatchesGit`), gone-branch risk guarding, +deferred remote deletes, the force-retry flow, and rendering width invariants. + +What is missing is almost entirely **environmental variation**: every test runs in the same +happy-path repo shape — clean checkout on `main`, one healthy file-URL remote named +`origin`/`upstream`, born HEAD, no tags. + +--- + +## Tier 1 — expose confirmed or likely bugs — **DONE 2026-08-17** + +### 1. Tag shadowing a branch name *(was a confirmed bug — fixed)* + +A tag sharing a branch's name silently shadowed the branch: git's ref search order puts +`refs/tags/` ahead of `refs/heads/`, and `riskCommitCount` / `loadDiff` passed +bare names to `git cherry` and `git diff`. Reproduced: `git cherry main feat` reported +**zero** at-risk commits for a branch that had one, with only a `refname 'feat' is +ambiguous` warning on stderr — so the risk warning said nothing was at stake on exactly the +branch a `-D` was about to destroy. Tagging a release branch with its own name (`v1.2`, +`release-3`) makes this an everyday shape. + +Writing the test exposed a **deeper root cause than the bare-name calls**: `loadBranches` +read `%(refname:short)`, which returns the shortest *unambiguous* name. The moment the tag +exists, the branch loads as `heads/feature/x` rather than `feature/x`, which breaks every +name-keyed lookup and any ref rebuilt from the name. `%(upstream:short)` has the same +failure, disambiguating to `remotes/origin/x` and splitting into a bogus remote named +`remotes`. + +Fixed by reading full refs and stripping the namespace ourselves: + +- `shortRef()` strips `refs/heads/` / `refs/remotes/`; `loadBranches` now reads + `%(refname)` and `%(upstream)`, and `mergedSet` reads `%(refname)`. +- `branchRef()` qualifies a local branch name for git; used by `riskCommitCount` and + `loadDiff`. +- `qualifyRef()` probes `refs/heads/` then `refs/remotes/` for refs whose namespace is not + known statically (the base). Resolved once into the new `model.riskBaseRef`, since it + costs up to two `rev-parse` calls. + +`riskBase` stays short for display; git is handed the qualified form. `git branch -d` was +never affected — it always operates in `refs/heads/`. + +Covered by `TestTagShadowingBranchName` (`branch_name`, `upstream_name`, `base_name`). + +### 2. Selections survive `p` *(was a real wipe — fixed)* + +`fetchDoneMsg` called `applyBranches` with freshly loaded structs, so every manually +selected / `R`-armed branch was silently wiped when the user pressed `p`. A fetch mutates +nothing local, so the marks now survive it: `carryMarks()` copies `selected` and +`deleteRemote` onto the new set by name. + +One deliberate exception: an armed remote delete is **dropped** when the fetch reveals the +upstream is gone, because the push it would run can only fail. + +Covered by `TestFetchPreservesSelections` and `TestFetchDisarmsRemoteForGoneBranch`. + +### 3. Gone *current* branch *(behavior was already correct — now locked in)* + +The user is sitting on `feature/x`, the PR merged, the remote branch was deleted, and they +press `p`. The fetch handler's `isCurrent` guard already kept it out of auto-selection and +out of the "press d to prune" count, but nothing tested it. The test also pins the +second line of defence: selected by hand, git refuses the delete, and because the refusal +is not "not fully merged" it correctly does **not** raise the force prompt. + +Covered by `TestGoneCurrentBranchIsNotPruned`. + +--- + +## Tier 2 — everyday repo environments never constructed in tests — **DONE 2026-08-17** + +No new production bugs here: every shape already behaved correctly, and the tests now pin +that. Two of the predictions above were wrong about git's behavior and were corrected +against real repos before being written up. + +### 4. No remotes at all *(covered by `TestLocalOnlyRepo`)* + +A scratch or never-pushed project. `initialModel` works, `riskBase` falls back to local +`main` (with `riskBaseRef` `refs/heads/main`), `loadDiff` bases on local `main`, the risk +warning still names the real cost, and `p` succeeds as a no-op — `git fetch --all --prune` +exits 0 with no remotes configured. + +### 5. Unborn HEAD *(covered by `TestUnbornHeadRepo`)* + +`git init`, zero commits: empty branch list, no base resolves, `listView` renders the empty +message, and every cursor/selection key is a no-op rather than a panic. `git branch --merged +HEAD` hard-fails here (`fatal: malformed object name HEAD`); `mergedSet` absorbs it. + +### 6. Detached HEAD *(covered by `TestDetachedHead`)* + +Mid-bisect, mid-rebase, or on a checked-out tag. No branch is `isCurrent`, so `a` selects +everything — there is no current branch to spare — and `headMerged` is computed against the +detached commit. Deleting the branch HEAD is parked on is legal and safe: the commits stay +reachable from HEAD. + +Noted: `git branch --merged HEAD --format=%(refname)` emits a `(HEAD detached at …)` +pseudo-entry, which lands in `mergedSet` as a junk key. Harmless — branch names cannot +contain spaces, so it can never collide with a real lookup. + +### 7. Remote default is neither `main` nor `master`, `origin/HEAD` unset *(covered by `TestNonStandardDefaultBranch`)* + +E.g. `trunk`/`develop`. `origin/HEAD` only exists after a clone, not after `remote add` + +push — verified: `symbolic-ref refs/remotes/origin/HEAD` fails with `not a symbolic ref`. +With no `origin/main`, `origin/master`, or local `main`/`master` either, `riskBase == ""`, +which is the path both "no base branch to compare against" messages hang off. Now asserted +in `riskWarning`, `confirmView`, and `forcePromptView`. + +The second subtest covers `remoteDefault`'s `symbolic-ref` path, which had **no coverage at +all** — `setupRepo` never sets `origin/HEAD`. After `git remote set-head origin trunk` it +resolves to `origin/trunk`. + +### 8. Upstream's remote no longer exists *(covered by `TestUnreachableRemote`)* + +**Correction:** the prediction that `git remote remove origin` leaves the branch showing +"gone" is wrong. Removing a remote also unsets `branch..remote`/`merge` and deletes +its remote-tracking refs, so the branches simply become upstream-less — which is case 4, +not a distinct one. + +The case that does exercise the error path is a remote that is still configured but +**unreachable** (server moved, repo deleted, laptop offline). Remote-tracking refs are +local, so the upstream and all merge state survive; only network operations fail. The test +pins that a failed fetch reaches `m.err` and clears `fetching` so `p` can be pressed again, +and that a failed `push --delete` is captured in `remoteErr` and reported on the results +screen without taking the successful local delete down with it. + +This also closes Tier 3 item 11 (fetch failure), which shares the shape. + +--- + +## Tier 3 — everyday operations on the happy repo — **DONE 2026-08-17** + +### 9. Diverged branch (ahead AND behind) *(covered by `TestDivergedBranch`)* + +`trackRe`'s `behind` capture group and `sortAheadBehind` had only ever seen hand-built +structs. Now driven from real `[ahead 2, behind 1]` output, asserting both arrows render and +that a branch ahead of its upstream is not safely deletable. + +### 10. Remote delete race *(covered by `TestRemoteDeleteRace`)* + +**Correction:** the prediction that this surfaces an error is no longer true, because of the +qualified refspec introduced in the simplify pass (below). `git push origin --delete +refs/heads/x` treats an already-absent ref as a no-op and exits 0, where the bare form fails +with "remote ref does not exist". + +That is a deliberate trade: the bare form's error is more informative in this one case, but +the bare form *also silently deletes nothing* when the remote carries a tag of the same name +(git rejects it as "src refspec matches more than one"). Idempotence in the race is worth +more than a message, since the race ends in exactly the state the user armed. The test pins +the idempotent outcome; `TestUnreachableRemote` still covers genuine push failures. + +### 11. Fetch failure — **DONE**, see Tier 2 item 8 + +### 12. Current-branch UI protection *(covered by `TestCurrentBranchCannotBeMarked`)* + +`space`, `r`, and `a` all skipping `isCurrent` was only ever implicit in other tests. Now +explicit: the checked-out branch can never be marked or swept into a select-all. + +### 13. `loadDiff` HEAD fallback *(covered by `TestLoadDiffFallsBackToHead`)* + +Reached with `setupTrunkRepo`, where no remote default and no local `main`/`master` resolve. +Asserts the diff is still correct against HEAD and that the diff header names the base it +actually used. + +### 14. Odd-but-legal content *(covered by `TestUnicodeNameAndEmptySubject`)* + +An empty commit subject (`--allow-empty-message`) leaves a trailing empty field in the +NUL-delimited `for-each-ref` output — a stricter field count would drop the whole branch. +Combined with a multibyte branch name, pinned end to end through parse, render, and delete. + +--- + +## Ref handling — the rule this work established + +**Never use git's `%(refname:short)`, `%(upstream:short)`, or `symbolic-ref --short`.** They +return the shortest *unambiguous* name, which silently grows a `heads/` or `remotes/` prefix +the moment a tag shares the name. Instead: + +- Resolvers return **fully qualified refs**. `remoteDefault`, `localDefaultBranch` and + `baseBranch` all do; resolution is the only point where the namespace is known for + certain, so it is carried forward from there rather than re-guessed later. +- `shortRef()` shortens at the **display boundary** only. The model keeps both forms: + `riskBase`/`remoteDefault` for views, `riskBaseRef` for git. +- `branchRef()` qualifies a local branch name. Correct because `loadBranches` only ever + reads `refs/heads`, so the mapping is total. +- `refExists()` wraps the `rev-parse --verify --quiet` probe and must be given a qualified + ref — a bare name would match a tag, which is the bug it exists to avoid. + +An earlier iteration used a `qualifyRef()` helper that probed both namespaces at the point +of *use*. That was the wrong altitude: it cost up to two subprocesses per call, silently +fell through to the shadowable bare name when both probes missed, and left the real hole +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. + +- Tier 1 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. + +Fixtures compose rather than duplicate: `initRepo` (bare init + identity) → +`setupLocalRepo` (branch shapes) → `setupRepo` (+ `addOrigin` + a tracking branch), with +`setupTrunkRepo` reusing `initRepo` and `addOrigin`.