From 76abf2232496c34d997085f64b184f1776a7a087 Mon Sep 17 00:00:00 2001 From: omartelo Date: Fri, 21 Aug 2026 00:31:20 -0300 Subject: [PATCH] fix: open a worktree on a branch that already exists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every worktree creation went to git as `worktree add -b`, which creates the branch — so a branch that was already there failed with "A branch or worktree with that name already exists", and the only way through was to delete it first. A branch prepared before the session cannot afford that: cut from a freshly fetched remote, pushed, and linked to a GitHub issue. An existing branch is now checked out as it stands, on the New worktree dialog, `lich open --worktree` and the `open_session` tool alike — all three route through project.CreateWorktree, and the GUI reaches it without passing spawn's resolveCheckout, so the guard belongs here. Naming a branch is naming the work on it, which is already how spawn.resolveCheckout reads a branch that holds a worktree. The base drops out with it: it says where a branch starts, and that one already started. That opens a path CreateWorktree could not reach before — a branch held by another checkout — and git reworded that refusal in 2.36, from "is already checked out at" to "is already used by worktree at". Only the old wording was mapped, so modern git fell through to "git could not complete the operation". Both are matched now; both are live, depending on the distribution's git. TestCreateWorktreeErrors covered the old refusal as contract. The contract changed, so that case moved to TestCreateWorktreeExistingBranch and the table now pins what git still refuses. Fixes #334 --- CHANGELOG.md | 13 ++++++ docs/cli.md | 20 ++++++--- internal/cli/cli.go | 4 +- internal/cli/mcp.go | 8 ++-- internal/project/giterror.go | 4 ++ internal/project/giterror_test.go | 8 +++- internal/project/worktree.go | 37 ++++++++++------ internal/project/worktree_test.go | 72 +++++++++++++++++++++++++++++-- 8 files changed, 138 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2459d8d87..c73b9055c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **A worktree opens on a branch that already exists, instead of refusing it.** + Every worktree lich made was handed to git as `worktree add -b`, which creates + the branch — so naming a branch that was already there failed with "A branch or + worktree with that name already exists", and the only way through was to delete + the branch first. That is exactly what a prepared branch cannot afford: cut + from a freshly fetched remote, pushed, linked to a GitHub issue. An existing + branch is now checked out as it stands, on the New worktree dialog, on + `lich open --worktree` and on the `open_session` tool alike — naming a branch + is naming the work on it, which is already how a branch holding a worktree is + read. The base goes unused in that case: it says where a branch starts, and + that one already started. A branch checked out somewhere else is still refused, + and now says so on modern git too, which reworded that refusal and had been + falling through to "git could not complete the operation". - **The Files tab browses a folder that is not a git repository.** The tree was built from `git ls-files`, so a project opened on a plain directory — notes, a scratch folder, a checkout of something that is not git — answered "Not a git diff --git a/docs/cli.md b/docs/cli.md index 9c12841ae..e14fa8b59 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -244,16 +244,24 @@ It answers to "auth-fix" and to "auth-fix-9f8e". Its agent may still be starting `opencode`, `omp`, `crush`) or `shell`. The default is the caller's own provider, so an agent opening a worker gets another of itself; a caller that is not a session at all gets Claude Code. -- `--worktree` is the **branch name** of a new git worktree, created off - `--base` (the project's current branch by default) under the app data dir. The - session is rooted there and labelled after it, and the project's worktree - setup script (Settings › Project) runs in its terminal before the provider, - exactly as when the window creates one. Without it the session opens in the - project's own directory, beside the caller's. +- `--worktree` is the **branch name** of a git worktree, created off `--base` + (the project's current branch by default) under the app data dir. The session + is rooted there and labelled after it, and the project's worktree setup script + (Settings › Project) runs in its terminal before the provider, exactly as when + the window creates one. Without it the session opens in the project's own + directory, beside the caller's. + **A branch that already exists is checked out as it stands**, never recreated: + naming a branch is naming the work on it, and a caller that cannot see the + repository has no way to tell the two apart. That covers a branch prepared + before the session — cut from an updated remote, linked to an issue — and a + worktree closed but kept, which is resumed rather than made again. Only a + branch already checked out somewhere else is refused, by git itself. - `--base` is checked against the repository's branches — local, remote (`origin/…`, fetched and tracked), or one another worktree already holds. A base that is not a branch is refused rather than resolved: git would happily branch off a typo that names a revision, leaving a checkout nobody asked for. + It is **ignored when `--worktree` names a branch that already exists**: a base + says where a branch starts, and that one already started. - `--model` is the model the session's provider starts on. The value is whatever that provider's own `--model` takes — an alias, a full name, a `provider/model` pair — and lich passes it through unchecked: the accepted diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 108c026b5..3c19b5821 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -316,8 +316,8 @@ func (c *client) open(args []string) error { flags := newFlagSet("open") project := flags.String("project", "", "project to open the session in; defaults to the caller's own") kind := flags.String("kind", "", "what the session runs; defaults to the caller's own provider") - worktree := flags.String("worktree", "", "branch name of a new git worktree to root the session in") - base := flags.String("base", "", "branch the worktree starts from; defaults to the project's current branch") + worktree := flags.String("worktree", "", "branch name of a git worktree to root the session in; an existing branch is checked out as it stands") + base := flags.String("base", "", "branch a new worktree starts from; defaults to the project's current branch, ignored when the branch exists") model := flags.String("model", "", "model the provider runs, in the provider's own spelling") prompt := flags.String("prompt", "", "task to hand the new session as soon as its agent is up") asJSON := flags.Bool("json", false, "print the result as JSON") diff --git a/internal/cli/mcp.go b/internal/cli/mcp.go index 4475f13b4..2b88e207c 100644 --- a/internal/cli/mcp.go +++ b/internal/cli/mcp.go @@ -392,10 +392,12 @@ var mcpTools = []mcpTool{ "What the session runs: claude, codex, opencode, omp, crush, or shell. "+ "Defaults to the same agent you are."), "worktree": property("string", - "Branch name for a new git worktree to root the session in. Omit to open the "+ - "session in the project's own directory, beside yours."), + "Branch name for a git worktree to root the session in. A branch that already "+ + "exists is checked out as it stands; one that does not is created. Omit to "+ + "open the session in the project's own directory, beside yours."), "base": property("string", - "Branch the new worktree starts from. Defaults to the project's current branch."), + "Branch a new worktree starts from. Defaults to the project's current branch, "+ + "and is ignored when the worktree branch already exists."), "model": property("string", "Model the new session's provider runs, spelled exactly as that provider's own "+ "--model flag takes it — the name or alias, never a lich name for it. Omit "+ diff --git a/internal/project/giterror.go b/internal/project/giterror.go index 73bbcca74..b6409d69e 100644 --- a/internal/project/giterror.go +++ b/internal/project/giterror.go @@ -16,7 +16,11 @@ func gitFailure(args []string, stderr string, runErr error) error { var gitFailures = []toolFailure{ {"not a git repository", "This folder is not a git repository."}, {"already exists", "A branch or worktree with that name already exists."}, + // Two wordings for one refusal: git said "is already checked out at" until + // 2.36 and "is already used by worktree at" since. Both are live — the + // distribution git decides which one a user sees. {"is already checked out", "That branch is already checked out in another worktree."}, + {"is already used by worktree", "That branch is already checked out in another worktree."}, { "contains modified or untracked files", "That worktree has uncommitted changes. Remove it from the sidebar to discard them.", diff --git a/internal/project/giterror_test.go b/internal/project/giterror_test.go index a5dcb744b..565552c8f 100644 --- a/internal/project/giterror_test.go +++ b/internal/project/giterror_test.go @@ -26,11 +26,17 @@ func TestGitMessage(t *testing.T) { "A branch or worktree with that name already exists.", }, { - "a branch checked out elsewhere", + "a branch checked out elsewhere, git before 2.36", "fatal: 'fix/poll' is already checked out at '/data/worktrees/p/fix'", errTest, "That branch is already checked out in another worktree.", }, + { + "a branch checked out elsewhere, git since 2.36", + "fatal: 'fix/poll' is already used by worktree at '/data/worktrees/p/fix'", + errTest, + "That branch is already checked out in another worktree.", + }, { "a dirty worktree git refuses to remove", "fatal: '/data/worktrees/p/fix' contains modified or untracked files, use --force to delete it", diff --git a/internal/project/worktree.go b/internal/project/worktree.go index 503cf7eb1..993a626c7 100644 --- a/internal/project/worktree.go +++ b/internal/project/worktree.go @@ -200,8 +200,9 @@ func reserveWorktreePath(projectPath, projectID, name string) (string, error) { // CreateWorktree creates a git worktree named name (random when empty) under // the app data dir, branching off base. A remote base is fetched first and the -// new branch tracks it. The worktree is verified usable before returning, so a -// success here means a session can be opened at Path right away. +// new branch tracks it. A branch that already exists is checked out as it +// stands, and base is ignored. The worktree is verified usable before +// returning, so a success here means a session can be opened at Path right away. func (s *Service) CreateWorktree(projectPath, projectID, name, base string, baseIsRemote bool) (*Worktree, error) { if name == "" { name = randomWorktreeName(func(n string) bool { return branchExists(projectPath, n) }) @@ -211,19 +212,31 @@ func (s *Service) CreateWorktree(projectPath, projectID, name, base string, base return nil, err } + // A branch that already exists is checked out as it stands, never recreated: + // naming a branch is naming the work on it, the same reading + // spawn.resolveCheckout gives a branch that already has a checkout. git's + // -b refuses a name it already knows, so the only way to open a session on a + // branch prepared in advance used to be deleting it first. base drops out + // with it — it decides where a branch starts, and this one already started. args := []string{"worktree", "add"} - if baseIsRemote { - remote, branch, ok := strings.Cut(base, "/") - if !ok { - return nil, fmt.Errorf("%q is not a remote branch lich can track.", base) - } - if _, err := runGit(projectPath, "fetch", "--", remote, branch); err != nil { - return nil, err + source := base + if branchExists(projectPath, name) { + source = name + } else { + if baseIsRemote { + remote, branch, ok := strings.Cut(base, "/") + if !ok { + return nil, fmt.Errorf("%q is not a remote branch lich can track.", base) + } + if _, err := runGit(projectPath, "fetch", "--", remote, branch); err != nil { + return nil, err + } + args = append(args, "--track") } - args = append(args, "--track") + args = append(args, "-b", name) } - // "--": base is unvalidated, and "-"-prefixed it would parse as a flag. - args = append(args, "-b", name, "--", wtPath, base) + // "--": source is unvalidated, and "-"-prefixed it would parse as a flag. + args = append(args, "--", wtPath, source) if _, err := runGit(projectPath, args...); err != nil { return nil, err } diff --git a/internal/project/worktree_test.go b/internal/project/worktree_test.go index 3cfeba4b8..8a35d6e34 100644 --- a/internal/project/worktree_test.go +++ b/internal/project/worktree_test.go @@ -161,12 +161,19 @@ func TestCreateWorktree(t *testing.T) { } } -// TestCreateWorktreeErrors proves invalid names, duplicate branches and -// pre-existing paths all fail with a message instead of half-creating state. +// TestCreateWorktreeErrors proves invalid names, branches held by another +// checkout and pre-existing paths all fail with a message instead of +// half-creating state. +// +// A branch that merely exists is no longer among them: it used to be, back when +// every creation went through `worktree add -b`. That expectation moved to +// TestCreateWorktreeExistingBranch — the contract changed, the test followed it. func TestCreateWorktreeErrors(t *testing.T) { t.Setenv("XDG_DATA_HOME", t.TempDir()) repo, git := initRepo(t) - git("branch", "taken") + // Held by a checkout of its own, which is the state git still refuses: one + // branch cannot be checked out twice. + git("worktree", "add", "-b", "taken", filepath.Join(t.TempDir(), "taken")) svc := New(nil) // The messages are what the dialog shows, so they name the branch rather @@ -178,7 +185,7 @@ func TestCreateWorktreeErrors(t *testing.T) { }{ {"invalid name", "has space", `"has space" is not a name git accepts`}, {"dotdot name", "a..b", `"a..b" is not a name git accepts`}, - {"duplicate branch", "taken", "already exists"}, + {"branch checked out elsewhere", "taken", "already checked out"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -201,6 +208,63 @@ func TestCreateWorktreeErrors(t *testing.T) { } } +// TestCreateWorktreeExistingBranch replicates issue #334: a branch cut before +// the session — off an updated remote, linked to a GitHub issue — is checked +// out as it stands rather than recreated, and the base is ignored because the +// branch already started somewhere. +// +// lich handed git `worktree add -b` for every creation, and git refuses a name +// it already knows, so the only way to open a session on a prepared branch was +// to delete it first — exactly what the reporter could not do, with the issue +// branch already pushed. +func TestCreateWorktreeExistingBranch(t *testing.T) { + t.Setenv("XDG_DATA_HOME", t.TempDir()) + repo, git := initRepo(t) + addLocalOrigin(t, git) + + // The branch carries a commit no base has, so "reused" is provable rather + // than a name that happens to match: rewinding main leaves the commit + // reachable only from the branch. + const branch = "issue-4-ownership" + git("commit", "--allow-empty", "-m", "prepared work") + git("branch", branch) + prepared := strings.TrimSpace(git("rev-parse", branch)) + git("reset", "--hard", "HEAD~1") + + // The reporter's call, verbatim: the prepared branch as the name, the remote + // it was cut from as the base. + svc := New(nil) + wt, err := svc.CreateWorktree(repo, "pid", branch, "origin/main", true) + if err != nil { + t.Fatalf("CreateWorktree(existing branch): %v", err) + } + if wt.Name != branch { + t.Errorf("Name = %q, want %q", wt.Name, branch) + } + + head, err := runGit(wt.Path, "rev-parse", "HEAD") + if err != nil { + t.Fatalf("rev-parse HEAD: %v", err) + } + if got := strings.TrimSpace(head); got != prepared { + t.Errorf("worktree HEAD = %s, want the prepared branch tip %s — the base was not ignored", got, prepared) + } + on, err := runGit(wt.Path, "rev-parse", "--abbrev-ref", "HEAD") + if err != nil { + t.Fatalf("rev-parse --abbrev-ref HEAD: %v", err) + } + if got := strings.TrimSpace(on); got != branch { + t.Errorf("worktree is on %q, want %q", got, branch) + } + + // git has to own it too, or nothing can remove it later. + if !slices.ContainsFunc(parseCheckouts(git("worktree", "list", "--porcelain")), func(c Worktree) bool { + return c.Name == branch + }) { + t.Errorf("the branch is not registered as a worktree: %s", git("worktree", "list")) + } +} + // TestCreateWorktreeRemoteBase proves a remote base is fetched and the new // branch tracks it. func TestCreateWorktreeRemoteBase(t *testing.T) {