Delete a session, and the worktree it leaves - #12
Merged
Merged
Conversation
AddWorktree and ErrNoCommits leave gitx.go for worktree.go, and their tests follow into worktree_test.go. No line of the function body changes. Size did not force this: gitx.go was 155 lines, under the limit. The category did. What stays in gitx.go is 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. Making a worktree is a different job, and RemoveWorktree needs somewhere to land that is not the file holding the rev-parse wrappers. ErrNoCommits travels because AddWorktree is its only producer. ErrNotARepo stays, because RepoRoot returns it too.
Each described behaviour the code or git does not have, and each was found by running something rather than by reading it again. AddWorktree's doc said git refuses to reuse a populated directory. Running `git worktree add -b feat <existing-empty-dir> HEAD` exits 0, so git accepts an empty one. The os.Stat check in AddWorktree is Deck's own rule and it is the stricter one: it refuses any existing path. TestRunErrorCarriesGitStderr asserted the error mentions the branch name. run formats its message as "git <args>: <stderr>", so the args already carry the branch. Replacing the stderr read with err.Error() left the test passing on "git worktree add -b main ...: exit status 255". It now asserts the message is not the bare exit status, which is the assertion that fails when the passthrough is removed. TestHoldsRepos ended with an if whose body was a bare return, as the last statement of the function. Both branches ended the test, so the one-level property it named was asserted by nothing. HoldsRepos does stop at one level. The test now builds a repository two levels down and checks that its grandparent is refused.
RemoveWorktree runs `git worktree remove` and DeleteBranch runs `git
branch -d`. Neither forces, which is the whole design: git's own refusals
are the answer rather than an obstacle to work around.
Every claim in the doc comments was measured against a real repository
rather than read off the manual.
- A tree holding modified or untracked files is refused. Untracked is
the case that catches people: an agent that wrote a file and never
added it leaves the tree dirty, with nothing in `git diff` to show
for it.
- `git branch -d` refuses a branch a worktree still has checked out,
which is why the order is worktree first and branch second. The
reverse fails on every session rather than on the ones worth
refusing.
- `git branch -d` measures merged-ness against the branch's upstream,
or against HEAD when there is none. `worktree add -b` off a local
HEAD sets none, so for a session branch it is HEAD.
- A directory the user already deleted is not an error. 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.
Each test was checked by stubbing both functions to return nil. All five
fail, so none of them is green on the fixture alone.
Both functions are exported with no production caller. The consumer is
the x modal in the next commit on this branch. If that does not land,
this commit comes out with it.
The field holds the session the open picker is about, captured because a modal outlives the keystroke that opened it. That reason is not specific to connecting, and the next commit opens a second picker that needs the same capture for the same reason. Rename only. No behaviour changes.
Stopping an agent and releasing what it held in the coordinator always happen together, and the pair was written out twice. The session-delete path would have made it three. A claim held by a process that has exited is worse than no claim, which is why the two belong in one call rather than side by side at every call site. No behaviour change. stopCurrent still reports "session is not running" before stopping anything, and still tells the coordinator only when there was a runner to stop.
x closed the session outright, so the key that keeps a worktree and the key that would remove one were the same key with no step in between. It now opens a modal holding both outcomes. Close is the row the cursor starts on. 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(+)", or "no files changed" — rather than a warning about what is about to go. A confirmation that states a fact can be answered. One that states a warning can only be believed or dismissed. A measurement that fails 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 refuses a worktree holding modified or untracked files and the session then stays, so the user can open it again, commit, 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, `branch -d` then refuses, and the notice names the branch that was kept. A session that ran in the project directory skips the modal and closes as before. It has no worktree and no branch of its own, so Delete would have nothing to remove. deleteSession guards that itself rather than trusting its caller, because such a session's Dir is the project's own checkout. The delete tests drive real git rather than a stub. The refusals are the whole feature, and a fake that refuses when told to would prove only that it was told.
Adds "Ending a session is two outcomes" to the architecture doc: the two
rows, why Close is the default, and the three rules the code has to keep —
nothing is forced, the worktree is the gate and the branch is best effort,
and a session in the project directory has nothing to delete.
It also says plainly that forget is not atomic. RemoveSession runs before
Save, so a failed save leaves the session gone from memory and still in
state.json. Making the pair atomic belongs in store, and saying so is
better than a doc that implies the guarantee.
Four stale claims the change exposed, each checked against the code:
- The dead-code paragraph named RemoveWorktree and DeleteBranch as
helpers gitx does not carry, and prescribed adding one back with the
feature that needs it. That is exactly what happened, so the
paragraph now records it. IsDirty, AheadBehind and HeadSubject are
still absent.
- The file map named picker_open.go, which was renamed to pickerctl.go.
- "One list widget, two jobs" counted two kinds. There are four.
- README's key table said x closes a session. It asks now.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
xclosed a session outright. It now opens a modal with both outcomes: Close keeps the worktree, Delete removes it and the branch.Close is the row the cursor starts on, because it is the reversible one. The Delete row carries what the session changed — "3 files changed, 41 insertions(+)", or "no files changed" — measured by
gitx.DiffagainstBaseRef. A confirmation that states a fact can be answered. One that states a warning can only be believed or dismissed.Nothing is forced.
git worktree removerefuses a tree holding modified or untracked files, andgit branch -drefuses unmerged commits. Both refusals are the answer. The worktree is the gate and the branch is best effort: a clean tree whose branch holds unmerged commits removes fine,branch -dthen refuses, and the notice names the branch that was kept.A session that ran in the project directory skips the modal and closes as before. Its
Diris the project's own checkout, sodeleteSessionguards that itself rather than trusting its caller.Commits
Each builds and passes on its own.
ifwhose body was a barereturnas the last statement of a test.connectFromtopickerSubject— rename only.stopRunnerfromstopCurrent— the delete path would have been the third copy.Worth a reviewer's eye
forgetis not atomic.RemoveSessionruns beforeSave, so a failed save leaves the session gone from memory and still instate.json, where it returns at the next launch. Putting it back is not possible from there —RemoveSessionalso drops the links the session held. The doc comment and the architecture doc both say so. Making the pair atomic belongs instore, and is not in this PR.Three things git does that were measured rather than assumed, and that shaped the design:
git worktree removerefuses a tree with untracked files, not only modified ones. An agent that wrote a file and never added it is enough to stop a delete.git branch -drefuses a branch a worktree still has checked out, which fixes the order: worktree first, branch second.The delete tests drive real git rather than a stub. A fake that refuses when told to would prove only that it was told.