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) {