Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,9 +168,12 @@ func cmdRemove(stderr io.Writer, args []string, cwd string) error {
if err != nil {
return err
}
base, err := invocationBase(ctx, cwd)
if err != nil {
return err
// A forced removal judges nothing against a base, so it takes none.
var base removeBase
if !*force {
if base, err = invocationBase(ctx, cwd); err != nil {
return err
}
}
// A single branch keeps the plain one-line error; the per-branch prefix
// and the summary would only repeat it.
Expand Down Expand Up @@ -309,12 +312,9 @@ func cmdHook(stdin io.Reader, stdout, stderr io.Writer, args []string, cwd strin
}
// The hook has no invoking worktree to take a base from (where it
// runs is not where the session started); the primary checkout's
// HEAD is the fallback base.
primaryHead, err := headCommit(ctx.PrimaryPath)
if err != nil {
return err
}
if err := removeWorktree(ctx, removeBase{PrimaryHead: primaryHead}, branch, false); err != nil {
// HEAD is the fallback base, needed only without an upstream.
base := removeBase{PrimaryHead: resolveHead(ctx.PrimaryPath)}
if err := removeWorktree(ctx, base, branch, false); err != nil {
var refusal refusalError
if errors.As(err, &refusal) {
// Keeping a worktree that still holds work is a valid
Expand Down
59 changes: 59 additions & 0 deletions main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,49 @@ func TestRunRemoveForce(t *testing.T) {
assertRemoved(t, repo, wt, "topic")
}

func TestRunRemoveForceWithUnbornPrimaryHead(t *testing.T) {
repo := newTestRepo(t)
ctx, _ := loadRepoWithRoot(t, repo)
wt := mustResolve(t, ctx, repo, "topic")
// `git switch --orphan` leaves the primary checkout on an unborn HEAD
// that resolves to no commit. A forced removal judges nothing against
// a base, so it must not need one.
gitT(t, repo, "switch", "-q", "--orphan", "orphan")

code, _, stderr := runEda(t, repo, "", "remove", "--force", "topic")
if code != 0 {
t.Fatalf("remove --force: exit=%d stderr=%q", code, stderr)
}
assertRemoved(t, repo, wt, "topic")
}

func TestRunRemoveMultipleWithUnbornPrimaryHead(t *testing.T) {
repo := newTestRepo(t)
ctx, _ := loadRepoWithRoot(t, repo)
wtA := mustResolve(t, ctx, repo, "a")
wtB := mustResolve(t, ctx, repo, "b")
wtC := mustResolve(t, ctx, repo, "c")
gitT(t, repo, "branch", "-q", "--set-upstream-to=main", "a")
gitT(t, repo, "branch", "-q", "--set-upstream-to=main", "c")
gitT(t, repo, "switch", "-q", "--orphan", "orphan")

// The unborn HEAD fails only b, which has no upstream to be judged by;
// a before it and c after it are removed through their upstreams.
code, _, stderr := runEda(t, repo, "", "remove", "a", "b", "c")
if code == 0 {
t.Fatal("a branch judged by the unborn HEAD must fail the command")
}
assertRemoved(t, repo, wtA, "a")
assertKept(t, repo, wtB, "b")
assertRemoved(t, repo, wtC, "c")
if !strings.Contains(stderr, "remove b:") || !strings.Contains(stderr, "cannot resolve HEAD") {
t.Errorf("stderr must report the unresolvable HEAD for b, got %q", stderr)
}
if !strings.Contains(stderr, "failed to remove 1 of 3") {
t.Errorf("stderr must summarize the failures, got %q", stderr)
}
}

func TestRunStatus(t *testing.T) {
repo := newTestRepo(t)
ctx, _ := loadRepoWithRoot(t, repo)
Expand Down Expand Up @@ -487,6 +530,22 @@ func TestRunHookWorktreeRemoveKeepsDuplicateBranch(t *testing.T) {
assertKept(t, repo, dup, "agent-abc")
}

func TestRunHookWorktreeRemoveWithUnbornPrimaryHead(t *testing.T) {
repo := newTestRepo(t)
ctx, _ := loadRepoWithRoot(t, repo)
wt := mustResolve(t, ctx, repo, "agent-abc")
gitT(t, repo, "branch", "-q", "--set-upstream-to=main", "agent-abc")
// The primary HEAD is the hook's fallback base; unborn, it cannot be
// resolved, but a branch judged by its upstream never needs it.
gitT(t, repo, "switch", "-q", "--orphan", "orphan")

code, _, stderr := runEda(t, repo, removeHookInput(repo, wt), "hook", "worktree-remove")
if code != 0 {
t.Fatalf("hook worktree-remove: exit=%d stderr=%q", code, stderr)
}
assertRemoved(t, repo, wt, "agent-abc")
}

func TestRunHookWorktreeRemoveFromOtherDirectory(t *testing.T) {
// The session cwd follows `cd`, so by the time the session ends it may
// be in another repository or outside any; worktree_path alone must
Expand Down
57 changes: 35 additions & 22 deletions remove.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,19 +30,37 @@ func managedWorktree(ctx *repoContext, path string) bool {
// removeBase is the HEAD a branch without a resolvable upstream is judged
// against, fixed once when the command starts so that every branch of one
// invocation sees the same commits, whatever earlier removals deleted.
// Resolving a HEAD may fail (unborn, as after `git switch --orphan`); the
// failure is kept as well and surfaces only for a branch that needs that
// HEAD, so branches judged by their upstream are unaffected. The zero value
// is the base of a forced removal, which judges nothing.
type removeBase struct {
// Top is the realpath of the worktree the command was run in, empty
// when it was not run in one (the hook), and Head is the OID its HEAD
// pointed to.
// when it was not run in one (the hook), and Head is what its HEAD
// resolved to.
Top string
Head string
// PrimaryHead is the OID of the primary checkout's HEAD. It replaces
// Head for the worktree the command was run in, and is the base for
// everything when there is no such worktree.
PrimaryHead string
Head resolvedHead
// PrimaryHead is what the primary checkout's HEAD resolved to. It
// replaces Head for the worktree the command was run in, and is the
// base for everything when there is no such worktree.
PrimaryHead resolvedHead
}

// invocationBase resolves the removeBase of a command run in cwd.
// resolvedHead is the outcome of resolving a HEAD to a commit: the OID, or
// the error to report should that HEAD be needed as a base.
type resolvedHead struct {
OID string
Err error
}

func resolveHead(dir string) resolvedHead {
oid, err := headCommit(dir)
return resolvedHead{OID: oid, Err: err}
}

// invocationBase resolves the removeBase of a command run in cwd. It fails
// when cwd is not in a worktree; a HEAD that does not resolve is recorded,
// not reported.
func invocationBase(ctx *repoContext, cwd string) (removeBase, error) {
out, err := runGit(cwd, "rev-parse", "--show-toplevel")
if err != nil {
Expand All @@ -54,24 +72,16 @@ func invocationBase(ctx *repoContext, cwd string) (removeBase, error) {
if err != nil {
return removeBase{}, fmt.Errorf("resolve invoking worktree path: %w", err)
}
head, err := headCommit(cwd)
if err != nil {
return removeBase{}, err
}
primaryHead, err := headCommit(ctx.PrimaryPath)
if err != nil {
return removeBase{}, err
}
return removeBase{Top: top, Head: head, PrimaryHead: primaryHead}, nil
return removeBase{Top: top, Head: resolveHead(cwd), PrimaryHead: resolveHead(ctx.PrimaryPath)}, nil
}

// headFor returns the base OID for the worktree at path and a description
// of it for diagnostics.
func (b removeBase) headFor(path string) (oid, desc string) {
// of it for diagnostics, or the error resolving that HEAD produced.
func (b removeBase) headFor(path string) (oid, desc string, err error) {
if b.Top == "" || path == b.Top {
return b.PrimaryHead, "the HEAD of the primary checkout"
return b.PrimaryHead.OID, "the HEAD of the primary checkout", b.PrimaryHead.Err
}
return b.Head, "the HEAD of " + b.Top
return b.Head.OID, "the HEAD of " + b.Top, b.Head.Err
}

// removeWorktree deletes a worktree and its branch as a pair. Unless force
Expand Down Expand Up @@ -132,7 +142,10 @@ func removeWorktree(ctx *repoContext, base removeBase, branch string, force bool
return err
}
if rev == "" {
rev, desc = base.headFor(entry.Path)
rev, desc, err = base.headFor(entry.Path)
if err != nil {
return fmt.Errorf("cannot judge branch %q against %s: %w", branch, desc, err)
}
}
reachable, err := isAncestor(ctx.PrimaryPath, "refs/heads/"+branch, rev)
if err != nil {
Expand Down
58 changes: 56 additions & 2 deletions remove_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,16 @@ func baseFrom(t *testing.T, ctx *repoContext, cwd string) removeBase {
return base
}

// removeFrom runs removeWorktree as a remove command started in cwd would.
// removeFrom runs removeWorktree as a remove command started in cwd would:
// a forced removal takes no base, like cmdRemove.
func removeFrom(t *testing.T, repo, cwd, branch string, force bool) error {
t.Helper()
ctx := reload(t, repo)
return removeWorktree(ctx, baseFrom(t, ctx, cwd), branch, force)
var base removeBase
if !force {
base = baseFrom(t, ctx, cwd)
}
return removeWorktree(ctx, base, branch, force)
}

// newTestClone clones a fresh test repository so branches can have a
Expand Down Expand Up @@ -252,6 +257,55 @@ func TestRemoveWorktreeLocalUpstream(t *testing.T) {
assertRemoved(t, repo, wt, "topic")
}

func TestRemoveWorktreeUnbornPrimaryHead(t *testing.T) {
repo := newTestRepo(t)
ctx, _ := loadRepoWithRoot(t, repo)
wt := mustResolve(t, ctx, repo, "topic")
// `git switch --orphan` leaves the primary checkout on an unborn HEAD
// that resolves to no commit.
gitT(t, repo, "switch", "-q", "--orphan", "orphan")

// Without an upstream the unborn HEAD is the base, and resolving it
// fails only now, for this branch.
err := removeFrom(t, repo, repo, "topic", false)
if err == nil || !strings.Contains(err.Error(), "cannot resolve HEAD") {
t.Fatalf("branch judged by the unborn HEAD must fail on it, got %v", err)
}
assertKept(t, repo, wt, "topic")

// Judged by its upstream, the branch never needs that HEAD.
gitT(t, repo, "branch", "-q", "--set-upstream-to=main", "topic")
if err := removeFrom(t, repo, repo, "topic", false); err != nil {
t.Fatalf("branch reachable from its upstream must be removable: %v", err)
}
assertRemoved(t, repo, wt, "topic")
}

func TestRemoveWorktreeUnbornInvokingHead(t *testing.T) {
repo := newTestRepo(t)
ctx, _ := loadRepoWithRoot(t, repo)
wt := mustResolve(t, ctx, repo, "topic")
wtOther := mustResolve(t, reload(t, repo), repo, "other")
// The primary HEAD stays resolvable; only the worktree the command runs
// in is left on an unborn HEAD.
gitT(t, wtOther, "switch", "-q", "--orphan", "orphan")

err := removeFrom(t, repo, wtOther, "topic", false)
if err == nil || !strings.Contains(err.Error(), "cannot resolve HEAD") {
t.Fatalf("branch judged by the unborn invoking HEAD must fail on it, got %v", err)
}
if !strings.Contains(err.Error(), "the HEAD of "+wtOther) {
t.Errorf("error must name the invoking HEAD, got %q", err)
}
assertKept(t, repo, wt, "topic")

gitT(t, repo, "branch", "-q", "--set-upstream-to=main", "topic")
if err := removeFrom(t, repo, wtOther, "topic", false); err != nil {
t.Fatalf("branch reachable from its upstream must be removable: %v", err)
}
assertRemoved(t, repo, wt, "topic")
}

func TestRemoveWorktreeGoneUpstream(t *testing.T) {
repo := newTestClone(t)
ctx, _ := loadRepoWithRoot(t, repo)
Expand Down