diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..b255d2c --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,50 @@ +name: CI + +on: + push: + branches: [master] + pull_request: + workflow_dispatch: + +permissions: + contents: read + +jobs: + test: + strategy: + fail-fast: false + matrix: + os: [ubuntu-latest, macos-latest] + runs-on: ${{ matrix.os }} + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-go@v5 + with: + go-version-file: go.mod + cache: true + + # The tests drive a real git binary against throwaway repos, so the runner + # needs a committer identity and a predictable default branch name. + - name: Configure git + run: | + git config --global user.name "CI" + git config --global user.email "ci@example.invalid" + git config --global init.defaultBranch main + + - name: gofmt + run: | + unformatted="$(gofmt -l .)" + if [ -n "$unformatted" ]; then + echo "::error::not gofmt'd:"; echo "$unformatted"; exit 1 + fi + + - run: go build ./... + - run: go vet ./... + - run: go test -race ./... + + # Exercise the no-sudo path of the installer end to end so it cannot rot. + - name: install.sh + run: | + ./install.sh --bindir "$RUNNER_TEMP/bin" + "$RUNNER_TEMP/bin/git_pruner" version diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..2b70acc --- /dev/null +++ b/.gitignore @@ -0,0 +1,3 @@ +# `go build ./...` drops the binary in the repo root; the Makefile installs to +# $(BINDIR) instead. +/git_pruner diff --git a/LICENSE b/LICENSE new file mode 100644 index 0000000..d852600 --- /dev/null +++ b/LICENSE @@ -0,0 +1,21 @@ +MIT License + +Copyright (c) 2026 John Bolliger + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in all +copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +SOFTWARE. diff --git a/Makefile b/Makefile index 51dcaa8..684b16f 100644 --- a/Makefile +++ b/Makefile @@ -1,5 +1,7 @@ BINARY := git_pruner -BINDIR := $(HOME)/shared/bin +# Overridable so `make build/clean BINDIR=...` can target the same directory +# install.sh used; the default is the local dev convention. +BINDIR ?= $(HOME)/shared/bin TARGET := $(BINDIR)/$(BINARY) .PHONY: build install test vet clean diff --git a/README.md b/README.md index c63d367..dc60d21 100644 --- a/README.md +++ b/README.md @@ -13,9 +13,14 @@ whose upstream has been deleted so they can be cleaned up in one step. Requires Go 1.26+ and git on your PATH. ```sh -make build # builds to ~/shared/bin/git_pruner (on PATH) +./install.sh # build + install to a user bin directory +./install.sh --bindir ~/bin # ...or pick the directory yourself ``` +With no flags it installs to the first usable of `~/.local/bin` or `~/bin`, falling back to +`/usr/local/bin` (via sudo), and warns if the directory is not on your PATH. See +`./install.sh --help` for the details. + Then run it from inside any git repository: ```sh @@ -38,7 +43,7 @@ git_pruner version # also --version, -v | `a` / `n` | Select all / clear selection | | `r` | Toggle "also delete remote" for the row (needs an upstream) | | `v` | View the branch's diff (green additions / red removals) | -| `p` | Fetch `--all --prune`, then select branches whose upstream is gone | +| `p` | Fetch `--all --prune`, then select gone branches that hold no unique work | | `s` | Cycle sort field: committerdate -> name -> ahead/behind | | `o` | Reverse sort direction | | `f` | Toggle delete mode: safe `-d` <-> force `-D` | @@ -67,22 +72,30 @@ In the diff view: `↑`/`↓` scroll, `space`/`ctrl+d` page down, `ctrl+u`/`pgup - `>` cursor, `[x]` selected, `R` remote deletion armed, `*` current branch - ahead/behind shown as `↑N↓M` (`=` when in sync, `gone` in red when the upstream was deleted) - a green `✓` after the track column means the upstream is merged into the remote default - branch (`origin/HEAD`, else `origin/main`/`origin/master`) — i.e. the remote is safe to delete + branch — i.e. the remote is safe to delete - relative commit date, short hash, and commit subject ## Viewing a branch's changes Press `v` to see what a branch contains as a colorized patch — **green** for additions, **red** for removals, magenta hunk headers. The diff is computed against the repository's default branch -(`origin/HEAD`, falling back to `main`, then `master`) using a three-dot diff +(see [Resolving the default branch](#resolving-the-default-branch)) using a three-dot diff (`git diff ...`), so it shows only the changes introduced on that branch since it diverged. The view is scrollable for large diffs; the header shows which base it was compared to. +## Resolving the default branch + +The default branch is used as the diff base, as the merge target for the `✓` indicator, and to +measure what a force delete would discard. It resolves to `/HEAD` if set, else +`/main`, else `/master`, trying each configured remote in turn with `origin` +first — so repositories whose only remote is named something else (`upstream`, a fork, …) still +get merge information. If no remote resolves, a local `main`/`master` is used. + ## Pruning gone branches Press `p` to run `git fetch --all --prune` in the background (the UI stays responsive). Once it -finishes, any local branch whose upstream was deleted is marked **gone** and automatically -selected, and a status line reports how many were found. Press `d` to review and delete them. +finishes, any local branch whose upstream was deleted is marked **gone**, and a status line +reports what was found. Press `d` to review and delete them. This is the interactive equivalent of: @@ -93,10 +106,26 @@ git fetch --all --prune && git branch -vv | awk '/: gone]/{print $1}' | xargs gi Gone branches are always removed with `git branch -D` (force), since `-d` refuses a branch whose upstream no longer exists — this is why selecting them via `p` prunes them even in safe mode. +Because `-D` discards unmerged commits and git reports **no ahead/behind count for a gone +branch**, git_pruner measures each one against the default branch with `git cherry` and counts +the commits that have no equivalent patch there: + +- gone branches holding **no** such commits are auto-selected by `p` — the one-keystroke workflow +- gone branches that **do** hold unique commits are left unselected and reported in the status + line, so discarding them takes a deliberate `space`; the confirmation screen then shows + `⚠ N commit(s) not in — force delete (-D) will discard them` + +`git cherry` is used rather than `git rev-list ..` so commits that were +cherry-picked, rebased, or squashed individually into the base are correctly recognized as +already integrated. A group of commits squashed together into one still counts as unique, since +no single equivalent patch exists — which is why the warning reads "not in ``" rather than +claiming the work is unrecoverable. + ## Deletion behavior - Local: `git branch -d` by default (refuses unmerged branches); `f` switches to `git branch -D`. - Branches whose upstream is **gone** are always deleted with `-D`, regardless of the mode. + Branches whose upstream is **gone** are always deleted with `-D`, regardless of the mode, and + the confirmation screen flags any commits that would be discarded (see above). When a `-d` delete is refused for being unmerged, a follow-up prompt lets you retry those branches with `-D` without leaving the results — no need to back out and re-select. - Remote: when armed with `r`, runs `git push --delete `, where the remote is @@ -111,7 +140,22 @@ upstream no longer exists — this is why selecting them via `p` prunes them eve ## Development ```sh +make build # build straight to $BINDIR (default ~/shared/bin), skipping install.sh make test # go test ./... make vet # go vet ./... -make clean # remove the installed binary +make clean # remove the binary from $BINDIR ``` + +CI runs `gofmt`, `go build`, `go vet`, and `go test -race` on Linux and macOS for every push to +`master` and every pull request (`.github/workflows/ci.yml`). + +[`docs/improvements.md`](docs/improvements.md) records the codebase analysis, the reasoning behind +the current safety behavior, and the roadmap of remaining work. + +The test suite drives a real `git` binary against throwaway repositories created per test, so it +needs `git` on `PATH` and a committer identity (`user.name` / `user.email`); the tests set one +inside each temporary repo. + +## License + +[MIT](LICENSE) diff --git a/docs/improvements.md b/docs/improvements.md new file mode 100644 index 0000000..0c11ae6 --- /dev/null +++ b/docs/improvements.md @@ -0,0 +1,141 @@ +# git_pruner — analysis and improvement roadmap + +A deep review of the codebase (2026-07-26), the fixes that came out of it, and the work that +remains. Each finding was reproduced in a throwaway repository before being recorded here; +findings that did **not** survive testing are listed at the bottom so they are not re-litigated. + +## Design assessment + +The safety model is the strongest part of this codebase and should be preserved as it evolves: + +- a confirmation screen that itemizes every branch before anything is deleted +- a deliberate `y` (local) vs `R` (local + remote) split, so remote deletion is never one + accidental keystroke +- `-d` → `-D` escalation via an explicit prompt rather than silent forcing +- merge status computed from local remote-tracking refs, so it needs no network +- deletions run off the update loop with a live per-branch checklist + +The findings below are mostly about places where that model had a gap, not about its design. + +--- + +## Completed + +### Tier 1 (all done) + +**1. Gone branches could silently discard unpushed commits.** *(the significant one)* + +`%(upstream:track)` reports a bare `[gone]` with **no ahead count**, so `branch.ahead` parsed to +`0`. The chain: `p` auto-selected every gone branch → `confirmView`'s unmerged warning was gated +on `!br.gone && br.ahead > 0` and so never fired → `deleteFlag` returned `-D` unconditionally → +`y` destroyed the commits. The `-d`→`-D` force prompt never fired either, because `-D` succeeds +on the first try. Every other destructive path in the tool warns; this one — the headline `p` +workflow — did not. + +Reproduced with a branch that was pushed, had its remote deleted, then accumulated local commits: + +``` +feature/important [origin/feature/important: gone] track=[gone] → ahead parsed as 0 +git rev-list --count main..feature/important → 2 commits destroyed, no warning +``` + +Fixed by `riskCommitCount`, which measures each gone branch against the default branch. Gone +branches with no unique commits are still auto-selected by `p` (the one-keystroke workflow is +intact); ones holding unique commits are left unselected, reported in the status line, and +flagged on the confirmation screen. + +*Why `git cherry` rather than `git rev-list ..`:* both were measured against a +squash-merged branch, a single-commit squash, and genuinely unmerged work: + +| branch | `rev-list --count` | `git cherry` `+` lines | +| --------- | ------------------ | ---------------------- | +| squashed (2 commits → 1) | 2 | 2 | +| single-commit squash | 1 | **0** | +| genuinely unmerged | 1 | 1 | + +`git cherry` is strictly more accurate at the same cost — it recognizes cherry-picked, rebased, +and singly-squashed work as already integrated. It cannot detect a *group* squash, and nothing +cheap can. That residual over-report is why the warning is worded `N commit(s) not in ` +rather than claiming the work is unrecoverable. + +**2. `truncate` sliced bytes, emitting invalid UTF-8.** `s[:w-1]` split multibyte runes: + +``` +truncate("日本語のコミットです", 9) → "日本\xe8\xaa…" validUTF8 = false +truncate("日本語のコミットです", 11) → "日本語\xe3…" validUTF8 = false +``` + +Byte length also is not display width, so wide (CJK/emoji) columns were mis-sized in both +directions. Fixed with `ansi.Truncate` plus a new `pad` helper; `recomputeNameWidth` now measures +cells via `ansi.StringWidth`. + +**3. The `gone` track value was 8 cells wide where every other value was 10**, shifting every +column after it on exactly the rows the user is there to act on. The regression test was verified +to fail against the old code (`date column at cell 31, want 33`) before being kept. + +**4. Default-branch resolution hardcoded `origin`.** On a repo whose only remote was `upstream`, +`remoteDefault()` returned `""` and the `✓ merged` indicator plus the confirm-screen merge line +silently vanished — no error, the safety signal simply was not there. `remotes()` now tries every +configured remote with `origin` ordered first. + +### Also completed + +- `LICENSE` (MIT) +- `.github/workflows/ci.yml` — gofmt, `go build`, `go vet`, `go test -race` on Linux and macOS +- `.gitignore` — `go build ./...` drops a binary in the repo root + +--- + +## Remaining work + +### Tier 2 — robustness + +**5. Blocking git calls inside `Update`.** `loadDiff` (`v`), `refreshMergeInfo`, and +`reloadBranches` run synchronously in the update loop. `git branch -r --merged` is +O(remote refs × history) and runs on *every* reload; on a repo with thousands of remote branches +the UI freezes. The `tea.Cmd` pattern already works for fetch — reuse it. Note that +`refreshMergeInfo` now also issues one `git cherry` per gone branch, which raises the stakes. + +**6. `runGit` has no timeout and does not disable terminal prompts.** `fetch --all --prune` and +`push --delete` are network-bound; a credential or SSH prompt hangs the TUI with no recovery. +Set `GIT_TERMINAL_PROMPT=0` and attach a `context.WithTimeout` so it fails fast instead. + +**7. The tested delete path is not the one users run.** `performDeletions` is test-only by its own +comment; the live async path's completion logic — `branchDeletedMsg` → `deletesDone` → the +`stateForcePrompt` / `stateResult` transition — is never fed through `Update` in any test. The +riskiest state machine in the program is the untested one. Port the tests to the async path and +delete `performDeletions`. + +**8. Smaller items.** +- `listView` runs one line over terminal height when `status` and `err` are both set + (`visibleRows` is `height-5`; actual emission is `height+1`). +- ANSI and control characters in commit subjects and branch names render raw into the terminal. +- `applyBranches` silently discards the user's existing selections on `p`. +- `stateDeleting`'s ctrl+c quits while `git push --delete` children are still running. + +### Tier 3 — features for the tool's actual job + +**9. `/` incremental filter.** With dozens of branches there is currently no way to narrow the +list — the single biggest UX gap for the repos this tool exists to clean up. + +**10. Bulk-select predicates** (merged, older than N days). "Select everything merged and older +than 90 days" is the canonical prune workflow and currently has to be done by hand. + +**11. Reflog recovery hint after a `-D`.** The force-prompt screen says "permanently discard their +unmerged commits" without telling the user that `git reflog` can still recover them. Pairs +naturally with finding 1. + +### Tier 4 — hygiene + +**12. Split `main.go`** (~1,300 lines) into `git.go` / `model.go` / `view.go`. + +**13. Make the Makefile's `BINDIR` overridable** — it hardcodes `$HOME/shared/bin`. + +--- + +## Investigated and rejected + +**Concurrent `git branch -d` racing on `packed-refs.lock`.** `tea.Batch` runs deletions +concurrently, which looked like it should collide on the packed-refs lock. Tested with 60 parallel +deletes against a freshly packed repo: **all 60 succeeded.** Git's ref-lock retry handles it. No +change needed — recorded so it is not re-investigated. diff --git a/go.mod b/go.mod index 62901ca..1332afb 100644 --- a/go.mod +++ b/go.mod @@ -5,12 +5,12 @@ go 1.26.2 require ( github.com/charmbracelet/bubbletea v1.3.10 github.com/charmbracelet/lipgloss v1.1.0 + github.com/charmbracelet/x/ansi v0.10.1 ) require ( github.com/aymanbagabas/go-osc52/v2 v2.0.1 // indirect github.com/charmbracelet/colorprofile v0.2.3-0.20250311203215-f60798e515dc // indirect - github.com/charmbracelet/x/ansi v0.10.1 // indirect github.com/charmbracelet/x/cellbuf v0.0.13-0.20250311204145-2c3ea96c31dd // indirect github.com/charmbracelet/x/term v0.2.1 // indirect github.com/erikgeiser/coninput v0.0.0-20211004153227-1c3628e74d0f // indirect diff --git a/install.sh b/install.sh new file mode 100755 index 0000000..46189c0 --- /dev/null +++ b/install.sh @@ -0,0 +1,133 @@ +#!/bin/sh +# Build git_pruner and install it onto the user's PATH. Run with --help for flags. +set -eu + +BINARY=git_pruner +BINDIR=${BINDIR:-} + +usage() { + cat <&2; exit 2; } + BINDIR=$2 + shift 2 + ;; + --bindir=*) + BINDIR=${1#--bindir=} + shift + ;; + -h | --help) + usage + exit 0 + ;; + *) + printf '%s\n' "install.sh: unknown argument: $1" >&2 + usage >&2 + exit 2 + ;; + esac +done + +command -v go >/dev/null 2>&1 || { + printf '%s\n' "install.sh: go is not on PATH (see https://go.dev/dl/)" >&2 + exit 1 +} +# git_pruner shells out to git for every operation, so a missing git makes the +# installed binary useless. +command -v git >/dev/null 2>&1 || { + printf '%s\n' "install.sh: git is not on PATH" >&2 + exit 1 +} + +# Usable means writable without sudo: the directory is writable, or it is +# missing and the nearest existing ancestor is writable (mkdir -p creates the +# rest). Clobbers _dir/_parent, since POSIX sh has no `local`. +dir_is_usable() { + _dir=$1 + while [ ! -d "$_dir" ]; do + _parent=$(dirname "$_dir") + if [ "$_parent" = "$_dir" ]; then + return 1 # walked up to / without finding an existing ancestor + fi + _dir=$_parent + done + [ -w "$_dir" ] +} + +# HOME is unset in some headless and sudo environments, and set -u would abort +# on "$HOME"; skipping straight to the fallback is the right answer there. +if [ -z "$BINDIR" ] && [ -n "${HOME:-}" ]; then + for candidate in "$HOME/.local/bin" "$HOME/bin"; do + if dir_is_usable "$candidate"; then + BINDIR=$candidate + break + fi + done +fi +# No user-owned directory was usable: fall back to the conventional system +# location and let the sudo path below deal with the permissions. +[ -n "$BINDIR" ] || BINDIR=/usr/local/bin + +# Resolve a relative BINDIR against the caller's cwd before the cd below moves us. +case $BINDIR in +/*) ;; +*) BINDIR=$PWD/$BINDIR ;; +esac + +# Build from the repo root regardless of the caller's cwd; the build must run +# inside the git checkout for Go's VCS stamping (git_pruner version). CDPATH is +# cleared because an exported one would redirect this cd. +# shellcheck disable=SC1007 # the empty CDPATH= is a deliberate command prefix +CDPATH= cd -- "$(dirname -- "$0")" + +tmpdir=$(mktemp -d) +# POSIX signal traps resume execution afterwards, so the signal handlers have to +# exit themselves; EXIT then does the cleanup exactly once. +trap 'rm -rf "$tmpdir"' EXIT +trap 'exit 130' INT +trap 'exit 129' HUP +trap 'exit 143' TERM + +printf '%s\n' "Building $BINARY..." +go build -o "$tmpdir/$BINARY" . + +SUDO= +if ! dir_is_usable "$BINDIR"; then + command -v sudo >/dev/null 2>&1 || { + printf '%s\n' "install.sh: $BINDIR is not writable and sudo is unavailable" >&2 + exit 1 + } + SUDO=sudo + printf '%s\n' "$BINDIR needs elevated permissions; using sudo." +fi + +# Unquoted on purpose: empty $SUDO must expand to zero words, not an empty one. +$SUDO mkdir -p "$BINDIR" +$SUDO install -m 0755 "$tmpdir/$BINARY" "$BINDIR/$BINARY" + +printf '%s\n' "Installed $BINDIR/$BINARY" + +case ":$PATH:" in +*":$BINDIR:"*) ;; +*) + printf '\n%s\n' "Warning: $BINDIR is not on your PATH. Add this to your shell profile:" + printf '%s\n' " export PATH=\"$BINDIR:\$PATH\"" + ;; +esac diff --git a/main.go b/main.go index 22c2a26..38dfd42 100644 --- a/main.go +++ b/main.go @@ -13,6 +13,7 @@ import ( tea "github.com/charmbracelet/bubbletea" "github.com/charmbracelet/lipgloss" + "github.com/charmbracelet/x/ansi" ) // branch holds the metadata git_pruner displays and acts on for one local branch. @@ -27,6 +28,9 @@ type branch struct { behind int 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 + riskMeasured bool // riskCommits has been computed (0 is a meaningful value) isCurrent bool selected bool deleteRemote bool @@ -46,6 +50,26 @@ func (b branch) remoteBranch() string { return b.name } +// safeDeletable reports whether `git branch -d` will accept b, mirroring git's +// rule: a resolvable upstream is the sole criterion (ahead == 0 means the branch +// holds no commit the upstream lacks), and HEAD is consulted only when there is +// no upstream to ask. This is a precedence, not an either-or — git refuses a +// branch ahead of its upstream even when HEAD already contains it. Note that a +// branch with no upstream always has ahead == 0, git reporting no track info for +// it, so ahead alone cannot answer this. +func (b branch) safeDeletable() bool { + if b.upstream != "" && !b.gone { + return b.ahead == 0 + } + return b.headMerged +} + +// forcedDelete reports whether b will be deleted with -D rather than -d. Gone +// branches always are: git reports no track info for them, so a safe delete +// would turn on HEAD alone and refuse branches whose work is in the remote +// default but not in the local checkout — the headline prune case. +func (b branch) forcedDelete(force bool) bool { return force || b.gone } + type sortField int const ( @@ -80,15 +104,18 @@ const ( ) type deleteResult struct { - name string - ahead int // commits the branch was ahead of upstream (for the force prompt) - done bool // the async deletion for this branch has completed + br branch // the branch this deletion was run for + done bool // the async deletion for this branch has completed localOK bool localErr string forceable bool // a safe (-d) delete failed and could be retried with -D remoteTried bool remoteOK bool remoteErr string + // remoteSkipped records that the armed push was deliberately deferred + // because the local delete failed — the one piece of state not derivable + // from br, since arming is the caller's decision. + remoteSkipped bool } type model struct { @@ -105,6 +132,7 @@ type model struct { results []deleteResult remoteDefault string // resolved remote default branch, e.g. "origin/main" + riskBase string // ref that branch.riskCommits is measured against ("" if unresolved) spinnerFrame int // animation frame for the deleting spinner (deletion counts derive from results) @@ -147,6 +175,10 @@ var ( func runGit(args ...string) (string, error) { cmd := exec.Command("git", args...) + // Pin the locale: deleteBranch classifies failures by matching git's own + // error text, which gettext would otherwise translate. Everything else we + // parse is --format-driven and unaffected. + cmd.Env = append(os.Environ(), "LC_ALL=C") var out, errBuf strings.Builder cmd.Stdout = &out cmd.Stderr = &errBuf @@ -206,16 +238,29 @@ func loadBranches() ([]branch, error) { return branches, nil } -// baseBranch returns a reference to diff a branch against: the repo's default -// branch (origin/HEAD, else main, else master), excluding name itself. -func baseBranch(name string) string { - if out, err := runGit("symbolic-ref", "--short", "refs/remotes/origin/HEAD"); err == nil { - if s := strings.TrimSpace(out); s != "" && s != name { - return s +// remotes lists the configured remotes with "origin" first, so the conventional +// remote wins when several exist while repos whose only remote is named +// something else (upstream, fork, …) still resolve a default branch. +func remotes() []string { + out, err := runGit("remote") + if err != nil { + return nil + } + var names []string + for _, line := range strings.Split(out, "\n") { + if s := strings.TrimSpace(line); s != "" { + names = append(names, s) } } + sort.SliceStable(names, func(i, j int) bool { return names[i] == "origin" && names[j] != "origin" }) + return names +} + +// localDefaultBranch returns a local main/master, skipping exclude so a branch +// is never compared against itself. Returns "" when neither exists. +func localDefaultBranch(exclude string) string { for _, c := range []string{"main", "master"} { - if c == name { + if c == exclude { continue } if _, err := runGit("rev-parse", "--verify", "--quiet", c); err == nil { @@ -226,31 +271,62 @@ func baseBranch(name string) string { } // remoteDefault resolves the remote's default branch as a remote-tracking ref -// (e.g. "origin/main"): origin/HEAD if set, else origin/main, else origin/master. -// Returns "" when none can be determined. +// (e.g. "origin/main"): /HEAD if set, else /main, else +// /master, trying each remote in turn. Returns "" when none can be found. func remoteDefault() string { - if out, err := runGit("symbolic-ref", "--short", "refs/remotes/origin/HEAD"); err == nil { - if s := strings.TrimSpace(out); s != "" { - return s + for _, r := range remotes() { + if out, err := runGit("symbolic-ref", "--short", "refs/remotes/"+r+"/HEAD"); err == nil { + if s := strings.TrimSpace(out); s != "" { + return s + } } - } - for _, c := range []string{"origin/main", "origin/master"} { - if _, err := runGit("rev-parse", "--verify", "--quiet", "refs/remotes/"+c); err == nil { - return c + for _, c := range []string{r + "/main", r + "/master"} { + if _, err := runGit("rev-parse", "--verify", "--quiet", "refs/remotes/"+c); err == nil { + return c + } } } return "" } -// 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. -func remoteMergedSet(def string) map[string]bool { - set := map[string]bool{} - if def == "" { - return set +// baseBranch returns a reference 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 { + return def + } + return localDefaultBranch(name) +} + +// riskCommitCount counts commits on name whose patch is not already present in +// base — the work a force delete (-D) would discard. Uses `git cherry` rather +// than `rev-list base..name` so commits that were cherry-picked, rebased, or +// squashed singly into base are correctly seen as already integrated. Commits +// 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 { + if base == "" || base == name { + return 0 + } + out, err := runGit("cherry", base, name) + if err != nil { + return 0 + } + n := 0 + for _, line := range strings.Split(out, "\n") { + if strings.HasPrefix(line, "+") { // '+' = no equivalent patch in base + n++ + } } - out, err := runGit("branch", "-r", "--merged", def, "--format=%(refname:short)") + return n +} + +// mergedSet runs a `git branch --merged` query and collects the short ref names +// it reports into a set. +func mergedSet(args ...string) map[string]bool { + set := map[string]bool{} + out, err := runGit(append(args, "--format=%(refname:short)")...) if err != nil { return set } @@ -262,6 +338,21 @@ func remoteMergedSet(def string) map[string]bool { 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. +func remoteMergedSet(def string) map[string]bool { + if def == "" { + return map[string]bool{} + } + return mergedSet("branch", "-r", "--merged", def) +} + +// localMergedSet returns the local branches whose tip is merged into HEAD — +// git's criterion for accepting `branch -d` on a branch with no upstream. One +// git call covers the whole list, so this costs nothing per branch. +func localMergedSet() map[string]bool { return mergedSet("branch", "--merged", "HEAD") } + // fetchDoneMsg reports completion of an async `git fetch --all --prune`. type fetchDoneMsg struct{ err error } @@ -293,8 +384,7 @@ var spinnerFrames = []string{"⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", " // independently and concurrently under tea.Batch. func deleteBranchCmd(idx int, b branch, flag string, wantRemote bool) tea.Cmd { return func() tea.Msg { - res := deleteBranch(b.name, flag, wantRemote, b.remoteName(), b.remoteBranch(), b.ahead) - return branchDeletedMsg{idx: idx, res: res} + return branchDeletedMsg{idx: idx, res: deleteBranch(b, flag, wantRemote)} } } @@ -430,18 +520,31 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { if branches, err := loadBranches(); err == nil { m.applyBranches(branches) // preserves the cursor by name (fetch is non-destructive) } - gone := 0 + // Auto-select only gone branches that carry nothing missing from the + // base. Ones holding unique commits are left unselected so discarding + // them stays a deliberate keystroke rather than a side effect of `p`. + gone, risky := 0, 0 for i := range m.branches { - if m.branches[i].gone && !m.branches[i].isCurrent { - m.branches[i].selected = true - gone++ + br := &m.branches[i] + if !br.gone || br.isCurrent { + continue } + gone++ + if br.riskCommits > 0 { + risky++ + continue + } + br.selected = true } m.err = "" - if gone > 0 { - m.status = fmt.Sprintf("fetched & pruned — %d gone branch(es) selected; press d to prune", gone) - } else { + switch { + case gone == 0: m.status = "fetched & pruned — no gone branches" + case risky == 0: + m.status = fmt.Sprintf("fetched & pruned — %d gone branch(es) selected; press d to prune", gone) + default: + m.status = fmt.Sprintf("fetched & pruned — %d of %d gone branch(es) selected; %d hold commits not in %s (select with space to discard)", + gone-risky, gone, risky, m.riskBase) } return m, nil case branchDeletedMsg: @@ -575,6 +678,7 @@ func (m model) updateList(msg tea.KeyMsg) (tea.Model, tea.Cmd) { m.state = stateHelp case "d", "enter": if len(m.selectedBranches()) > 0 { + m.measureSelectedRisk() // the confirm screen states what each delete costs m.state = stateConfirm } } @@ -620,6 +724,7 @@ func (m model) armedRemoteCount() int { // per branch (which run concurrently) plus the spinner tick. Remote branches are // pushed --delete only when includeRemote is set (see updateConfirm). func (m *model) startDeletions(includeRemote bool) tea.Cmd { + m.measureSelectedRisk() // results carry the cost through to the force prompt sel := m.selectedBranches() m.results = make([]deleteResult, len(sel)) m.spinnerFrame = 0 @@ -627,7 +732,7 @@ func (m *model) startDeletions(includeRemote bool) tea.Cmd { cmds := []tea.Cmd{spinnerTickCmd()} for i, b := range sel { - m.results[i] = deleteResult{name: b.name, ahead: b.ahead} + m.results[i] = deleteResult{br: b} wantRemote := includeRemote && b.deleteRemote && b.upstream != "" cmds = append(cmds, deleteBranchCmd(i, b, b.deleteFlag(m.force), wantRemote)) } @@ -701,10 +806,9 @@ func (m model) updateDiff(msg tea.KeyMsg) (tea.Model, tea.Cmd) { return m, nil } -// deleteFlag returns the git branch delete flag for b under the given force mode: -// -D for forced or gone branches (which -d refuses), -d otherwise. +// deleteFlag returns the git branch delete flag for b under the given force mode. func (b branch) deleteFlag(force bool) string { - if force || b.gone { + if b.forcedDelete(force) { return "-D" } return "-d" @@ -713,34 +817,52 @@ func (b branch) deleteFlag(force bool) string { // deleteBranch runs one branch's local delete and, when wantRemote is set, its // remote-branch push --delete. It is the single worker shared by the synchronous // performDeletions path and the asynchronous deleteBranchCmd path. -func deleteBranch(name, flag string, wantRemote bool, remoteName, remoteBranch string, ahead int) deleteResult { - res := deleteResult{name: name, ahead: ahead, done: true} - if _, err := runGit("branch", flag, name); err != nil { +func deleteBranch(b branch, flag string, wantRemote bool) deleteResult { + res := deleteResult{br: b, done: true} + if _, err := runGit("branch", flag, b.name); err != nil { res.localErr = err.Error() - res.forceable = flag == "-d" // a refused safe delete can be retried with -D + // Only an unmerged refusal is worth escalating to -D. Other failures — a + // branch held by another worktree, most commonly — fail identically under + // -D, so offering the retry would just mislabel them as lost commits. + res.forceable = flag == "-d" && strings.Contains(res.localErr, "not fully merged") } else { res.localOK = true } - if wantRemote { - res.remoteTried = true - if _, err := runGit("push", remoteName, "--delete", remoteBranch); err != nil { - res.remoteErr = err.Error() - } else { - res.remoteOK = true - } + // Never delete the remote copy while the local branch survives a refused + // delete: that would strand its commits with nowhere else to exist. The push + // is deferred until the force retry clears the local branch. + switch { + case !wantRemote: + case res.localOK: + pushRemoteDelete(&res) + default: + res.remoteSkipped = true } return res } +// pushRemoteDelete deletes res's remote branch, clearing any deferral: the +// results screen tests remoteSkipped first, so a stale flag would report the +// remote as kept right after a successful push. +func pushRemoteDelete(res *deleteResult) { + res.remoteSkipped = false + res.remoteTried = true + if _, err := runGit("push", res.br.remoteName(), "--delete", res.br.remoteBranch()); err != nil { + res.remoteErr = err.Error() + } else { + res.remoteOK = true + } +} + // performDeletions deletes the selected branches synchronously. The interactive // UI uses the async startDeletions path instead; this remains for tests and as // the straightforward equivalent. func (m *model) performDeletions() { + m.measureSelectedRisk() m.results = nil for _, b := range m.selectedBranches() { wantRemote := b.deleteRemote && b.upstream != "" - res := deleteBranch(b.name, b.deleteFlag(m.force), wantRemote, b.remoteName(), b.remoteBranch(), b.ahead) - m.results = append(m.results, res) + m.results = append(m.results, deleteBranch(b, b.deleteFlag(m.force), wantRemote)) } m.reloadBranches() } @@ -766,13 +888,52 @@ func (m *model) reloadBranches() { } } -// refreshMergeInfo caches the remote default branch and marks each branch whose -// upstream is merged into it. Call after every branch (re)load. +// refreshMergeInfo caches the remote default branch, marks each branch whose +// upstream is merged into it or whose tip is merged into HEAD, and measures what +// deleting it would cost. Call after every branch (re)load. func (m *model) refreshMergeInfo() { m.remoteDefault = remoteDefault() merged := remoteMergedSet(m.remoteDefault) + headMerged := localMergedSet() + + m.riskBase = m.remoteDefault + if m.riskBase == "" { + m.riskBase = localDefaultBranch("") + } + + for i := range m.branches { + b := &m.branches[i] + b.remoteMerged = b.upstream != "" && merged[b.upstream] + b.headMerged = headMerged[b.name] + b.riskMeasured = false // the branch was just reloaded; any old count is stale + // Only gone branches are measured up front, because `p` consults the count + // to decide what it may auto-select. The rest wait for measureSelectedRisk: + // riskCommitCount is a subprocess per branch, and running it for every + // unmergeable branch here cost a second of startup on a repo with dozens. + if b.gone { + m.measureRisk(b) + } + } +} + +// measureRisk fills in b's cost-of-deletion count, once per (re)load. +func (m *model) measureRisk(b *branch) { + if b.riskMeasured { + return + } + b.riskCommits = riskCommitCount(b.name, m.riskBase) + b.riskMeasured = true +} + +// measureSelectedRisk measures what deleting each selected branch would cost. +// Call before any view that reports the cost: only branches a safe delete would +// refuse are measured, since those are the ones deleted with -D. +func (m *model) measureSelectedRisk() { for i := range m.branches { - m.branches[i].remoteMerged = m.branches[i].upstream != "" && merged[m.branches[i].upstream] + b := &m.branches[i] + if b.selected && (b.gone || !b.safeDeletable()) { + m.measureRisk(b) + } } } @@ -785,11 +946,16 @@ func (m *model) forceDeleteUnmerged() { if r.localOK || !r.forceable { continue } - if _, err := runGit("branch", "-D", r.name); err != nil { + if _, err := runGit("branch", "-D", r.br.name); err != nil { r.localErr = err.Error() - } else { - r.localOK = true - r.localErr = "" + continue + } + r.localOK = true + r.localErr = "" + // The armed remote delete was deferred while the local branch survived; + // now that it is gone, honour what the user confirmed. + if r.remoteSkipped { + pushRemoteDelete(r) } } m.reloadBranches() @@ -870,7 +1036,7 @@ func (m model) diffView() string { func (m *model) recomputeNameWidth() { w := 0 for _, br := range m.branches { - w = max(w, len(br.name)) + w = max(w, ansi.StringWidth(br.name)) } m.nameW = min(40, max(6, w)) } @@ -938,7 +1104,7 @@ func (m model) renderRow(i, nameW int) string { cur = currentStyle.Render("*") } - name := fmt.Sprintf("%-*s", nameW, truncate(br.name, nameW)) + name := pad(truncate(br.name, nameW), nameW) var nameRendered string switch { @@ -965,7 +1131,9 @@ func (m model) renderRow(i, nameW int) string { func (m model) trackStr(br branch) string { if br.gone { - return goneStyle.Render(fmt.Sprintf("%-8s", "gone")) + // Same column style as every other track value, or the columns that + // follow shift left on exactly the rows the user is here to act on. + return trackColStyle.Render(goneStyle.Render("gone")) } if br.upstream == "" { return trackColStyle.Render(dimStyle.Render("-")) @@ -994,14 +1162,23 @@ func (m model) subjectWidth(nameW int) int { return max(10, m.width-used) } +// truncate shortens s to w terminal cells, appending an ellipsis when it does +// not fit. Measured in display cells rather than bytes so multibyte text is +// never sliced mid-rune and wide (CJK/emoji) characters do not overflow. func truncate(s string, w int) string { - if len(s) <= w { - return s - } - if w <= 1 { + if w <= 0 { return "" } - return s[:w-1] + "…" + return ansi.Truncate(s, w, "…") +} + +// pad right-pads s to w display cells. The fmt width verbs count runes, which +// misaligns columns whose content contains wide characters. +func pad(s string, w int) string { + if d := w - ansi.StringWidth(s); d > 0 { + return s + strings.Repeat(" ", d) + } + return s } func (m model) helpView() string { @@ -1022,7 +1199,7 @@ func (m model) helpView() string { {"a / n", "select all / none"}, {"r", "toggle delete of upstream remote branch"}, {"v", "view branch diff (green add / red remove)"}, - {"p", "fetch --all --prune & select gone branches"}, + {"p", "fetch --all --prune & select safe gone branches"}, {"s", "cycle sort field (date, name, ahead/behind)"}, {"o", "toggle sort order (asc/desc)"}, {"f", "toggle force delete (-d / -D)"}, @@ -1045,6 +1222,11 @@ func (m model) helpView() string { {"gone", "upstream was configured but no longer exists"}, }) + b.WriteString("\n") + b.WriteString(dimStyle.Render("Gone branches are deleted with -D. Any holding commits that are not in\n" + + "the default branch are left unselected by p and flagged on the confirm screen.")) + b.WriteString("\n") + b.WriteString("\n") b.WriteString(dimStyle.Render("press any key to return")) b.WriteString("\n") @@ -1094,8 +1276,8 @@ func (m model) confirmView() string { if br.deleteRemote && br.upstream != "" { b.WriteString(" " + errStyle.Render(fmt.Sprintf("+ delete remote %s/%s", br.remoteName(), br.remoteBranch())) + "\n") } - if !m.force && !br.gone && br.ahead > 0 { - b.WriteString(" " + errStyle.Render(fmt.Sprintf("⚠ %d unmerged commit(s) — safe delete (-d) will fail; use force (f)", br.ahead)) + "\n") + if w := m.riskWarning(br); w != "" { + b.WriteString(" " + errStyle.Render(w) + "\n") } b.WriteString("\n") } @@ -1110,6 +1292,30 @@ func (m model) confirmView() string { return b.String() } +// riskWarning states the cost of deleting br, or "" when the delete is clean. +// It covers every branch git's safe delete would refuse plus gone branches, +// which take the -D path regardless: under -D the unmerged commits are +// discarded, under -d the delete simply fails. +func (m model) riskWarning(br branch) string { + if br.safeDeletable() && !br.gone { + return "" + } + if br.forcedDelete(m.force) { // -D: the question is what gets discarded + if br.riskCommits > 0 { + return fmt.Sprintf("⚠ %d commit(s) not in %s — force delete (-D) will discard them", br.riskCommits, m.riskBase) + } + if m.riskBase == "" { + return "⚠ no base branch to compare against — force delete (-D) discards any unmerged commits" + } + 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 { + return fmt.Sprintf("⚠ not fully merged: %d commit(s) not in %s — safe delete (-d) will fail; use force (f)", br.riskCommits, m.riskBase) + } + return "⚠ not fully merged — safe delete (-d) will fail; use force (f)" +} + func (m model) forcePromptView() string { var b strings.Builder failures := m.forceableFailures() @@ -1122,9 +1328,20 @@ func (m model) forcePromptView() string { b.WriteString(".\n\n") for _, r := range failures { - b.WriteString(" " + cursorStyle.Render("• "+r.name) + "\n") - if r.ahead > 0 { - b.WriteString(" " + errStyle.Render(fmt.Sprintf("⚠ %d unmerged commit(s) will be lost", r.ahead)) + "\n") + b.WriteString(" " + cursorStyle.Render("• "+r.br.name) + "\n") + // Every branch here failed -d, so its risk was measured before the delete + // ran: riskCommits == 0 means either nothing is missing from the base or + // there was no base to measure against. + switch { + case r.br.riskCommits > 0: + b.WriteString(" " + errStyle.Render(fmt.Sprintf("⚠ %d commit(s) not in %s will be lost", r.br.riskCommits, m.riskBase)) + "\n") + case m.riskBase == "": + b.WriteString(" " + errStyle.Render("⚠ no base branch to compare against — unmerged commits may be lost") + "\n") + default: + b.WriteString(" " + dimStyle.Render("no commits missing from "+m.riskBase) + "\n") + } + if r.remoteSkipped { + b.WriteString(" " + errStyle.Render(fmt.Sprintf("+ remote %s/%s will be deleted once the branch is gone", r.br.remoteName(), r.br.remoteBranch())) + "\n") } } @@ -1139,16 +1356,18 @@ func (m model) forcePromptView() string { // tried) into b. Shared by the results screen and the live deleting screen. func writeResultLines(b *strings.Builder, r deleteResult) { if r.localOK { - b.WriteString(okStyle.Render(" ✓ ") + "deleted local " + r.name + "\n") + b.WriteString(okStyle.Render(" ✓ ") + "deleted local " + r.br.name + "\n") } else { - b.WriteString(errStyle.Render(" ✗ ") + "local " + r.name + ": " + r.localErr + "\n") + b.WriteString(errStyle.Render(" ✗ ") + "local " + r.br.name + ": " + r.localErr + "\n") } - if r.remoteTried { - if r.remoteOK { - b.WriteString(okStyle.Render(" ✓ ") + "deleted remote " + r.name + "\n") - } else { - b.WriteString(errStyle.Render(" ✗ ") + "remote " + r.name + ": " + r.remoteErr + "\n") - } + switch { + case r.remoteSkipped: + // Say why the armed remote survived, or it reads as a silent failure. + b.WriteString(errStyle.Render(" ! ") + "kept remote " + r.br.remoteName() + "/" + r.br.remoteBranch() + ": local delete failed\n") + case r.remoteTried && r.remoteOK: + b.WriteString(okStyle.Render(" ✓ ") + "deleted remote " + r.br.name + "\n") + case r.remoteTried: + b.WriteString(errStyle.Render(" ✗ ") + "remote " + r.br.name + ": " + r.remoteErr + "\n") } } @@ -1161,7 +1380,7 @@ func (m model) deletingView() string { if r.done { writeResultLines(&b, r) } else { - b.WriteString(dimStyle.Render(" "+spin+" deleting "+r.name+"…") + "\n") + b.WriteString(dimStyle.Render(" "+spin+" deleting "+r.br.name+"…") + "\n") } } b.WriteString("\n") diff --git a/main_test.go b/main_test.go index 0e0140f..0bf3781 100644 --- a/main_test.go +++ b/main_test.go @@ -1,12 +1,16 @@ package main import ( + "fmt" "os" "os/exec" + "regexp" "strings" "testing" + "unicode/utf8" tea "github.com/charmbracelet/bubbletea" + "github.com/charmbracelet/x/ansi" ) // key builds a rune KeyMsg (e.g. "y", "R") for driving update handlers in tests. @@ -55,20 +59,12 @@ func setupRepo(t *testing.T) string { git(t, tmp, "init", "-q", "-b", "main") git(t, tmp, "config", "user.email", "t@t.t") git(t, tmp, "config", "user.name", "t") - if err := os.WriteFile(tmp+"/a", []byte("a"), 0o644); err != nil { - t.Fatal(err) - } - git(t, tmp, "add", "a") - git(t, tmp, "commit", "-qm", "init commit") + 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") - if err := os.WriteFile(tmp+"/b", []byte("b"), 0o644); err != nil { - t.Fatal(err) - } - git(t, tmp, "add", "b") - git(t, tmp, "commit", "-qm", "wip") + commitFile(t, tmp, "b", "b") git(t, tmp, "checkout", "-q", "-b", "feature/tracked") git(t, tmp, "push", "-q", "-u", "origin", "feature/tracked") git(t, tmp, "checkout", "-q", "main") @@ -218,7 +214,7 @@ func TestSafeDeleteRefusesUnmerged(t *testing.T) { var merged, unmerged *deleteResult for i := range m.results { - switch m.results[i].name { + switch m.results[i].br.name { case "feature/merged": merged = &m.results[i] case "feature/unmerged": @@ -255,10 +251,10 @@ func TestForceDeleteUnmergedRetry(t *testing.T) { // The refused unmerged branch should be surfaced for a force prompt, with // its ahead count copied onto the result (0 here: it has no upstream). failures := m.forceableFailures() - if len(failures) != 1 || failures[0].name != "feature/unmerged" { + if len(failures) != 1 || failures[0].br.name != "feature/unmerged" { t.Fatalf("want feature/unmerged in forceableFailures, got %+v", failures) } - if failures[0].ahead != 0 { + if failures[0].br.ahead != 0 { t.Fatalf("want ahead=0 captured (no upstream), got %+v", failures[0]) } if find(m.branches, "feature/unmerged") == nil { @@ -271,7 +267,7 @@ func TestForceDeleteUnmergedRetry(t *testing.T) { t.Fatalf("no failures should remain after force retry: %+v", m.results) } for _, r := range m.results { - if r.name == "feature/unmerged" && (!r.localOK || r.localErr != "") { + if r.br.name == "feature/unmerged" && (!r.localOK || r.localErr != "") { t.Fatalf("feature/unmerged should be deleted after force retry: %+v", r) } } @@ -545,6 +541,508 @@ func TestWheelScrollDoesNotLeakBetweenViews(t *testing.T) { } } +var ansiRe = regexp.MustCompile("\x1b\\[[0-9;]*m") + +func stripANSI(s string) string { return ansiRe.ReplaceAllString(s, "") } + +// truncate measures terminal cells, so multibyte text must never be sliced +// mid-rune (which emitted invalid UTF-8) and wide characters must not overflow. +func TestTruncateDisplayWidth(t *testing.T) { + for _, w := range []int{1, 4, 9, 10, 11, 40} { + for _, s := range []string{"日本語のコミットです", "feat: 🚀 ship it", "feature/café", "plain-ascii"} { + got := truncate(s, w) + if !utf8.ValidString(got) { + t.Fatalf("truncate(%q, %d) = %q: invalid UTF-8", s, w, got) + } + if cells := ansi.StringWidth(got); cells > w { + t.Fatalf("truncate(%q, %d) = %q: %d cells, over budget", s, w, got, cells) + } + } + } + if got := truncate("plain-ascii", 40); got != "plain-ascii" { + t.Fatalf("short strings must pass through unchanged, got %q", got) + } +} + +// pad must align on display cells; wide glyphs otherwise push later columns. +func TestPadDisplayWidth(t *testing.T) { + for _, s := range []string{"日本語", "ab", "🚀", ""} { + if got := ansi.StringWidth(pad(s, 10)); got != 10 { + t.Fatalf("pad(%q, 10) is %d cells, want 10", s, got) + } + } +} + +// Every track value must occupy the same column width, or the columns after it +// shift on gone rows (regression: "gone" was 8 wide where others were 10). +func TestTrackColumnAlignment(t *testing.T) { + m := model{width: 120} + rows := []branch{ + {name: "aaa", upstream: "origin/aaa", ahead: 2, behind: 1}, + {name: "bbb", upstream: "origin/bbb", gone: true}, + {name: "ccc"}, + {name: "ddd", upstream: "origin/ddd", remoteMerged: true}, + } + want := -1 + for _, br := range rows { + m.branches = []branch{br} + plain := stripANSI(m.renderRow(0, 10)) + at := strings.Index(plain, "0001-Jan-01") // the column right after track + if at < 0 { + t.Fatalf("date column missing for %q: %q", br.name, plain) + } + // Compare display cells, not the byte offset: the arrows in the track + // column are multibyte, which is the whole point of this alignment. + cells := ansi.StringWidth(plain[:at]) + if want == -1 { + want = cells + } else if cells != want { + t.Fatalf("branch %q: date column at cell %d, want %d (track width differs)\n%q", + br.name, cells, want, plain) + } + } +} + +// The core safety fix: a gone branch holding commits that are not in the base +// is force-deleted with -D, so it must be measured, warned about on the confirm +// screen, and left out of `p`'s auto-selection. +func TestGoneBranchWithUnpushedCommitsIsGuarded(t *testing.T) { + repo := setupRepo(t) + chdir(t, repo) + + // feature/tracked is pushed; add a commit that never reaches the remote, + // then delete the remote branch and prune so it goes "gone". + git(t, repo, "checkout", "-q", "feature/tracked") + commitFile(t, repo, "unpushed", "irreplaceable") + git(t, repo, "checkout", "-q", "main") + git(t, repo, "push", "-q", "origin", "--delete", "feature/tracked") + git(t, repo, "fetch", "-q", "--all", "--prune") + + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + tb := find(m.branches, "feature/tracked") + if tb == nil || !tb.gone { + t.Fatalf("feature/tracked should be gone: %+v", tb) + } + if tb.ahead != 0 { + t.Fatalf("precondition: git reports no ahead count for gone branches, got %d", tb.ahead) + } + if tb.riskCommits == 0 { + t.Fatal("gone branch with unpushed commits must report riskCommits > 0") + } + + // The confirm screen must name the cost before anything is deleted. + tb.selected = true + m.state = stateConfirm + out := stripANSI(m.confirmView()) + if !strings.Contains(out, "force delete (-D) will discard them") { + t.Fatalf("confirm view must warn about discarded commits:\n%s", out) + } + if !strings.Contains(out, fmt.Sprintf("%d commit(s) not in", tb.riskCommits)) { + t.Fatalf("confirm view must state the commit count:\n%s", out) + } + + // `p` must not auto-select it: discarding those commits stays deliberate. + for i := range m.branches { + m.branches[i].selected = false + } + nm, _ := m.Update(fetchDoneMsg{}) + after := nm.(model) + 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) + } + if !strings.Contains(after.status, "hold commits not in") { + t.Fatalf("status should report the skipped branches, got %q", after.status) + } +} + +// A gone branch whose work is already in the base carries no risk, so `p` still +// auto-selects it — the headline prune workflow must stay one keystroke. +func TestGoneMergedBranchStillAutoSelected(t *testing.T) { + repo := setupRepo(t) + chdir(t, repo) + + // feature/tracked's tip is already an ancestor of main's content here: reset + // it to main, push, then delete the remote branch and prune. + git(t, repo, "branch", "-f", "feature/tracked", "main") + git(t, repo, "push", "-qf", "origin", "feature/tracked") + git(t, repo, "push", "-q", "origin", "--delete", "feature/tracked") + git(t, repo, "fetch", "-q", "--all", "--prune") + + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + b := find(m.branches, "feature/tracked") + if b == nil || !b.gone || b.riskCommits != 0 { + t.Fatalf("merged gone branch should carry no risk: %+v", b) + } + // The headline prune must stay warning-free: -D discards nothing here. + 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) + if b := find(after.branches, "feature/tracked"); b == nil || !b.selected { + t.Fatalf("risk-free gone branch should still be auto-selected: %+v", b) + } + if !strings.Contains(after.status, "press d to prune") { + t.Fatalf("status should offer the prune, got %q", after.status) + } +} + +// riskCommitCount uses patch-equivalence, so a cherry-picked commit already in +// the base is not counted as work at risk. +func TestRiskCommitCountIgnoresCherryPicked(t *testing.T) { + repo := setupRepo(t) + chdir(t, repo) + + git(t, repo, "checkout", "-q", "-b", "picked", "main") + commitFile(t, repo, "p", "p") + git(t, repo, "checkout", "-q", "main") + git(t, repo, "cherry-pick", "picked") + + if n := riskCommitCount("picked", "main"); n != 0 { + t.Fatalf("cherry-picked commit should not count as at risk, got %d", n) + } + if n := riskCommitCount("feature/unmerged", "main"); 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 { + t.Fatalf("empty base should yield 0, got %d", n) + } +} + +// Merge info must resolve on repos whose only remote is not named "origin"; +// previously remoteDefault() returned "" and the safety indicators vanished. +func TestNonOriginRemoteResolves(t *testing.T) { + repo := setupRepo(t) + chdir(t, repo) + + // Rename origin -> upstream, leaving no remote called "origin". + git(t, repo, "remote", "rename", "origin", "upstream") + git(t, repo, "fetch", "-q", "--all", "--prune") + + if got := remoteDefault(); got != "upstream/main" { + t.Fatalf("remoteDefault should resolve upstream/main, got %q", got) + } + if got := baseBranch("feature/unmerged"); got != "upstream/main" { + t.Fatalf("baseBranch should use the non-origin remote, got %q", got) + } + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + if m.remoteDefault != "upstream/main" || m.riskBase != "upstream/main" { + t.Fatalf("model should cache the resolved default: %q / %q", m.remoteDefault, m.riskBase) + } +} + +// origin is preferred when several remotes are configured. +func TestRemotesPrefersOrigin(t *testing.T) { + repo := setupRepo(t) + chdir(t, repo) + + git(t, repo, "remote", "add", "aaa-fork", repo) + got := remotes() + if len(got) == 0 || got[0] != "origin" { + t.Fatalf("origin should sort first, got %v", got) + } + if len(got) != 2 { + t.Fatalf("want both remotes, got %v", got) + } +} + +// commitFile writes a file on the current branch and commits it. +func commitFile(t *testing.T, dir, name, body string) { + t.Helper() + if err := os.WriteFile(dir+"/"+name, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + git(t, dir, "add", name) + git(t, dir, "commit", "-qm", "add "+name) +} + +// setupDeleteShapes adds one branch for every shape that decides whether +// `git branch -d` is accepted, so predictions can be checked against real git. +// setupRepo already supplies the two upstream-less shapes (feature/unmerged and +// feature/merged), so they are not rebuilt here. +func setupDeleteShapes(t *testing.T, repo string) { + t.Helper() + // Upstream, with a local commit the upstream lacks. + git(t, repo, "checkout", "-q", "-b", "shape/ahead", "main") + git(t, repo, "push", "-q", "-u", "origin", "shape/ahead") + commitFile(t, repo, "ahead", "x") + + // Ahead of a live upstream, but merged into HEAD. git consults the upstream + // alone whenever it resolves, so it refuses this even though HEAD holds the + // work — the case an either-or reading of the two criteria gets wrong. + git(t, repo, "checkout", "-q", "-b", "shape/ahead-head-merged", "main") + git(t, repo, "push", "-q", "-u", "origin", "shape/ahead-head-merged") + commitFile(t, repo, "ahead-head-merged", "x") + git(t, repo, "checkout", "-q", "main") + git(t, repo, "merge", "-q", "--no-ff", "-m", "merge shape/ahead-head-merged", "shape/ahead-head-merged") + + // Upstream, strictly behind it — an ancestor, so merged into its upstream. + git(t, repo, "checkout", "-q", "-b", "shape/behind", "main") + git(t, repo, "push", "-q", "-u", "origin", "shape/behind") + commitFile(t, repo, "behind", "x") + git(t, repo, "push", "-q", "origin", "shape/behind") + git(t, repo, "reset", "-q", "--hard", "HEAD~1") + + // Squash-merged into origin/main: different commits, same content, upstream + // still present. + git(t, repo, "checkout", "-q", "-b", "shape/squashed", "main") + commitFile(t, repo, "squashed", "x") + git(t, repo, "push", "-q", "-u", "origin", "shape/squashed") + git(t, repo, "checkout", "-q", "main") + git(t, repo, "merge", "-q", "--squash", "shape/squashed") + git(t, repo, "commit", "-qm", "squash shape/squashed") + git(t, repo, "push", "-q", "origin", "main") + + git(t, repo, "checkout", "-q", "main") + git(t, repo, "fetch", "-q", "--all", "--prune") +} + +// Ground truth: safeDeletable must agree with what `git branch -d` actually does +// for every branch shape. This is the predicate the confirm-screen warning and +// the risk measurement both hang off, so a wrong answer here is either a silent +// delete failure (the reported bug) or a false alarm. +func TestSafeDeletableMatchesGit(t *testing.T) { + repo := setupRepo(t) + chdir(t, repo) + setupDeleteShapes(t, repo) + + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + + want := map[string]bool{ + "shape/ahead": false, // holds a commit its upstream lacks + "shape/ahead-head-merged": false, // ditto, and HEAD holding it does not help + "shape/behind": true, // ancestor of its upstream + "shape/squashed": true, // equal to its upstream + "feature/unmerged": false, // no upstream, unmerged + "feature/merged": true, // no upstream, merged into HEAD + } + for name, expect := range want { + b := find(m.branches, name) + if b == nil { + t.Fatalf("%s missing from the branch list", name) + } + if b.safeDeletable() != expect { + t.Errorf("%s: safeDeletable()=%v want %v (upstream=%q ahead=%d headMerged=%v)", + name, b.safeDeletable(), expect, b.upstream, b.ahead, b.headMerged) + } + // Now ask git itself. Deleting one branch cannot change another's merge + // status, so the whole set can be checked in one pass. + _, gitErr := runGit("branch", "-d", name) + if (gitErr == nil) != expect { + t.Errorf("git branch -d %s: err=%v, want accepted=%v", name, gitErr, expect) + } + if gitErr != nil && !strings.Contains(gitErr.Error(), "not fully merged") { + t.Errorf("%s refused for an unexpected reason: %v", name, gitErr) + } + } +} + +// The reported bug: an unmerged branch with no upstream was deleted with -d and +// failed, with nothing on the confirm screen having warned about it. +func TestUnmergedBranchWithoutUpstreamIsWarned(t *testing.T) { + repo := setupRepo(t) + chdir(t, repo) + + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + b := find(m.branches, "feature/unmerged") + if b == nil || b.upstream != "" || b.ahead != 0 { + t.Fatalf("precondition: want an upstream-less branch with ahead=0: %+v", b) + } + if b.safeDeletable() { + t.Fatalf("an unmerged upstream-less branch is not safely deletable: %+v", b) + } + + // Drive the real path: 'd' measures the selection's risk, then confirms. + b.selected = true + nm, _ := m.updateList(key("d")) + m = nm.(model) + if m.state != stateConfirm { + t.Fatalf("'d' should open the confirm screen, got %v", m.state) + } + b = find(m.branches, "feature/unmerged") + if b.riskCommits != 1 { + t.Fatalf("want the branch's 1 unique commit measured, got %d", b.riskCommits) + } + out := stripANSI(m.confirmView()) + if !strings.Contains(out, "safe delete (-d) will fail") { + t.Fatalf("confirm view must warn that -d will be refused:\n%s", out) + } + if !strings.Contains(out, fmt.Sprintf("%d commit(s) not in %s", b.riskCommits, m.riskBase)) { + t.Fatalf("confirm view must state what a force would discard:\n%s", out) + } +} + +// Negative spec: branches git will happily delete must draw no warning at all, +// or the confirm screen cries wolf on the ordinary cleanup path. +func TestNoWarningForSafelyDeletableBranches(t *testing.T) { + repo := setupRepo(t) + chdir(t, repo) + setupDeleteShapes(t, repo) + + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + for _, name := range []string{"feature/merged", "shape/behind", "shape/squashed"} { + b := find(m.branches, name) + if b == nil { + t.Fatalf("%s missing", name) + } + if w := m.riskWarning(*b); w != "" { + t.Errorf("%s deletes cleanly but warned: %q", name, w) + } + b.selected = true + } + m.state = stateConfirm + // Only the delete-risk lines are asserted on: the separate "not merged into + // " indicator is about the remote's state, not about whether the + // delete will succeed, and legitimately fires for squash-merged branches. + out := stripANSI(m.confirmView()) + for _, phrase := range []string{"safe delete (-d) will fail", "force delete (-D)", "not fully merged"} { + if strings.Contains(out, phrase) { + t.Fatalf("confirm view must not warn %q for clean branches:\n%s", phrase, out) + } + } +} + +// The force prompt asks the user to approve a -D, so it must state what that +// costs. It used to print the ahead count, which is 0 for exactly the branches +// that reach the prompt without an upstream — leaving the prompt blank. +func TestForcePromptStatesCommitCount(t *testing.T) { + repo := setupRepo(t) + chdir(t, repo) + + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + find(m.branches, "feature/unmerged").selected = true + m.force = false + m.performDeletions() + + failures := m.forceableFailures() + if len(failures) != 1 { + t.Fatalf("want one refused delete, got %+v", failures) + } + if failures[0].br.riskCommits != 1 { + t.Fatalf("the refused result must carry its measured cost, got %+v", failures[0]) + } + + m.state = stateForcePrompt + out := stripANSI(m.forcePromptView()) + if !strings.Contains(out, fmt.Sprintf("1 commit(s) not in %s will be lost", m.riskBase)) { + t.Fatalf("force prompt must state the commit count:\n%s", out) + } +} + +// The remote copy is the last place unmerged commits survive a refused local +// delete, so the armed push must be deferred — and then honoured once the force +// retry actually removes the branch. +func TestRemoteDeleteDeferredUntilLocalSucceeds(t *testing.T) { + repo := setupRepo(t) + chdir(t, repo) + + // Give feature/tracked a commit its upstream lacks, so -d is refused. + git(t, repo, "checkout", "-q", "feature/tracked") + commitFile(t, repo, "unpushed", "x") + git(t, repo, "checkout", "-q", "main") + + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + tb := find(m.branches, "feature/tracked") + if tb == nil || tb.ahead != 1 { + t.Fatalf("precondition: feature/tracked should be ahead 1: %+v", tb) + } + tb.selected = true + tb.deleteRemote = true + m.force = false // safe delete, which git will refuse + m.performDeletions() + + r := m.results[0] + if r.localOK { + t.Fatalf("safe delete should have been refused: %+v", r) + } + if r.remoteTried { + t.Fatalf("the remote must not be touched while the local branch survives: %+v", r) + } + if !r.remoteSkipped { + t.Fatalf("the deferred push must be recorded: %+v", r) + } + if !remoteHasBranch(t, repo, "feature/tracked") { + t.Fatal("remote feature/tracked must survive a refused local delete") + } + if out := stripANSI(m.resultView()); !strings.Contains(out, "kept remote origin/feature/tracked") { + t.Fatalf("results must explain the kept remote:\n%s", out) + } + + // The force retry clears the branch, so the arming is finally honoured. + m.forceDeleteUnmerged() + r = m.results[0] + if !r.localOK || !r.remoteTried || !r.remoteOK || r.remoteSkipped { + t.Fatalf("force retry should complete both deletes: %+v", r) + } + if remoteHasBranch(t, repo, "feature/tracked") { + t.Fatal("remote feature/tracked should be deleted after the force retry") + } +} + +// Negative spec: a delete refused for any reason other than unmerged commits +// cannot be rescued by -D, so it must not raise the force prompt — which would +// both mislabel the cause and offer a retry that fails identically. +func TestNonUnmergedFailureIsNotForceable(t *testing.T) { + repo := setupRepo(t) + chdir(t, repo) + + // A second worktree holds feature/unmerged; git refuses to delete it under + // -d and -D alike. + wt := t.TempDir() + "/wt" + git(t, repo, "worktree", "add", "-q", wt, "feature/unmerged") + + if _, err := runGit("branch", "-D", "feature/unmerged"); err == nil { + t.Fatal("precondition: -D should also fail for a branch held by a worktree") + } + + b := branch{name: "feature/unmerged"} + res := deleteBranch(b, "-d", false) + if res.localOK { + t.Fatalf("delete should have failed: %+v", res) + } + if res.forceable { + t.Fatalf("a worktree conflict must not be offered as force-retryable: %q", res.localErr) + } + + // End to end: the async path lands on the results screen, not the prompt. + m, err := initialModel() + if err != nil { + t.Fatal(err) + } + m.results = []deleteResult{{br: branch{name: "feature/unmerged"}}} + m.state = stateDeleting + msg := deleteBranchCmd(0, *find(m.branches, "feature/unmerged"), "-d", false)() + nm, _ := m.Update(msg) + if got := nm.(model).state; got != stateResult { + t.Fatalf("state should be stateResult, got %v", got) + } +} + // The version string is well-formed even when VCS build info is absent. func TestVersionString(t *testing.T) { s := versionString()