From 7ac97daa349d88c591b92d7134312ffd428f9616 Mon Sep 17 00:00:00 2001 From: Masayoshi Wada Date: Thu, 3 Sep 2026 10:40:17 +0900 Subject: [PATCH 1/2] refuse to remove a branch attached to more than one worktree --- main_test.go | 20 ++++++++++++++++++++ remove.go | 18 +++++++++++++----- remove_test.go | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 5 deletions(-) diff --git a/main_test.go b/main_test.go index f9d7be6..bad547c 100644 --- a/main_test.go +++ b/main_test.go @@ -467,6 +467,26 @@ func TestRunHookWorktreeRemoveUsesPrimaryHead(t *testing.T) { assertKept(t, repo, wtB, "agent-b") } +func TestRunHookWorktreeRemoveKeepsDuplicateBranch(t *testing.T) { + repo := newTestRepo(t) + ctx, root := loadRepoWithRoot(t, repo) + wt := mustResolve(t, ctx, repo, "agent-abc") + dup := filepath.Join(root, "dup") + gitT(t, repo, "worktree", "add", "-q", "--force", dup, "agent-abc") + + // The hook removes by branch, and the branch names two worktrees: it + // must not delete the other one in place of the path it was given. + code, _, stderr := runEda(t, repo, removeHookInput(repo, dup), "hook", "worktree-remove") + if code != 0 { + t.Fatalf("keeping a worktree is a success for the hook: exit=%d stderr=%q", code, stderr) + } + if !strings.Contains(stderr, "worktree kept") { + t.Errorf("the kept worktree must be reported on stderr, got %q", stderr) + } + assertKept(t, repo, wt, "agent-abc") + assertKept(t, repo, dup, "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 diff --git a/remove.go b/remove.go index dee2d2c..e9f30fc 100644 --- a/remove.go +++ b/remove.go @@ -78,7 +78,8 @@ func (b removeBase) headFor(path string) (oid, desc string) { // is set, it refuses to touch anything when the worktree is dirty or the // branch tip is not reachable from its base, the rule of `git branch -d`: // the base is the branch's upstream when that ref resolves, otherwise the -// HEAD that base names. +// HEAD that base names. A branch attached to more than one worktree (which +// `git worktree add --force` allows) is refused: the pair is not unique. // // All conditions are checked before any deletion. A failure between the // worktree removal and the branch deletion leaves the branch behind without @@ -91,16 +92,23 @@ func removeWorktree(ctx *repoContext, base removeBase, branch string, force bool if len(ctx.Entries) > 0 && ctx.Entries[0].Branch == branch { return refusalf("branch %q is checked out in the primary checkout; eda does not remove it", branch) } - var entry *worktreeEntry + var matches []*worktreeEntry for i, e := range ctx.Entries[1:] { if !e.Bare && !e.Detached && e.Branch == branch { - entry = &ctx.Entries[i+1] - break + matches = append(matches, &ctx.Entries[i+1]) } } - if entry == nil { + if len(matches) == 0 { return refusalf("no worktree found for branch %q", branch) } + if len(matches) > 1 { + paths := make([]string, len(matches)) + for i, e := range matches { + paths[i] = e.Path + } + return refusalf("branch %q is attached to more than one worktree (%s); detach the extra ones with `git worktree remove`", branch, strings.Join(paths, ", ")) + } + entry := matches[0] if entry.Locked { return refusalf("worktree %s is locked; unlock it with `git worktree unlock` first", entry.Path) } diff --git a/remove_test.go b/remove_test.go index b8f3721..8a4deb9 100644 --- a/remove_test.go +++ b/remove_test.go @@ -352,6 +352,54 @@ func TestRemoveWorktreeRefusesPrunable(t *testing.T) { } } +func TestRemoveWorktreeRefusesDuplicateBranch(t *testing.T) { + repo := newTestRepo(t) + ctx, root := loadRepoWithRoot(t, repo) + wt := mustResolve(t, ctx, repo, "topic") + // `git worktree add --force` attaches a branch that is already checked + // out elsewhere; the branch then names two worktrees and eda cannot + // tell which one to remove. + dup := filepath.Join(root, "dup") + gitT(t, repo, "worktree", "add", "-q", "--force", dup, "topic") + + err := removeFrom(t, repo, repo, "topic", true) + var refusal refusalError + if !errors.As(err, &refusal) { + t.Fatalf("branch attached to two worktrees must be refused, got %v", err) + } + for _, path := range []string{wt, dup} { + if !strings.Contains(err.Error(), path) { + t.Errorf("refusal must list the worktree %q, got %q", path, err) + } + } + list := gitT(t, repo, "worktree", "list", "--porcelain") + for _, path := range []string{wt, dup} { + if !strings.Contains(list, "worktree "+path+"\n") { + t.Errorf("worktree %q must stay registered, got %q", path, list) + } + } + assertKept(t, repo, wt, "topic") + assertKept(t, repo, dup, "topic") +} + +func TestRemoveWorktreeRefusesDuplicateBranchOutsideRoot(t *testing.T) { + repo := newTestRepo(t) + ctx, _ := loadRepoWithRoot(t, repo) + wt := mustResolve(t, ctx, repo, "topic") + // The duplicate outside the root is not eda's, but neither is the + // decision which attach is the real one: nothing is touched. + dup := filepath.Join(t.TempDir(), "manual-wt") + gitT(t, repo, "worktree", "add", "-q", "--force", dup, "topic") + + err := removeFrom(t, repo, repo, "topic", true) + var refusal refusalError + if !errors.As(err, &refusal) { + t.Fatalf("branch attached to two worktrees must be refused, got %v", err) + } + assertKept(t, repo, wt, "topic") + assertKept(t, repo, dup, "topic") +} + func TestRemoveWorktreeCleanCheckIgnoresStatusConfig(t *testing.T) { repo := newTestRepo(t) ctx, _ := loadRepoWithRoot(t, repo) From f5500cb293d44c6edf3476db82b1eaa3881c54d0 Mon Sep 17 00:00:00 2001 From: Masayoshi Wada Date: Thu, 3 Sep 2026 10:47:00 +0900 Subject: [PATCH 2/2] pin the duplicate refusal ahead of the unmanaged check --- remove_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/remove_test.go b/remove_test.go index 8a4deb9..e4516d3 100644 --- a/remove_test.go +++ b/remove_test.go @@ -396,6 +396,13 @@ func TestRemoveWorktreeRefusesDuplicateBranchOutsideRoot(t *testing.T) { if !errors.As(err, &refusal) { t.Fatalf("branch attached to two worktrees must be refused, got %v", err) } + // The duplicate refusal names both worktrees; the unmanaged refusal + // names only one and must not take precedence. + for _, path := range []string{wt, dup} { + if !strings.Contains(err.Error(), path) { + t.Errorf("refusal must list the worktree %q, got %q", path, err) + } + } assertKept(t, repo, wt, "topic") assertKept(t, repo, dup, "topic") }