Merge pull request #1 from skunkworker/safety-fixes-and-installer

Safety fixes and installer

Author: skunkworker

.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

.gitignore

@@ -0,0 +1,3 @@
+# `go build ./...` drops the binary in the repo root; the Makefile installs to
+# $(BINDIR) instead.
+/git_pruner

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.

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

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 <base>...<branch>`), 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 `<remote>/HEAD` if set, else
+`<remote>/main`, else `<remote>/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 <base> — force delete (-D) will discard them`
+
+`git cherry` is used rather than `git rev-list <base>..<branch>` 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 `<base>`" 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 <remote> --delete <branch>`, 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)

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 <base>..<branch>`:* 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 <base>`
+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.

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