From 62aeaf063b66ed8558b258f6af87d318e0cb8568 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 12 Sep 2026 00:37:10 +1000 Subject: [PATCH] Keep worktrees git could not read, never delete them Any git error on a directory under the worktree root was classified orphan, and orphan deletes the directory outright, so a transient failure reading the source root became data loss on a plain `agentws prune --yes`. - prove a backing repo gone by stat before calling a directory an orphan - classify an unexplained git failure as keep, naming the error - refuse to remove a worktree whose git state is unknown, even with --include-keep - spell out that --include-keep discards uncommitted and in-progress work --- README.md | 11 ++++- cmd/agentws/main.go | 75 ++++++++++++++++++++++++++++--- cmd/agentws/prune.go | 21 ++++++--- cmd/agentws/prune_test.go | 92 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 185 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index f71803f..27f0cb5 100644 --- a/README.md +++ b/README.md @@ -181,11 +181,18 @@ spelled out, or `--json` for scripting. Neither form needs a terminal. | `--no-fetch` | judge against the refs already on disk, for offline use | | `--json` | emit the report as JSON on stdout, notes on stderr | | `--include-unmanaged` | also remove worktrees outside the worktree root | -| `--include-keep` | dangerous: also remove worktrees classified `keep`, never their branch | +| `--include-keep` | dangerous: also remove worktrees classified `keep`, destroying uncommitted and in-progress work | Without `--include-unmanaged` a hand-made worktree is reported and then skipped, naming the flag that would remove it. `--include-keep` is the only way past a -`keep`, and it still leaves the branch, so the commits outlive the worktree. +`keep`. It leaves the branch, so committed work outlives the worktree, but +`git worktree remove --force` discards a dirty working tree and a paused +rebase's sequencer state without a word, and no branch was carrying those. + +A directory under the worktree root is deleted outright only when its git dir +and the repo's shared `.git` are both proven absent by `stat`. When git merely +fails to answer for a checkout, the verdict is `keep` with the error as its +reason: an unread state is never a dead one. ### Auth / credential-helper design diff --git a/cmd/agentws/main.go b/cmd/agentws/main.go index 8cb6dbd..cbfeead 100644 --- a/cmd/agentws/main.go +++ b/cmd/agentws/main.go @@ -231,8 +231,11 @@ func newListCmd() *cobra.Command { } for _, w := range managed { branch := w.branch - if w.orphan { + switch { + case w.orphan: branch = "(orphan)" + case w.inspectErr != nil: + branch = "(unreadable)" } _, _ = fmt.Fprintf(out, "%s\t%s\t%s\n", w.repo, branch, w.path) } @@ -258,15 +261,20 @@ type managedWt struct { // missing is a registration whose working tree is gone: nothing to inspect, // nothing to lose. missing bool - // orphan is a directory under the worktree root whose backing repo no longer - // resolves, so no git state can be read from it at all. + // orphan is a directory under the worktree root whose backing git dir is + // proven gone, so no git state can be read from it ever again. orphan bool + // inspectErr is set when git refused to answer for a checkout and the reason + // was not a proven-absent backing repo. The state is unknown, never removable. + inspectErr error } // managedWorktrees scans the worktree root and resolves each entry's repo and // branch from git so branch names are accurate (not the sanitized dir name). -// Directories whose backing repo no longer resolves are returned as orphans -// rather than dropped, so callers can see (and clean up) the leftovers. +// Directories whose backing repo is proven gone are returned as orphans rather +// than dropped, so callers can see (and clean up) the leftovers; a directory git +// merely failed to answer for is returned with its error instead, because an +// unread state must never be mistaken for a dead one. func managedWorktrees() ([]managedWt, error) { wr, err := worktreeRoot() if err != nil { @@ -291,7 +299,15 @@ func managedWorktrees() ([]managedWt, error) { branch, branchErr := agent.GitCurrentBranch(path) srcDir, srcErr := agent.SourceRepoDir(path) if branchErr != nil || srcErr != nil { - out = append(out, managedWt{repo: repoFromDirName(e.Name()), path: path, managed: true, orphan: true}) + entry := managedWt{repo: repoFromDirName(e.Name()), path: path, managed: true} + if gone, err := backingRepoGone(path); err == nil && gone { + entry.orphan = true + } else if branchErr != nil { + entry.inspectErr = branchErr + } else { + entry.inspectErr = srcErr + } + out = append(out, entry) continue } out = append(out, managedWt{ @@ -306,6 +322,49 @@ func managedWorktrees() ([]managedWt, error) { return out, nil } +// backingRepoGone proves, by stat alone, that a worktree directory's backing +// repo no longer exists: its .git file names a git dir that is absent, and the +// repo's shared .git the git dir lived in is absent too. Only that pair licenses +// deleting the directory. Every other outcome — an unreadable .git file, a git +// dir still on disk, a stat that failed for any reason other than "not there", +// or a mere lost registration in a repo that is still present — reports false, +// so a transient or unexplained failure can never be read as "safe to delete". +func backingRepoGone(path string) (bool, error) { + dot := filepath.Join(path, ".git") + info, err := os.Lstat(dot) + if err != nil { + return false, err + } + if info.IsDir() { + return false, nil // a standalone checkout, not a linked worktree + } + data, err := os.ReadFile(dot) + if err != nil { + return false, err + } + rest, ok := strings.CutPrefix(strings.TrimSpace(string(data)), "gitdir:") + if !ok { + return false, fmt.Errorf("%s: not a worktree gitdir pointer", dot) + } + gitDir := strings.TrimSpace(rest) + if gitDir == "" { + return false, fmt.Errorf("%s: empty gitdir", dot) + } + if !filepath.IsAbs(gitDir) { + gitDir = filepath.Join(path, gitDir) + } + // The git dir is "/.git/worktrees/"; both it and the shared .git + // it sits in must be absent before the repo counts as gone. + for _, dir := range []string{gitDir, filepath.Dir(filepath.Dir(gitDir))} { + if _, err := os.Stat(dir); err == nil { + return false, nil + } else if !os.IsNotExist(err) { + return false, err + } + } + return true, nil +} + // repoFromDirName recovers the repo name from the "__" layout used // under the worktree root, for entries git can no longer answer for. func repoFromDirName(name string) string { @@ -509,6 +568,10 @@ func resolveWorktree(target string) (managedWt, error) { // proved the commits survive elsewhere may set it. func removeWorktree(out io.Writer, wt managedWt, deleteBranch, forceBranch bool) error { switch { + case wt.inspectErr != nil: + // No srcDir to act through and no idea what is in there; --include-keep + // must not turn that into a delete. + return fmt.Errorf("refusing to remove %s: git state unreadable: %w", wt.path, wt.inspectErr) case wt.orphan: return removeOrphanDir(out, wt) case wt.missing: diff --git a/cmd/agentws/prune.go b/cmd/agentws/prune.go index ad92f83..a5f18b4 100644 --- a/cmd/agentws/prune.go +++ b/cmd/agentws/prune.go @@ -88,7 +88,7 @@ func newPruneCmd() *cobra.Command { f.BoolVar(&opts.keepBranches, "keep-branches", false, "Never delete a local branch, whatever the classification") f.BoolVar(&opts.noFetch, "no-fetch", false, "Do not fetch; judge against the refs already on disk") f.BoolVar(&opts.jsonOut, "json", false, "Emit JSON instead of a table") - f.BoolVar(&opts.includeKeep, "include-keep", false, "Dangerous: also remove worktrees classified keep (needs --yes; never deletes their branch)") + f.BoolVar(&opts.includeKeep, "include-keep", false, "Dangerous: also remove worktrees classified keep (needs --yes). Destroys uncommitted changes and paused rebase/merge state, which no branch is carrying; only the branch itself survives") f.BoolVar(&opts.includeUnmanaged, "include-unmanaged", false, "Also remove worktrees that live outside the worktree root") return cmd } @@ -142,11 +142,15 @@ func classifyAll(notes io.Writer, prs prLister, worktrees []managedWt, opts prun byRepo := map[string][]managedWt{} var results []pruneResult for _, w := range worktrees { - if w.orphan { + switch { + case w.inspectErr != nil: + // Unknown is not gone: a checkout git refused to answer for keeps. + results = append(results, pruneResult{wt: w, verdict: verdictKeep, reason: "inspection failed: " + oneLine(w.inspectErr.Error())}) + case w.orphan: results = append(results, pruneResult{wt: w, verdict: verdictRemove, reason: "backing repo gone, no git state to read"}) - continue + default: + byRepo[w.srcDir] = append(byRepo[w.srcDir], w) } - byRepo[w.srcDir] = append(byRepo[w.srcDir], w) } srcDirs := make([]string, 0, len(byRepo)) @@ -159,14 +163,14 @@ func classifyAll(notes io.Writer, prs prLister, worktrees []managedWt, opts prun ctx, err := newRepoCtx(notes, prs, srcDir, opts.noFetch) if err != nil { for _, w := range byRepo[srcDir] { - results = append(results, pruneResult{wt: w, verdict: verdictKeep, reason: "repo state unknown: " + err.Error()}) + results = append(results, pruneResult{wt: w, verdict: verdictKeep, reason: "repo state unknown: " + oneLine(err.Error())}) } continue } for _, w := range byRepo[srcDir] { res, err := classify(w, ctx) if err != nil { - res = pruneResult{wt: w, verdict: verdictKeep, reason: "inspection failed: " + err.Error()} + res = pruneResult{wt: w, verdict: verdictKeep, reason: "inspection failed: " + oneLine(err.Error())} } results = append(results, res) } @@ -210,6 +214,11 @@ func report(out io.Writer, results []pruneResult, opts pruneOpts) error { return tw.Flush() } +// oneLine flattens a git error onto a single line so one row stays one row. +func oneLine(s string) string { + return strings.Join(strings.Fields(s), " ") +} + func dash(s string) string { if s == "" { return "-" diff --git a/cmd/agentws/prune_test.go b/cmd/agentws/prune_test.go index a499cff..2c21066 100644 --- a/cmd/agentws/prune_test.go +++ b/cmd/agentws/prune_test.go @@ -795,6 +795,98 @@ func TestPruneOrphanedDirectoryWhenRepoIsGone(t *testing.T) { } } +// addForeignWorktree clones a second repo under the source root and gives it a +// worktree under the worktree root, so it can be broken without touching the +// fixture's own repo. It returns the clone and the worktree path. +func (f *fixture) addForeignWorktree(t *testing.T, repo, branch string) (string, string) { + t.Helper() + src := filepath.Join(f.root, "src", repo) + git(t, filepath.Join(f.root, "src"), "clone", f.bare, src) + identity(t, src) + wt := filepath.Join(f.wtRoot, agent.WorktreeDirName(repo, branch)) + git(t, src, "worktree", "add", wt, "-b", branch, "origin/main") + return src, wt +} + +// The regression this guards: git failing for a reason that is not "the backing +// repo is gone" used to be read as orphan, and orphan deletes the directory +// outright. Here the git dir is still on disk — only its commondir pointer is +// broken, as a half-written or momentarily unreachable repo would be — so the +// verdict must be keep and the directory must survive a --yes run. +func TestPruneKeepsWorktreeWhenGitFailsButRepoExists(t *testing.T) { + f := newFixture(t) + src, wt := f.addForeignWorktree(t, "other", "benvin/unreadable") + writeCommit(t, wt, "u.txt", "u\n", "work") + gitDir, err := agent.GitDir(wt) + if err != nil { + t.Fatal(err) + } + if err := os.Remove(filepath.Join(gitDir, "commondir")); err != nil { + t.Fatal(err) + } + if _, err := agent.GitCurrentBranch(wt); err == nil { + t.Fatal("fixture did not break git in the worktree") + } + if !exists(gitDir) || !exists(filepath.Join(src, ".git")) { + t.Fatal("fixture removed the backing repo; it must still be present") + } + + srv := fakeGitea(t) + out := runOpts(t, client(srv), pruneOpts{apply: true}) + assertVerdict(t, out, "-", verdictKeep, "inspection failed") + if !exists(wt) { + t.Error("a worktree git could not be read must not be deleted") + } +} + +// The same rule one step further in: the registration is gone but the repo is +// not, so the directory is still recoverable and must not be deleted. +func TestPruneKeepsWorktreeWhenOnlyRegistrationIsGone(t *testing.T) { + f := newFixture(t) + src, wt := f.addForeignWorktree(t, "other", "benvin/deregistered") + gitDir, err := agent.GitDir(wt) + if err != nil { + t.Fatal(err) + } + if err := os.RemoveAll(gitDir); err != nil { + t.Fatal(err) + } + if !exists(filepath.Join(src, ".git")) { + t.Fatal("fixture removed the backing repo; it must still be present") + } + + srv := fakeGitea(t) + out := runOpts(t, client(srv), pruneOpts{apply: true}) + assertVerdict(t, out, "-", verdictKeep, "inspection failed") + if !exists(wt) { + t.Error("a worktree whose repo still exists must not be deleted") + } +} + +// --include-keep overrides a keep, but it must not become a second route to the +// deletion finding #1 closed: with no readable git state there is nothing to +// remove through, so the removal fails loudly and the directory stays. +func TestPruneIncludeKeepDoesNotDeleteUnreadableWorktree(t *testing.T) { + f := newFixture(t) + _, wt := f.addForeignWorktree(t, "other", "benvin/forced") + gitDir, err := agent.GitDir(wt) + if err != nil { + t.Fatal(err) + } + if err := os.Remove(filepath.Join(gitDir, "commondir")); err != nil { + t.Fatal(err) + } + + var out, errOut bytes.Buffer + err = runPrune(&out, &errOut, client(fakeGitea(t)), pruneOpts{apply: true, includeKeep: true}) + if err == nil { + t.Error("forcing removal of an unreadable worktree should fail, not succeed silently") + } + if !exists(wt) { + t.Error("--include-keep must not delete a worktree whose git state is unknown") + } +} + // --- safety signals ------------------------------------------------------- // An interrupted rebase holds sequencer state that exists nowhere else, and it