Merge pull request 'agentws: refuse rm on an ambiguous branch name' (#22) from benvin/agentws-rm-ambiguous into main

Reviewed-on: #22

README.md

@@ -174,8 +174,10 @@ agentws new argocd-apps --branch benvin/hotfix --from release-1.2
 # List managed worktrees (repo, branch, path)
 agentws list
 
-# Remove a worktree (by path or branch); refreshes the source repo afterwards
+# Remove a worktree (by path or branch); refreshes the source repo afterwards.
+# A branch name shared by several repos is refused; pass a path or --repo.
 agentws rm benvin/my-change
+agentws rm benvin/my-change --repo argocd-apps
 agentws rm ~/.cache/agentws/argocd-apps__benvin-my-change --delete-branch
 
 # Classify every worktree found; dry run unless --yes is given

cmd/agentws/main.go

@@ -12,7 +12,7 @@
 //
 //	agentws new <repo> [--branch benvin/<name>] [--from <base-branch>]
 //	agentws list
-//	agentws rm <path-or-branch> [--delete-branch]
+//	agentws rm <path-or-branch> [--repo <repo>] [--delete-branch]
 //	agentws prune [--yes] [--keep-branches] [--no-fetch] [--json]
 //	              [--include-unmanaged] [--include-keep]
 //	agentws clean
@@ -591,6 +591,7 @@ func underRoot(path, root string) bool {
 
 func newRmCmd() *cobra.Command {
 	var deleteBranch bool
+	var repo string
 	cmd := &cobra.Command{
 		Use:          "rm <path-or-branch>",
 		Short:        "Remove a managed worktree and refresh its source repo",
@@ -598,7 +599,7 @@ func newRmCmd() *cobra.Command {
 		SilenceUsage: true,
 		RunE: func(cmd *cobra.Command, args []string) error {
 			target := strings.TrimSpace(args[0])
-			wt, err := resolveWorktree(target)
+			wt, err := resolveWorktree(target, repo)
 			if err != nil {
 				return err
 			}
@@ -607,22 +608,41 @@ func newRmCmd() *cobra.Command {
 		},
 	}
 	cmd.Flags().BoolVar(&deleteBranch, "delete-branch", false, "Also delete the local branch after removing the worktree")
+	cmd.Flags().StringVar(&repo, "repo", "", "Only match worktrees of this repo (disambiguates a branch name)")
 	return cmd
 }
 
-// resolveWorktree finds a managed worktree by exact path or by branch name.
+// resolveWorktree finds a managed worktree by exact path or by branch name. A
-func resolveWorktree(target string) (managedWt, error) {
+// branch name shared by several repos is refused unless repo narrows it to one.
+func resolveWorktree(target, repo string) (managedWt, error) {
 	managed, err := managedWorktrees()
 	if err != nil {
 		return managedWt{}, err
 	}
 	abs, _ := filepath.Abs(target)
+	var matches []managedWt
 	for _, w := range managed {
-		if w.path == target || w.path == abs || (w.branch != "" && w.branch == target) {
+		if repo != "" && w.repo != repo {
+			continue
+		}
+		if w.path == target || w.path == abs {
 			return w, nil
 		}
+		if w.branch != "" && w.branch == target {
+			matches = append(matches, w)
+		}
 	}
-	return managedWt{}, fmt.Errorf("no managed worktree matching %q (try `agentws list`)", target)
+	switch len(matches) {
+	case 0:
+		return managedWt{}, fmt.Errorf("no managed worktree matching %q (try `agentws list`)", target)
+	case 1:
+		return matches[0], nil
+	}
+	paths := make([]string, len(matches))
+	for i, w := range matches {
+		paths[i] = "  " + w.path
+	}
+	return managedWt{}, fmt.Errorf("branch %q matches %d worktrees; pass a path or --repo:\n%s", target, len(matches), strings.Join(paths, "\n"))
 }
 
 // removeWorktree removes a managed worktree and, when asked, its local branch.

cmd/agentws/rm_test.go

@@ -0,0 +1,103 @@
+package main
+
+import (
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"git.unkin.net/unkin/agent-tools/internal/agent"
+)
+
+// addOtherRepoWorktree clones a second source repo "other" and gives it a
+// managed worktree on branch, so two repos share one branch name.
+func addOtherRepoWorktree(t *testing.T, f *fixture, branch string) string {
+	t.Helper()
+	src := filepath.Join(f.root, "src", "other")
+	git(t, filepath.Join(f.root, "src"), "clone", f.bare, src)
+	path := filepath.Join(f.wtRoot, agent.WorktreeDirName("other", branch))
+	git(t, src, "worktree", "add", path, "-b", branch, "origin/main")
+	return path
+}
+
+func TestResolveUniqueBranch(t *testing.T) {
+	f := newFixture(t)
+	want := f.addWorktree(t, "benvin/one")
+	f.addWorktree(t, "benvin/two")
+	wt, err := resolveWorktree("benvin/one", "")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if wt.path != want {
+		t.Errorf("path %q, want %q", wt.path, want)
+	}
+}
+
+func TestResolveAmbiguousBranchRefused(t *testing.T) {
+	f := newFixture(t)
+	a := f.addWorktree(t, "benvin/shared")
+	b := addOtherRepoWorktree(t, f, "benvin/shared")
+	_, err := resolveWorktree("benvin/shared", "")
+	if err == nil {
+		t.Fatal("ambiguous branch resolved, want refusal")
+	}
+	for _, p := range []string{a, b} {
+		if !strings.Contains(err.Error(), p) {
+			t.Errorf("error %q does not list candidate %s", err, p)
+		}
+	}
+}
+
+func TestRmAmbiguousBranchRemovesNothing(t *testing.T) {
+	f := newFixture(t)
+	a := f.addWorktree(t, "benvin/shared")
+	b := addOtherRepoWorktree(t, f, "benvin/shared")
+	cmd := newRootCmd()
+	cmd.SetArgs([]string{"rm", "benvin/shared"})
+	cmd.SetOut(new(strings.Builder))
+	cmd.SetErr(new(strings.Builder))
+	if err := cmd.Execute(); err == nil {
+		t.Fatal("rm succeeded on an ambiguous branch")
+	}
+	if !exists(a) || !exists(b) {
+		t.Error("ambiguous rm removed a worktree")
+	}
+}
+
+func TestResolveByPathDespiteSharedBranch(t *testing.T) {
+	f := newFixture(t)
+	f.addWorktree(t, "benvin/shared")
+	b := addOtherRepoWorktree(t, f, "benvin/shared")
+	wt, err := resolveWorktree(b, "")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if wt.path != b || wt.repo != "other" {
+		t.Errorf("got %s (%s), want %s (other)", wt.path, wt.repo, b)
+	}
+}
+
+func TestResolveRepoDisambiguates(t *testing.T) {
+	f := newFixture(t)
+	a := f.addWorktree(t, "benvin/shared")
+	b := addOtherRepoWorktree(t, f, "benvin/shared")
+	for repo, want := range map[string]string{"repo": a, "other": b} {
+		wt, err := resolveWorktree("benvin/shared", repo)
+		if err != nil {
+			t.Fatalf("--repo %s: %v", repo, err)
+		}
+		if wt.path != want {
+			t.Errorf("--repo %s: path %q, want %q", repo, wt.path, want)
+		}
+	}
+	if _, err := resolveWorktree("benvin/shared", "missing"); err == nil {
+		t.Error("--repo missing resolved, want no match")
+	}
+}
+
+func TestResolvePathOutsideRepoFilterRefused(t *testing.T) {
+	f := newFixture(t)
+	a := f.addWorktree(t, "benvin/one")
+	if _, err := resolveWorktree(a, "other"); err == nil {
+		t.Error("path in repo resolved under --repo other")
+	}
+}