diff --git a/README.md b/README.md index 0e3e42e..334d963 100644 --- a/README.md +++ b/README.md @@ -114,7 +114,7 @@ the arrows keep working inside `claude`. | `n` | new session | | `a` | add project | | `e` | rename project | -| `x` | close session | +| `x` | end session — close and keep the worktree, or delete both | | `c` | connect this session to one on another project | | `t` | theme picker | | `?` | help | diff --git a/docs/architecture.md b/docs/architecture.md index f77d594..ad8212d 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -56,7 +56,7 @@ see its siblings. ``` main.go flags, state load, cwd registration, teardown after Run internal/naming scheming-hawk-jhgk names, branch names, slugs -internal/gitx repo root, branch, worktree add +internal/gitx repo root, branch, worktree add and remove internal/termquery answers terminal queries for harnesses with no terminal internal/store projects + sessions, atomic JSON persistence: store.go the document, load and save @@ -90,6 +90,8 @@ internal/ui the Bubble Tea program, split by job: keys.go the binding table; ptykeys.go encodes to bytes selection.go cursor and list navigation actions.go forms; sessions.go and projects.go do the work + closing.go ending a session: close keeps the worktree, + delete removes it and the branch runner.go agent lifecycle; agentargs.go builds its argv dashboard.go + projectlist.go / projectdetail.go / help.go session.go + chrome.go / sidebar.go / status.go / pane.go @@ -97,7 +99,7 @@ internal/ui the Bubble Tea program, split by job: formproject.go build the forms, formfields.go the pieces, forminput.go keys browser.go the directory explorer; dirlist.go lists it - picker.go the list modal; picker_open.go opens it + picker.go the list modal; pickerctl.go opens and drives it theme.go + palette.go / styles.go internal/gittest throwaway git repositories for tests, in one place because four packages had grown their own and they had drifted @@ -106,8 +108,12 @@ probe/ debug harness: renders frames without a human present `gitx` holds only what something calls. It briefly carried `IsDirty`, `AheadBehind`, `HeadSubject`, `RemoveWorktree` and `DeleteBranch` because a git -wrapper "should" have them; all five had tests and no caller, which is how -dead code passes review. If a feature needs one, add it back with the feature. +wrapper "should" have them; all five had tests and no caller, which is how dead +code passes review. If a feature needs one, add it back with the feature. + +`RemoveWorktree` and `DeleteBranch` came back that way, with the `x` modal that +calls them. The other three are still absent, and having a test is still not +having a caller. ### Relationship to cathode @@ -541,13 +547,21 @@ screen and one keystroke from the right directory, so an error belongs there. A pick that survives opens the project form with the path prefilled and focus on the description. -### One list widget, two jobs (`ui/picker.go`) +### One list widget, four jobs (`ui/picker.go`) + +`picker` is a modal list with a `pickerKind`. It serves the theme list, any form +field marked `pickable` — currently the project field — the connect list, and +the question `x` asks about a worktree. The kinds behave differently on purpose. +The theme picker previews by applying and persists on commit. A field picker +floats over the open form, previews nothing, and only writes the chosen value +back into the field. Connecting and ending both act on commit alone, because a +link is a change to the store and ending a session is not reversible, so +previewing either as the cursor moves would do the thing being asked about. -`picker` is a modal list with a `pickerKind`. It serves the theme list and any -form field marked `pickable` — currently the project field. The kinds behave -differently on purpose: the theme picker previews by applying and persists on -commit, while a field picker floats over the open form, previews nothing, and -only writes the chosen value back into the field. +`pickerSubject` holds the session an open picker is about, captured when it +opens rather than read back on the commit key. The modal outlives the keystroke +that opened it, and the dashboard and the session view resolve "the selected +session" by different rules. The form's own choice field renders options as a row of chips and falls back to a one-at-a-time cycler when they overflow. That is right for two working-copy @@ -674,6 +688,44 @@ switching to the project directory is one `tab` away. Check `gitx.HasCommits` before anything else that assumes a resolvable HEAD — git's own "fatal: invalid reference: HEAD" is accurate and useless. +### Ending a session is two outcomes (`ui/closing.go`) + +`x` opens a modal rather than acting. **Close** forgets the record and leaves +the worktree on disk. **Delete** removes the worktree and the branch. Close is +the row the cursor starts on, because it is the reversible one and a modal that +opens on the destructive row turns a confirmation into a trap. + +The Delete row carries what the session changed — "3 files changed, 41 +insertions(+)" — measured by `gitx.Diff` against `Session.BaseRef`. A +confirmation that states a fact can be answered. One that states a warning can +only be believed or dismissed. A failed measurement says so in place of the +figure, because a row reading "no files changed" because git errored would state +a fact it does not have. + +**Nothing is forced.** `git worktree remove` refuses a tree holding modified or +untracked files and `git branch -d` refuses unmerged commits, and both refusals +are the answer rather than an obstacle. Untracked counts, which is the case that +surprises: an agent that wrote a file and never added it leaves the tree dirty +with nothing in `git diff` to show for it. The session stays, so the way out is +to open it again, commit the work, and delete it after. + +**The worktree is the gate, the branch is best effort.** A clean tree whose +branch holds unmerged commits removes fine, and `branch -d` then refuses. The +session goes and the work stays, which is the right outcome, so the notice names +the branch that was kept. + +**A session that ran in the project directory has nothing to delete.** Its `Dir` +is the project's own checkout, so it skips the modal and closes outright. +`deleteSession` guards that itself rather than trusting its caller, because the +cost of reaching it with one is git aimed at the user's own tree. + +The disk work comes first and `forget` comes last, so a worktree that refused to +go still has a row naming it. `forget` is not itself atomic: `RemoveSession` +runs before `Save`, so a failed save leaves the session gone from memory and +still in `state.json`, where it returns at the next launch. Putting it back is +not possible from there, because `RemoveSession` also drops the links the +session held. Making the pair atomic belongs in `store`. + ### An exited agent is a restart, not a dead end `landOn` drops the attachment when the cursor moves to a session with no diff --git a/docs/backlog.md b/docs/backlog.md index 722ab33..a88ee48 100644 --- a/docs/backlog.md +++ b/docs/backlog.md @@ -236,64 +236,25 @@ and three decisions around it. Numbered 13 rather than reusing 12: that number already names the public-tree notice, and a closed entry should not change meaning. -## 14. Deleting a session, and the worktree it leaves - -Closing forgets the record and keeps the worktree, which is the right default -and currently the only one. Nothing in Deck removes what it keeps, so thirty -closed **isolated** sessions are thirty trees under -`$XDG_STATE_HOME/deck/worktrees`, thirty `session/*` branches, and thirty -entries in the project's `git worktree list`. The only way out today is git by -hand. A session that ran in the project directory leaves nothing behind, which -is the distinction the fourth decision below turns on. - -`x` opens a modal with both outcomes rather than growing a second key. -**Close** keeps the worktree and is the default. **Delete** removes the worktree -and the branch. The Delete row carries `gitx.Diff` against `BaseRef` — "3 files -changed, 41 insertions" or "no files changed" — because a confirmation that -states a fact is answerable and one that states a warning is not. Deck already -measures exactly this for `analyse`, so the figure costs nothing new. - -Four decisions the code has to keep. - -**The disk work comes first, and the record is forgotten only if it succeeded.** -A dropped record over a surviving worktree is an orphan the user can no longer -see, retry or name. The order is: stop the runner, unregister from the -coordinator, remove the worktree, delete the branch, then `RemoveSession` and -`Save`. - -**Nothing is forced.** `git worktree remove` refuses a dirty tree and `git -branch -d` refuses unmerged commits. Both refusals are the answer, reported with -the path. A "delete anyway" row is the one affordance the dirty case cannot -afford, and the way out — commit it, or remove it by hand — fits in the notice. -Add force only if that refusal proves to be a real obstacle. - -**The worktree is the gate, the branch is best effort.** A clean worktree whose -branch holds unmerged commits removes fine, and then `branch -d` refuses. The -session is gone and the work is not, which is the right outcome, so the notice -says the branch was kept and why. - -**A non-isolated session has no worktree and no branch.** Its `Dir` is the -project directory itself. It skips the modal and closes as it does today, and -the delete path must never be reachable with one. `coord.workOf` already refuses -one for the neighbouring reason — a shared project directory holds everyone's -edits at once, so its changes cannot be told apart — and the same fact rules out -deleting anything on such a session's behalf. - -The guard that came out of reading the handler shipped ahead of this, because it -was small and needed nothing from the modal. `x` fired whatever column had -focus, while `dashboardSession` resolves through `listIx`, so pressing it on the -projects list closed that project's newest session — a row the keyboard was not -driving. The row was never invisible: `cursorMarker` keeps a dimmed cursor on -the unfocused column deliberately. What was missing was the focus, and -`sectionLeft` already scoped ←/→ by it for the stated reason — the focused -column is drawn with an accent border, and a key that reaches across makes that -border a lie. `focusedSession` is where the rule lives now, and `c` goes through -it too. It matters to this entry because the modal must open on the session the -user pointed at, not on whichever one `listIx` happens to hold. - -Deliberately not this: `x` on the projects column meaning "remove project". What -happens to that project's sessions and their worktrees deserves its own answer, -not one reached in passing inside a session delete. +## ~~14. Deleting a session, and the worktree it leaves~~ — done 2026-09-18 + +Shipped as **Ending a session is two outcomes** in `docs/architecture.md`, with +`gitx.RemoveWorktree` and `gitx.DeleteBranch` underneath it. + +`x` opens a modal holding both outcomes rather than growing a second key. Close +is the default because it is the reversible one. The Delete row carries +`gitx.Diff` against `BaseRef`, so the confirmation states a fact instead of a +warning. + +All four decisions this entry was waiting on held once they met real git, and +one of them turned out wider than written. `git worktree remove` refuses a tree +holding **untracked** files, not only modified ones, so an agent that wrote a +file and never added it is enough to stop a delete. That is the right refusal, +and it is a second way to reach it that this entry did not name. + +Deliberately still not this: `x` on the projects column meaning "remove +project". What happens to that project's sessions and their worktrees deserves +its own answer, not one reached in passing inside a session delete. ## ~~12. A public-repo notice a cloner will meet~~ — done 2026-08-28 diff --git a/internal/gitx/gitx.go b/internal/gitx/gitx.go index 3d580a5..f274916 100644 --- a/internal/gitx/gitx.go +++ b/internal/gitx/gitx.go @@ -4,6 +4,11 @@ // guaranteed to agree with the user's own `git worktree list`. package gitx +// Running git at all, and the questions asked of a repository before Deck +// commits to anything: where its root is, what it has checked out, whether it +// has a commit yet, and whether a directory is a project or a shelf of them. +// The worktree a session gets is worktree.go. + import ( "bytes" "errors" @@ -17,11 +22,6 @@ import ( // ErrNotARepo is returned when a path is not inside a git working tree. var ErrNotARepo = errors.New("not a git repository") -// ErrNoCommits is returned when a repository has an unborn HEAD — freshly -// initialised, nothing committed. There is no commit for a worktree to check -// out, so an isolated session is impossible until the first commit exists. -var ErrNoCommits = errors.New("repository has no commits yet") - // run executes git in dir and returns trimmed stdout. git writes its // diagnostics to stderr, so the error carries them verbatim — a wrapped // "exit status 128" on its own tells the user nothing. @@ -122,34 +122,3 @@ func HoldsRepos(dir string) bool { func HeadCommit(dir string) (string, error) { return run(dir, "rev-parse", "HEAD") } - -// AddWorktree creates branch at the current HEAD of repo and checks it out -// into dest. dest must not exist; git refuses to reuse a populated directory -// and we do not try to talk it round. -func AddWorktree(repo, dest, branch string) error { - // A project need not be a repository — it may be a directory that only - // collects them. Say that plainly rather than letting the unborn-HEAD - // check below report "no commits yet", which is true of a non-repository - // and tells the user nothing about what is actually wrong. - if _, err := RepoRoot(repo); err != nil { - // Wrapping prepends the sentinel's own text, so this half must not - // repeat it or the message reads "not a git repository: X is not a - // git repository". - return fmt.Errorf("%w: %s has no branch to work from — open the session in the project directory instead", - ErrNotARepo, filepath.Base(repo)) - } - // Check for an unborn HEAD next. Without this the caller gets git's own - // "fatal: invalid reference: HEAD", which is accurate and tells a user - // nothing about what to do next. - if !HasCommits(repo) { - return fmt.Errorf("%w: commit something first, or open the session in the project directory", ErrNoCommits) - } - if err := os.MkdirAll(filepath.Dir(dest), 0o755); err != nil { - return fmt.Errorf("create worktree parent: %w", err) - } - if _, err := os.Stat(dest); err == nil { - return fmt.Errorf("worktree path already exists: %s", dest) - } - _, err := run(repo, "worktree", "add", "-b", branch, dest, "HEAD") - return err -} diff --git a/internal/gitx/gitx_test.go b/internal/gitx/gitx_test.go index b2aa958..c60ad3e 100644 --- a/internal/gitx/gitx_test.go +++ b/internal/gitx/gitx_test.go @@ -50,77 +50,25 @@ func TestHeadBranch(t *testing.T) { } } -// TestAddWorktree covers what a session needs: a checkout of its own, on its -// own branch. -// -// There is no removal half. Deck deliberately leaves a worktree on disk -// when a session closes, because it may hold uncommitted work — so there is no -// remove function to test, and adding one for the test's sake would be code -// with no caller. -func TestAddWorktree(t *testing.T) { - repo := testRepo(t) - dest := filepath.Join(t.TempDir(), "worktrees", "scheming-hawk-jhgk") - const branch = "session/scheming-hawk-jhgk" - - if err := AddWorktree(repo, dest, branch); err != nil { - t.Fatalf("AddWorktree: %v", err) - } - if _, err := os.Stat(filepath.Join(dest, ".git")); err != nil { - t.Fatalf("worktree has no .git: %v", err) - } - got, err := HeadBranch(dest) - if err != nil { - t.Fatalf("HeadBranch in worktree: %v", err) - } - if got != branch { - t.Errorf("worktree branch = %q, want %q", got, branch) - } -} - -// TestAddWorktreeRefusesExistingPath matters because silently reusing a -// populated directory would put an agent to work in someone else's tree. -func TestAddWorktreeRefusesExistingPath(t *testing.T) { - repo := testRepo(t) - dest := filepath.Join(t.TempDir(), "taken") - if err := os.MkdirAll(dest, 0o755); err != nil { - t.Fatal(err) - } - if err := AddWorktree(repo, dest, "session/x"); err == nil { - t.Fatal("AddWorktree overwrote an existing path") - } -} - // TestRunErrorCarriesGitStderr keeps the diagnostics: a bare "exit status 128" // tells the user nothing about what git objected to. +// +// It asserts on git's own words rather than on the branch name, because run +// formats the message as "git : " and the args already contain +// the branch. The earlier assertion was that the message mentioned "main", +// which stayed green with the stderr read replaced by err.Error() — measured, +// not reasoned about. The negative assertion is the discriminating one. func TestRunErrorCarriesGitStderr(t *testing.T) { repo := testRepo(t) err := AddWorktree(repo, filepath.Join(t.TempDir(), "wt"), "main") if err == nil { t.Fatal("creating a branch that already exists succeeded") } - if !strings.Contains(err.Error(), "main") { - t.Errorf("error does not mention the branch: %v", err) + if strings.Contains(err.Error(), "exit status") { + t.Errorf("error fell back to the exit code: %v", err) } -} - -// TestAddWorktreeOnUnbornHead covers a freshly `git init`-ed repository. git's -// own message is "fatal: invalid reference: HEAD", which is accurate and tells -// a user nothing about what to do next. -func TestAddWorktreeOnUnbornHead(t *testing.T) { - // gittest.Repo initialises without committing, which is the unborn HEAD - // this test is about. - dir := gittest.Repo(t) - - if HasCommits(dir) { - t.Fatal("a repository with no commits reported HasCommits") - } - - err := AddWorktree(dir, filepath.Join(t.TempDir(), "wt"), "session/x") - if !errors.Is(err, ErrNoCommits) { - t.Fatalf("error = %v, want ErrNoCommits", err) - } - if !strings.Contains(err.Error(), "project directory") { - t.Errorf("error does not suggest the way out: %v", err) + if !strings.Contains(err.Error(), "already exists") { + t.Errorf("error does not carry git's complaint: %v", err) } } @@ -130,25 +78,6 @@ func TestHasCommits(t *testing.T) { } } -// TestAddWorktreeOnNonRepo covers a project that is a collector of -// repositories rather than one itself. Without the explicit check the -// unborn-HEAD branch reports "no commits yet", which is true of a -// non-repository and explains nothing. -func TestAddWorktreeOnNonRepo(t *testing.T) { - plain := t.TempDir() - - err := AddWorktree(plain, filepath.Join(t.TempDir(), "wt"), "session/x") - if !errors.Is(err, ErrNotARepo) { - t.Fatalf("error = %v, want ErrNotARepo", err) - } - if strings.Contains(err.Error(), "no commits") { - t.Errorf("a non-repository was reported as having no commits: %v", err) - } - if !strings.Contains(err.Error(), "project directory") { - t.Errorf("error does not suggest the way out: %v", err) - } -} - // TestHoldsRepos covers the collector check that lets `deck` seed a directory // which is not itself a repository but coordinates several that are. func TestHoldsRepos(t *testing.T) { @@ -165,9 +94,15 @@ func TestHoldsRepos(t *testing.T) { t.Error("a directory holding a repository was not recognised") } - // One level only: a grandparent is not a collector, or $HOME would be. - if HoldsRepos(filepath.Dir(collector)) == HoldsRepos(collector) { - return // sibling temp dirs may coincidentally contain repos; not asserting + // One level only: a grandparent is not a collector, or $HOME would be. The + // tree is built here rather than read from filepath.Dir(collector), whose + // contents the test does not control. + deep := t.TempDir() + if err := os.MkdirAll(filepath.Join(deep, "mid", "repo", ".git"), 0o755); err != nil { + t.Fatal(err) + } + if HoldsRepos(deep) { + t.Error("a repository two levels down made its grandparent a collector") } } diff --git a/internal/gitx/worktree.go b/internal/gitx/worktree.go new file mode 100644 index 0000000..c246f8b --- /dev/null +++ b/internal/gitx/worktree.go @@ -0,0 +1,87 @@ +package gitx + +// The worktree an isolated session works in: creating one on a branch of its +// own, removing both when the session is deleted, and the refusals on either +// side that Deck reports rather than forces. + +import ( + "errors" + "fmt" + "os" + "path/filepath" +) + +// ErrNoCommits is returned when a repository has an unborn HEAD — freshly +// initialised, nothing committed. There is no commit for a worktree to check +// out, so an isolated session is impossible until the first commit exists. +var ErrNoCommits = errors.New("repository has no commits yet") + +// AddWorktree creates branch at the current HEAD of repo and checks it out +// into dest. +// +// dest must not exist, and that is Deck's rule rather than git's. `git worktree +// add` accepts an existing empty directory, so the os.Stat below is the +// stricter check: it refuses any existing path. Reusing one would put a session +// to work in a tree that something else already owns. +func AddWorktree(repo, dest, branch string) error { + // A project need not be a repository — it may be a directory that only + // collects them. Say that plainly rather than letting the unborn-HEAD + // check below report "no commits yet", which is true of a non-repository + // and tells the user nothing about what is actually wrong. + if _, err := RepoRoot(repo); err != nil { + // Wrapping prepends the sentinel's own text, so this half must not + // repeat it or the message reads "not a git repository: X is not a + // git repository". + return fmt.Errorf("%w: %s has no branch to work from — open the session in the project directory instead", + ErrNotARepo, filepath.Base(repo)) + } + // Check for an unborn HEAD next. Without this the caller gets git's own + // "fatal: invalid reference: HEAD", which is accurate and tells a user + // nothing about what to do next. + if !HasCommits(repo) { + return fmt.Errorf("%w: commit something first, or open the session in the project directory", ErrNoCommits) + } + if err := os.MkdirAll(filepath.Dir(dest), 0o755); err != nil { + return fmt.Errorf("create worktree parent: %w", err) + } + if _, err := os.Stat(dest); err == nil { + return fmt.Errorf("worktree path already exists: %s", dest) + } + _, err := run(repo, "worktree", "add", "-b", branch, dest, "HEAD") + return err +} + +// RemoveWorktree removes dest from repo. It never forces. +// +// git refuses a tree holding modified or untracked files, and that refusal is +// the answer rather than an obstacle to work around: the tree is where the +// session's work is. Untracked counts, which is the case that catches people — +// an agent that wrote a file and never added it leaves the tree dirty by this +// test. git's message names the path and says --force exists, so the user has +// the way out without Deck offering it as a button. +// +// A dest the user already deleted by hand is not an error. git drops the +// administrative entry and reports success. +func RemoveWorktree(repo, dest string) error { + _, err := run(repo, "worktree", "remove", dest) + return err +} + +// DeleteBranch deletes branch from repo. It never forces. +// +// Call it after RemoveWorktree, never before: git refuses to delete a branch +// that a worktree still has checked out, so the reverse order fails on every +// session rather than on the ones worth refusing. +// +// -d refuses a branch whose commits are not merged. git measures against the +// branch's upstream, or against HEAD when there is none, and `worktree add -b` +// off a local HEAD sets none — so for a session branch it is HEAD, unless the +// user has configured git to set tracking on every new branch. +// +// That refusal is the case where the session goes and its work stays, so the +// caller keeps the branch and says so. A session that committed nothing sits on +// an ancestor of HEAD and deletes without complaint. +func DeleteBranch(repo, branch string) error { + _, err := run(repo, "branch", "-d", branch) + return err +} diff --git a/internal/gitx/worktree_test.go b/internal/gitx/worktree_test.go new file mode 100644 index 0000000..ad50c4d --- /dev/null +++ b/internal/gitx/worktree_test.go @@ -0,0 +1,225 @@ +package gitx + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/tripledownab/deck/internal/gittest" +) + +// session is a repository with one committed file and an isolated session's +// worktree checked out of it, which is where every removal case starts. +// +// The file is committed rather than left absent because the refusal tests need +// something tracked to modify. +func session(t *testing.T) (repo, dest, branch string) { + t.Helper() + repo, _ = gittest.RepoWith(t, "a.txt", "one\n") + dest = filepath.Join(t.TempDir(), "wt") + branch = "session/scheming-hawk-jhgk" + if err := AddWorktree(repo, dest, branch); err != nil { + t.Fatalf("AddWorktree: %v", err) + } + return repo, dest, branch +} + +// TestAddWorktree covers what a session needs: a checkout of its own, on its +// own branch. +func TestAddWorktree(t *testing.T) { + repo := testRepo(t) + dest := filepath.Join(t.TempDir(), "worktrees", "scheming-hawk-jhgk") + const branch = "session/scheming-hawk-jhgk" + + if err := AddWorktree(repo, dest, branch); err != nil { + t.Fatalf("AddWorktree: %v", err) + } + if _, err := os.Stat(filepath.Join(dest, ".git")); err != nil { + t.Fatalf("worktree has no .git: %v", err) + } + got, err := HeadBranch(dest) + if err != nil { + t.Fatalf("HeadBranch in worktree: %v", err) + } + if got != branch { + t.Errorf("worktree branch = %q, want %q", got, branch) + } +} + +// TestAddWorktreeRefusesExistingPath matters because silently reusing a +// populated directory would put an agent to work in someone else's tree. +func TestAddWorktreeRefusesExistingPath(t *testing.T) { + repo := testRepo(t) + dest := filepath.Join(t.TempDir(), "taken") + if err := os.MkdirAll(dest, 0o755); err != nil { + t.Fatal(err) + } + if err := AddWorktree(repo, dest, "session/x"); err == nil { + t.Fatal("AddWorktree overwrote an existing path") + } +} + +// TestAddWorktreeOnUnbornHead covers a freshly `git init`-ed repository. git's +// own message is "fatal: invalid reference: HEAD", which is accurate and tells +// a user nothing about what to do next. +func TestAddWorktreeOnUnbornHead(t *testing.T) { + // gittest.Repo initialises without committing, which is the unborn HEAD + // this test is about. + dir := gittest.Repo(t) + + if HasCommits(dir) { + t.Fatal("a repository with no commits reported HasCommits") + } + + err := AddWorktree(dir, filepath.Join(t.TempDir(), "wt"), "session/x") + if !errors.Is(err, ErrNoCommits) { + t.Fatalf("error = %v, want ErrNoCommits", err) + } + if !strings.Contains(err.Error(), "project directory") { + t.Errorf("error does not suggest the way out: %v", err) + } +} + +// TestAddWorktreeOnNonRepo covers a project that is a collector of +// repositories rather than one itself. Without the explicit check the +// unborn-HEAD branch reports "no commits yet", which is true of a +// non-repository and explains nothing. +func TestAddWorktreeOnNonRepo(t *testing.T) { + plain := t.TempDir() + + err := AddWorktree(plain, filepath.Join(t.TempDir(), "wt"), "session/x") + if !errors.Is(err, ErrNotARepo) { + t.Fatalf("error = %v, want ErrNotARepo", err) + } + if strings.Contains(err.Error(), "no commits") { + t.Errorf("a non-repository was reported as having no commits: %v", err) + } + if !strings.Contains(err.Error(), "project directory") { + t.Errorf("error does not suggest the way out: %v", err) + } +} + +// TestRemoveWorktreeAndBranch is the path a deleted session takes when its work +// is committed and merged: the tree goes, the registration goes, the branch +// goes. +func TestRemoveWorktreeAndBranch(t *testing.T) { + repo, dest, branch := session(t) + + if err := RemoveWorktree(repo, dest); err != nil { + t.Fatalf("RemoveWorktree: %v", err) + } + if _, err := os.Stat(dest); !os.IsNotExist(err) { + t.Errorf("worktree directory survived: stat err = %v", err) + } + // The directory going is not the same as git forgetting it. A stale entry + // keeps the path reserved and shows up in the user's own worktree list. + if list := gittest.Run(t, repo, "worktree", "list"); strings.Contains(list, dest) { + t.Errorf("git still lists the worktree:\n%s", list) + } + + if err := DeleteBranch(repo, branch); err != nil { + t.Fatalf("DeleteBranch: %v", err) + } + if out := gittest.Run(t, repo, "branch", "--list", branch); out != "" { + t.Errorf("branch --list = %q, want it gone", out) + } +} + +// TestRemoveWorktreeRefusesWork is the promise that nothing is forced. Both +// cases are the same refusal from git, and the untracked one is here because it +// is the one that surprises: an agent that wrote a file and never added it has +// left the tree dirty, with nothing in `git diff` to show for it. +func TestRemoveWorktreeRefusesWork(t *testing.T) { + for _, tc := range []struct { + name string + file string + }{ + {"modified tracked file", "a.txt"}, + {"untracked file only", "scratch.txt"}, + } { + t.Run(tc.name, func(t *testing.T) { + repo, dest, _ := session(t) + if err := os.WriteFile(filepath.Join(dest, tc.file), []byte("work\n"), 0o644); err != nil { + t.Fatal(err) + } + + err := RemoveWorktree(repo, dest) + if err == nil { + t.Fatal("RemoveWorktree deleted a tree holding work") + } + if _, statErr := os.Stat(dest); statErr != nil { + t.Errorf("a refused removal still took the directory: %v", statErr) + } + // Not asserting that the message names dest: run formats "git + // : " and dest is an argument, so that would hold + // with the stderr dropped. See TestRunErrorCarriesGitStderr. + if strings.Contains(err.Error(), "exit status") { + t.Errorf("refusal does not say what git objected to: %v", err) + } + }) + } +} + +// TestDeleteBranchRefusesUnmergedWork is the worktree-is-the-gate rule seen from +// the branch side. A session that committed and never merged leaves a clean tree +// that removes fine, and then the branch is the only copy of the work. +func TestDeleteBranchRefusesUnmergedWork(t *testing.T) { + repo, dest, branch := session(t) + if err := os.WriteFile(filepath.Join(dest, "b.txt"), []byte("work\n"), 0o644); err != nil { + t.Fatal(err) + } + gittest.Run(t, dest, "add", "b.txt") + gittest.Run(t, dest, "commit", "-q", "-m", "session work") + + if err := RemoveWorktree(repo, dest); err != nil { + t.Fatalf("a committed session left a dirty tree: %v", err) + } + + err := DeleteBranch(repo, branch) + if err == nil { + t.Fatal("DeleteBranch dropped the only copy of unmerged work") + } + // The branch name is already an argument, so asserting on it would hold with + // the stderr dropped. "not fully merged" is what only git can have said, and + // it is the sentence the caller turns into the notice. + if !strings.Contains(err.Error(), "not fully merged") { + t.Errorf("refusal does not say why the branch was kept: %v", err) + } + if out := gittest.Run(t, repo, "branch", "--list", branch); out == "" { + t.Error("the branch is gone after a refused delete") + } +} + +// TestDeleteBranchRefusesWhileCheckedOut pins the ordering DeleteBranch's doc +// states. Called before RemoveWorktree it fails on every session, merged or not, +// so getting the order wrong would look like a broken delete rather than a rule. +func TestDeleteBranchRefusesWhileCheckedOut(t *testing.T) { + repo, _, branch := session(t) + + if err := DeleteBranch(repo, branch); err == nil { + t.Fatal("DeleteBranch deleted a branch a worktree had checked out") + } + if out := gittest.Run(t, repo, "branch", "--list", branch); out == "" { + t.Error("the branch is gone after a refused delete") + } +} + +// TestRemoveWorktreeAfterTheDirectoryIsGone covers the user who deleted the +// tree by hand. git drops the registration and reports success, which is what +// lets a delete finish instead of stranding the session behind a tree that is +// already gone. +func TestRemoveWorktreeAfterTheDirectoryIsGone(t *testing.T) { + repo, dest, _ := session(t) + if err := os.RemoveAll(dest); err != nil { + t.Fatal(err) + } + + if err := RemoveWorktree(repo, dest); err != nil { + t.Fatalf("RemoveWorktree on an absent tree: %v", err) + } + if list := gittest.Run(t, repo, "worktree", "list"); strings.Contains(list, dest) { + t.Errorf("git still lists the removed worktree:\n%s", list) + } +} diff --git a/internal/ui/app_test.go b/internal/ui/app_test.go index d648c70..5f955f1 100644 --- a/internal/ui/app_test.go +++ b/internal/ui/app_test.go @@ -4,6 +4,7 @@ import ( "strings" "testing" + tea "github.com/charmbracelet/bubbletea" "github.com/tripledownab/deck/internal/store" ) @@ -175,6 +176,9 @@ func TestCloseIgnoresProjectsColumn(t *testing.T) { refused, _ := m.dashboardKey(typed("x")) after := refused.(Model) + if after.picker != nil { + t.Error("x opened the end-session modal from the projects list") + } if n := len(after.state.Sessions); n != 2 { t.Errorf("sessions = %d after x on the projects list, want both kept", n) } @@ -182,22 +186,94 @@ func TestCloseIgnoresProjectsColumn(t *testing.T) { t.Error("the refused key said nothing about why") } - // The same key still closes with the session list focused, so what refused - // above was the guard and not a close that had stopped working. + // The same key still asks with the session list focused, so what refused + // above was the guard and not a route that had stopped working. after.focusContent() - closed, _ := after.dashboardKey(typed("x")) + asked, _ := after.dashboardKey(typed("x")) + if asked.(Model).picker == nil { + t.Error("x did not open the modal with the session list focused") + } +} + +// TestEndModalClosesAndKeepsTheWorktree drives the whole route: the key, the +// modal, and the default row. +// +// Close is the row the cursor starts on, so enter without moving is the +// reversible outcome. Naming the directory is the whole promise of keeping it — +// a notice that only said "closed" would leave the work somewhere the user +// cannot find. +func TestEndModalClosesAndKeepsTheWorktree(t *testing.T) { + t.Setenv("XDG_STATE_HOME", t.TempDir()) + m := modelWith(2) + m.focusContent() + + opened, _ := m.dashboardKey(typed("x")) + asked := opened.(Model) + if asked.picker == nil { + t.Fatal("x did not open the end-session modal") + } + if got := asked.picker.selected(); got != endClose { + t.Errorf("the modal opened on %q, want the reversible row %q", got, endClose) + } + + closed, _ := asked.pickerKey(tea.KeyMsg{Type: tea.KeyEnter}) done := closed.(Model) + if done.picker != nil { + t.Error("the modal stayed up after a choice") + } if n := len(done.state.Sessions); n != 1 { - t.Errorf("sessions = %d after x with the session list focused, want 1", n) + t.Errorf("sessions = %d after choosing Close, want 1", n) } - // Naming the directory is the whole promise of keeping the worktree: a - // notice that only said "closed" would leave the work somewhere the user - // cannot find. Asserting the text, not just that there is some. if !strings.Contains(done.notice, "/worktrees/sess") { t.Errorf("notice = %q, want it to name where the worktree was kept", done.notice) } } +// TestEndModalEscapeKeepsTheSession is the reason the modal exists. x alone +// used to end a session outright, so the key had no step at which the user +// could change their mind. +func TestEndModalEscapeKeepsTheSession(t *testing.T) { + t.Setenv("XDG_STATE_HOME", t.TempDir()) + m := modelWith(2) + m.focusContent() + + opened, _ := m.dashboardKey(typed("x")) + away, _ := opened.(Model).pickerKey(tea.KeyMsg{Type: tea.KeyEsc}) + after := away.(Model) + + if after.picker != nil { + t.Error("esc left the modal up") + } + if n := len(after.state.Sessions); n != 2 { + t.Errorf("sessions = %d after esc, want both kept", n) + } +} + +// TestEndSkipsTheModalWithoutAWorktree covers a session that ran in the project +// directory. It has no worktree and no branch of its own, so Delete would have +// nothing to remove and a modal offering it would be offering nothing. +func TestEndSkipsTheModalWithoutAWorktree(t *testing.T) { + t.Setenv("XDG_STATE_HOME", t.TempDir()) + st := &store.State{} + p := st.AddProject(store.Project{Name: "demo", Path: "/demo"}) + st.AddSession(store.Session{ProjectID: p.ID, Name: "inplace", Title: "in place", Dir: "/demo"}) + m := New(st, "bash", nil) + m.focusContent() + + ended, _ := m.dashboardKey(typed("x")) + after := ended.(Model) + + if after.picker != nil { + t.Fatal("a session with no worktree was asked what to do with one") + } + if n := len(after.state.Sessions); n != 0 { + t.Errorf("sessions = %d, want the session closed outright", n) + } + if strings.Contains(after.notice, "worktree") { + t.Errorf("notice = %q, want no mention of a worktree it never had", after.notice) + } +} + // TestClosingTheLastSessionReleasesFocus keeps colContent meaning "a session is // selected", which is what focusedSession's refusal depends on. // @@ -212,7 +288,7 @@ func TestClosingTheLastSessionReleasesFocus(t *testing.T) { t.Fatal("could not focus the session list") } - m.closeSelectedFromDashboard() + m.closeSession(*m.focusedSession()) if m.focus != colProjects { t.Errorf("focus = %v after closing the last session, want the projects column", m.focus) diff --git a/internal/ui/closing.go b/internal/ui/closing.go index 722f7c9..c8974fa 100644 --- a/internal/ui/closing.go +++ b/internal/ui/closing.go @@ -1,47 +1,145 @@ package ui -// Ending a session: stopping its agent, forgetting the record, and what is -// deliberately left on disk behind it. +// Ending a session, which is two outcomes rather than one: closing forgets the +// record and keeps the worktree, deleting removes the worktree and the branch +// behind it. The modal that asks which is pickerctl.go. -// closeSelectedFromDashboard stops a session's agent and forgets it. +import ( + tea "github.com/charmbracelet/bubbletea" + "github.com/tripledownab/deck/internal/gitx" + "github.com/tripledownab/deck/internal/store" +) + +// endSelectedFromDashboard is x on the dashboard. +// +// Only an isolated session is asked the question. One that ran in the project +// directory has no worktree and no branch of its own, so there is nothing for +// Delete to remove and a modal offering it would be offering nothing. +func (m Model) endSelectedFromDashboard() (tea.Model, tea.Cmd) { + sess := m.focusedSession() + if sess == nil { + return m, nil + } + if !sess.Isolated { + m.closeSession(*sess) + return m, nil + } + return m.openEndSessionPicker(sess) +} + +// endPicked carries out the choice the modal collected. +// +// It resolves the session from pickerSubject rather than from the cursor: the +// figure on the Delete row was measured for that session when the modal opened, +// and a confirmation that states a fact has to act on the thing it measured. +func (m Model) endPicked(choice string) (tea.Model, tea.Cmd) { + sess := m.state.Session(m.pickerSubject) + if sess == nil { + return m, nil + } + if choice == endDelete { + m.deleteSession(*sess) + } else { + m.closeSession(*sess) + } + return m, nil +} + +// closeSession stops a session's agent and forgets it. // // The worktree is left on disk on purpose. It may hold uncommitted work, and // deleting a branch's only checkout to tidy a list is not a trade Deck // gets to make silently. The notice says where it went. -func (m *Model) closeSelectedFromDashboard() { - p := m.currentProject() - selected := m.focusedSession() - if p == nil || selected == nil { +func (m *Model) closeSession(sess store.Session) { + m.stopRunner(sess.ID) + if !m.forget(sess) { + return + } + if sess.Isolated { + m.notice = "closed " + sess.Name + " — worktree kept at " + sess.Dir + } else { + m.notice = "closed " + sess.Name + } +} + +// deleteSession closes a session and removes the worktree and branch behind it. +// +// Nothing is forced. git refuses a worktree holding modified or untracked +// files, and that refusal is the answer: the session stays, so the user can +// open it again, commit the work, and delete it after. +// +// The worktree is the gate and the branch is best effort. A clean tree whose +// branch holds unmerged commits removes fine and then `branch -d` refuses, +// which is the right outcome — the session goes and the work stays — so the +// notice names the branch that was kept. +func (m *Model) deleteSession(sess store.Session) { + p := m.state.Project(sess.ProjectID) + // Guarded here and not only where the modal opens. A non-isolated session's + // Dir is the project directory itself, so a delete that reached this would + // aim git at the user's own checkout. + if p == nil || !sess.Isolated { + m.notice = sess.Name + " has no worktree of its own to delete" + return + } + + // The agent stops before the tree is touched, because git counts a file it + // is still writing and cannot be asked to wait. A refusal below therefore + // leaves the session in place with its agent stopped, which Deck already + // treats as a restart rather than a dead end. + m.stopRunner(sess.ID) + if err := gitx.RemoveWorktree(p.Path, sess.Dir); err != nil { + m.fault = err + return + } + + kept := "" + if sess.Branch != "" { + if err := gitx.DeleteBranch(p.Path, sess.Branch); err != nil { + kept = sess.Branch + } + } + + if !m.forget(sess) { return } - sess := *selected - if r, ok := m.runners[sess.ID]; ok { - r.Stop() - delete(m.runners, sess.ID) + m.notice = "deleted " + sess.Name + if kept != "" { + m.notice += " — branch " + kept + " kept, it holds unmerged commits" } - m.releaseCoord(sess.ID) +} + +// forget drops the record and saves, then puts the lists and the cursor back in +// a consistent state. It reports whether the save succeeded. +// +// Callers do their disk work first and call this last, so a worktree that +// refused to go still has a row naming it. The reverse order would leave an +// orphan the user can no longer see, retry or delete through Deck. +// +// This is not itself atomic, and the comment says so rather than implying +// otherwise. RemoveSession runs before Save, so a failed Save leaves the +// session gone from memory and still in state.json, and it returns at the next +// launch. Putting it back is not possible from here: RemoveSession also drops +// the links this session held, and the copy passed in does not carry them. +// Making the pair atomic belongs in store, not in a caller working around it. +func (m *Model) forget(sess store.Session) bool { // RemoveSession drops the links this session held, so the coordinator has // to be told: releaseCoord frees claims and the inbox, but peers is set // wholesale and outlives an agent exiting on purpose. m.state.RemoveSession(sess.ID) if err := m.state.Save(); err != nil { m.fault = err - return + return false } m.syncConnections() m.rebuildRows() - left := m.state.SessionsFor(p.ID) + left := m.state.SessionsFor(sess.ProjectID) m.listIx = clamp(m.listIx, 0, max(len(left)-1, 0)) - // Closing the last one leaves the cursor in a column with nothing in it — + // Ending the last one leaves the cursor in a column with nothing in it — // the state focusContent refuses to create, reached from the other side. // focusedSession's promise that a refusal says why rests on this: with no // sessions there is nothing for it to name, so it would refuse in silence. if len(left) == 0 { m.focus = colProjects } - if sess.Isolated { - m.notice = "closed " + sess.Name + " — worktree kept at " + sess.Dir - } else { - m.notice = "closed " + sess.Name - } + return true } diff --git a/internal/ui/closing_test.go b/internal/ui/closing_test.go new file mode 100644 index 0000000..0395b6a --- /dev/null +++ b/internal/ui/closing_test.go @@ -0,0 +1,188 @@ +package ui + +// Deleting a session against a real repository. The refusals are the point of +// the feature, so they are driven through git rather than a stub: a fake that +// refuses when told to would prove only that it was told. + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/tripledownab/deck/internal/gittest" + "github.com/tripledownab/deck/internal/gitx" + "github.com/tripledownab/deck/internal/store" +) + +// deckWithWorktree builds a model holding one project that is a real +// repository and one isolated session with a real worktree checked out of it. +func deckWithWorktree(t *testing.T) (Model, store.Session) { + t.Helper() + t.Setenv("XDG_STATE_HOME", t.TempDir()) + + repo, head := gittest.RepoWith(t, "a.txt", "one\n") + dest := filepath.Join(t.TempDir(), "wt") + const branch = "session/quiet-otter-abcd" + if err := gitx.AddWorktree(repo, dest, branch); err != nil { + t.Fatalf("AddWorktree: %v", err) + } + + st := &store.State{} + p := st.AddProject(store.Project{Name: "demo", Path: repo}) + sess := st.AddSession(store.Session{ + ProjectID: p.ID, + Name: "quiet-otter-abcd", + Title: "work", + Branch: branch, + BaseRef: head, + Dir: dest, + Isolated: true, + }) + m := New(st, "bash", nil) + m.focusContent() + return m, *sess +} + +// TestDeleteRemovesTheWorktreeAndBranch is the outcome the feature exists for: +// a session that changed nothing leaves nothing behind. +func TestDeleteRemovesTheWorktreeAndBranch(t *testing.T) { + m, sess := deckWithWorktree(t) + repo := m.state.Project(sess.ProjectID).Path + + m.deleteSession(sess) + + if m.fault != nil { + t.Fatalf("deleting a clean session reported: %v", m.fault) + } + if _, err := os.Stat(sess.Dir); !os.IsNotExist(err) { + t.Errorf("the worktree survived: stat err = %v", err) + } + if out := gittest.Run(t, repo, "branch", "--list", sess.Branch); out != "" { + t.Errorf("branch --list = %q, want the branch gone", out) + } + if n := len(m.state.Sessions); n != 0 { + t.Errorf("sessions = %d, want the record forgotten", n) + } +} + +// TestDeleteRefusesADirtyWorktree is the promise that nothing is forced. The +// file is never added, because untracked is the case that catches people: an +// agent that wrote one and stopped has left work with nothing in `git diff` to +// show for it. +func TestDeleteRefusesADirtyWorktree(t *testing.T) { + m, sess := deckWithWorktree(t) + if err := os.WriteFile(filepath.Join(sess.Dir, "scratch.txt"), []byte("work\n"), 0o644); err != nil { + t.Fatal(err) + } + + m.deleteSession(sess) + + if m.fault == nil { + t.Fatal("deleting a tree holding work reported nothing") + } + if _, err := os.Stat(sess.Dir); err != nil { + t.Errorf("a refused delete still took the directory: %v", err) + } + // The session has to survive its own refused delete. A record dropped over + // a surviving worktree is an orphan the user can no longer see or retry. + if n := len(m.state.Sessions); n != 1 { + t.Errorf("sessions = %d after a refused delete, want the session kept", n) + } +} + +// TestDeleteKeepsAnUnmergedBranch is the worktree-is-the-gate rule. A session +// that committed leaves a clean tree, so the tree goes and the branch is then +// the only copy of the work. git refuses it, the session still goes, and the +// notice is the only place the user learns where the work is. +func TestDeleteKeepsAnUnmergedBranch(t *testing.T) { + m, sess := deckWithWorktree(t) + repo := m.state.Project(sess.ProjectID).Path + if err := os.WriteFile(filepath.Join(sess.Dir, "b.txt"), []byte("work\n"), 0o644); err != nil { + t.Fatal(err) + } + gittest.Run(t, sess.Dir, "add", "b.txt") + gittest.Run(t, sess.Dir, "commit", "-q", "-m", "session work") + + m.deleteSession(sess) + + if m.fault != nil { + t.Fatalf("a committed session was refused: %v", m.fault) + } + if _, err := os.Stat(sess.Dir); !os.IsNotExist(err) { + t.Errorf("the worktree survived: stat err = %v", err) + } + if out := gittest.Run(t, repo, "branch", "--list", sess.Branch); out == "" { + t.Error("the branch holding the only copy of the work was deleted") + } + if n := len(m.state.Sessions); n != 0 { + t.Errorf("sessions = %d, want the record forgotten", n) + } + if !strings.Contains(m.notice, sess.Branch) { + t.Errorf("notice = %q, want it to name the branch that was kept", m.notice) + } +} + +// TestDeleteRefusesASessionWithoutAWorktree guards the case the modal is not +// supposed to reach. A non-isolated session's Dir is the project directory, so +// a delete that got here would aim git at the user's own checkout. +func TestDeleteRefusesASessionWithoutAWorktree(t *testing.T) { + t.Setenv("XDG_STATE_HOME", t.TempDir()) + repo, _ := gittest.RepoWith(t, "a.txt", "one\n") + + st := &store.State{} + p := st.AddProject(store.Project{Name: "demo", Path: repo}) + sess := st.AddSession(store.Session{ + ProjectID: p.ID, Name: "inplace", Title: "in place", Dir: repo, + }) + m := New(st, "bash", nil) + + m.deleteSession(*sess) + + if _, err := os.Stat(filepath.Join(repo, "a.txt")); err != nil { + t.Fatalf("the project's own checkout was touched: %v", err) + } + if n := len(m.state.Sessions); n != 1 { + t.Errorf("sessions = %d, want the session kept", n) + } + if m.notice == "" { + t.Error("the refusal said nothing about why") + } +} + +// TestWorkSummaryIsOneLine keeps the Delete row answerable. gitx.Work.Stat is +// one line per changed file followed by the total, and only the total fits. +func TestWorkSummaryIsOneLine(t *testing.T) { + _, sess := deckWithWorktree(t) + for _, name := range []string{"b.txt", "c.txt"} { + if err := os.WriteFile(filepath.Join(sess.Dir, name), []byte("work\n"), 0o644); err != nil { + t.Fatal(err) + } + } + + got := workSummary(sess) + + if strings.Contains(got, "\n") { + t.Errorf("summary spans lines, so the row cannot hold it:\n%s", got) + } + if !strings.Contains(got, "2 files changed") { + t.Errorf("summary = %q, want the total across both new files", got) + } +} + +// TestWorkSummaryReportsAFailedMeasurement covers a session with no recorded +// base. Answering "no files changed" there would be a confirmation stating a +// fact it does not have, which is worse than one admitting it could not look. +func TestWorkSummaryReportsAFailedMeasurement(t *testing.T) { + _, sess := deckWithWorktree(t) + sess.BaseRef = "" + + got := workSummary(sess) + + if strings.Contains(got, "no files changed") { + t.Errorf("summary = %q, want it to admit the measurement failed", got) + } + if !strings.Contains(got, "could not measure") { + t.Errorf("summary = %q, want it to say the measurement failed", got) + } +} diff --git a/internal/ui/connections.go b/internal/ui/connections.go index d7af16f..75de4c2 100644 --- a/internal/ui/connections.go +++ b/internal/ui/connections.go @@ -67,14 +67,14 @@ func (m Model) openConnectPicker(sess *store.Session) (tea.Model, tea.Cmd) { m.notice = "no sessions on another project to connect to" return m, nil } - m.connectFrom = sess.ID + m.pickerSubject = sess.ID m.picker = newPicker(pickConnect, "Connect "+sessionLabel(*sess)+" to", rows, "") return m, nil } // connectPicked links or unlinks the chosen session and persists the result. func (m Model) connectPicked(otherID string) (tea.Model, tea.Cmd) { - sess := m.state.Session(m.connectFrom) + sess := m.state.Session(m.pickerSubject) other := m.state.Session(otherID) if sess == nil || other == nil { return m, nil diff --git a/internal/ui/format.go b/internal/ui/format.go index cb7bc2b..4162fa8 100644 --- a/internal/ui/format.go +++ b/internal/ui/format.go @@ -6,7 +6,9 @@ package ui import ( "fmt" "path/filepath" + "strings" + "github.com/tripledownab/deck/internal/gitx" "github.com/tripledownab/deck/internal/store" ) @@ -56,3 +58,22 @@ func projectLabel(state *store.State, projectID string) string { } return filepath.Base(p.Path) } + +// workSummary is the one line a modal row can hold about what a session +// changed: "3 files changed, 41 insertions(+)", or "no files changed". +// +// gitx.Work.Stat is one line per changed file followed by that total, and the +// total is the last of them. Diff substitutes "no files changed" for an empty +// stat, so there is always a line to take. +// +// A failed measurement is reported in place of the figure. A row that said "no +// files changed" because git errored would be a confirmation stating a fact it +// does not have, which is worse than one admitting it could not look. +func workSummary(sess store.Session) string { + w, err := gitx.Diff(sess.Dir, sess.BaseRef) + if err != nil { + return "could not measure what it changed: " + err.Error() + } + lines := strings.Split(strings.TrimSpace(w.Stat), "\n") + return strings.TrimSpace(lines[len(lines)-1]) +} diff --git a/internal/ui/keyroutes.go b/internal/ui/keyroutes.go index d32d852..f3db165 100644 --- a/internal/ui/keyroutes.go +++ b/internal/ui/keyroutes.go @@ -39,7 +39,7 @@ func (m Model) dashboardKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { case key.Matches(msg, m.keys.Theme): return m.openThemePicker() case key.Matches(msg, m.keys.Delete): - m.closeSelectedFromDashboard() + return m.endSelectedFromDashboard() case key.Matches(msg, m.keys.Enter): return m.openFromDashboard() } diff --git a/internal/ui/keys.go b/internal/ui/keys.go index 722f576..ba2c0d9 100644 --- a/internal/ui/keys.go +++ b/internal/ui/keys.go @@ -64,7 +64,7 @@ func defaultKeys() keyMap { NewSession: key.NewBinding(key.WithKeys("n"), key.WithHelp("n", "new session")), AddProject: key.NewBinding(key.WithKeys("a"), key.WithHelp("a", "add project")), Rename: key.NewBinding(key.WithKeys("e"), key.WithHelp("e", "rename project")), - Delete: key.NewBinding(key.WithKeys("x"), key.WithHelp("x", "close session")), + Delete: key.NewBinding(key.WithKeys("x"), key.WithHelp("x", "end session")), Connect: key.NewBinding(key.WithKeys("c"), key.WithHelp("c", "connect to a session elsewhere")), Theme: key.NewBinding(key.WithKeys("t"), key.WithHelp("t", "theme")), diff --git a/internal/ui/model.go b/internal/ui/model.go index ad76f9e..bb9a62e 100644 --- a/internal/ui/model.go +++ b/internal/ui/model.go @@ -55,14 +55,15 @@ type Model struct { picker *picker showHelp bool - // connectFrom is the session the open connect picker is linking from. + // pickerSubject is the session the open picker is about: the one a connect + // is linking from, or the one the end-session modal will close or delete. // // Captured when the picker opens rather than resolved on the commit key, // for the same reason picker.restore is: the modal outlives the keystroke // that opened it, and the dashboard and the session view resolve "the - // selected session" by different rules. Re-reading it would connect + // selected session" by different rules. Re-reading it would act on // whichever session the other screen happens to be pointing at. - connectFrom string + pickerSubject string // notice is a one-line message in the footer; fault is a failure the user // must see, and it outranks the notice. diff --git a/internal/ui/picker.go b/internal/ui/picker.go index a2bdb8e..71e57a0 100644 --- a/internal/ui/picker.go +++ b/internal/ui/picker.go @@ -18,6 +18,7 @@ const ( pickTheme pickerKind = iota pickProject pickConnect + pickEndSession ) // picker is a modal list: a title, rows, a cursor. diff --git a/internal/ui/pickerctl.go b/internal/ui/pickerctl.go index 18aaed6..06eefbb 100644 --- a/internal/ui/pickerctl.go +++ b/internal/ui/pickerctl.go @@ -8,6 +8,15 @@ import ( "fmt" tea "github.com/charmbracelet/bubbletea" + "github.com/tripledownab/deck/internal/store" +) + +// The two rows of the end-session modal. They are ids rather than labels +// because the label is what the user reads and the id is what endPicked +// branches on, and a row renamed for clarity must not change what it does. +const ( + endClose = "close" + endDelete = "delete" ) // openThemePicker opens the theme list, positioned on the active theme. @@ -32,6 +41,25 @@ func (m Model) openFieldPicker() (tea.Model, tea.Cmd) { return m, nil } +// openEndSessionPicker asks what ending a session should do with the worktree +// behind it. +// +// Close is the row the cursor starts on. It is the reversible outcome, and a +// modal that opens on the destructive one turns a confirmation into a trap. +// +// The Delete row carries what that session changed rather than a warning about +// what it is about to lose. A confirmation that states a fact can be answered; +// one that states a warning can only be believed or dismissed. +func (m Model) openEndSessionPicker(sess *store.Session) (tea.Model, tea.Cmd) { + rows := []pickerRow{ + {id: endClose, label: "Close", desc: "keep the worktree at " + sess.Dir}, + {id: endDelete, label: "Delete", desc: "remove the worktree and branch — " + workSummary(*sess)}, + } + m.pickerSubject = sess.ID + m.picker = newPicker(pickEndSession, "End "+sessionLabel(*sess), rows, endClose) + return m, nil +} + // pickerKey drives the theme picker. // // The palette is applied as the cursor moves, so the whole frame behind the @@ -56,6 +84,21 @@ func (m Model) pickerKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { return m, nil } + // Ending previews nothing, and esc is the way out of a question rather than + // a way to undo an answer. The modal exists so the destructive row costs a + // second, deliberate keystroke. + if m.picker.kind == pickEndSession { + switch { + case cancel: + m.picker = nil + case commit: + choice := m.picker.selected() + m.picker = nil + return m.endPicked(choice) + } + return m, nil + } + // A field picker floats over the open form and only writes back into it. // Nothing is previewed and nothing is persisted, so cancel is simply // closing it. diff --git a/internal/ui/runner.go b/internal/ui/runner.go index 4613278..a62d2f7 100644 --- a/internal/ui/runner.go +++ b/internal/ui/runner.go @@ -57,18 +57,29 @@ func (m *Model) stopCurrent() { if sess == nil { return } - r, ok := m.runners[sess.ID] - if !ok { + if _, ok := m.runners[sess.ID]; !ok { m.notice = "session is not running" return } - r.Stop() - delete(m.runners, sess.ID) - m.releaseCoord(sess.ID) + m.stopRunner(sess.ID) m.attached = false m.notice = "stopped " + sess.Name } +// stopRunner stops a session's agent and frees what it held in the +// coordinator. +// +// The two always happen together, which is why they are one call: a claim held +// by a process that has exited is worse than no claim. Stopping a session that +// was not running is not an error, so the coordinator is told either way. +func (m *Model) stopRunner(sessionID string) { + if r, ok := m.runners[sessionID]; ok { + r.Stop() + delete(m.runners, sessionID) + } + m.releaseCoord(sessionID) +} + // releaseCoord drops a session from the coordinator, freeing its claims. A // claim held by a process that has exited is worse than no claim: the next // agent believes someone is working there. @@ -80,7 +91,7 @@ func (m *Model) releaseCoord(sessionID string) { // sweepExited releases sessions whose agent stopped without being told to. // -// stopCurrent and closeSelectedFromDashboard cover the deliberate paths, but +// stopCurrent and stopRunner cover the deliberate paths, but // an agent can also leave on its own — /exit, a crash, the process being // killed. Nothing calls back into the UI when that happens, so without this // sweep the coordinator keeps listing a dead session and holding its claims,