diff --git a/src/features/vcs/__tests__/branchOps.test.ts b/src/features/vcs/__tests__/branchOps.test.ts index 7b3e2db02..6acb3865e 100644 --- a/src/features/vcs/__tests__/branchOps.test.ts +++ b/src/features/vcs/__tests__/branchOps.test.ts @@ -1,12 +1,38 @@ import { beforeEach, expect, it, vi } from "vitest"; -import type { Branch, BranchInfo } from "@/bindings"; -const mocks = vi.hoisted(() => ({ prompt: vi.fn() })); +import type { Branch, BranchInfo, ProjectInfo, Worktree } from "@/bindings"; +const mocks = vi.hoisted(() => ({ + prompt: vi.fn(), + confirm: vi.fn(), + notify: vi.fn(), + removeWorktree: vi.fn(), + deleteBranch: vi.fn(), + worktrees: vi.fn(), + restoreBranch: vi.fn(), + branches: vi.fn(), +})); vi.mock("../dialogs", async (original) => ({ ...(await original()), promptText: mocks.prompt, + confirmDialog: mocks.confirm, })); -import { newBranch, newBranchInRepos, renameBranch } from "../branchOps"; +vi.mock("@/workbench/contributions", () => ({ notifications: () => ({ notify: mocks.notify, expire: vi.fn() }) })); +vi.mock("@/platform/ipc", async (original) => { + const real = await original(); + return { + ...real, + commands: { + ...real.commands, + vcsWorktrees: mocks.worktrees, + vcsRemoveWorktree: mocks.removeWorktree, + vcsDeleteBranch: mocks.deleteBranch, + vcsRestoreBranch: mocks.restoreBranch, + vcsBranches: mocks.branches, + }, + }; +}); +import { deleteBranch, deleteBranches, newBranch, newBranchInRepos, renameBranch } from "../branchOps"; import { useVcs } from "../store"; +import { useProject } from "@/platform/project"; const branch = (head: string | null): BranchInfo => ({ head, @@ -28,13 +54,16 @@ const local = (name: string, current = false): Branch => ({ }); beforeEach(() => { - vi.clearAllMocks(); + vi.resetAllMocks(); + useProject.setState({ project: { root: "/p", name: "p" } as ProjectInfo }); // The current branch is always among the local branches, so the suggestion names an existing one. useVcs.setState({ root: "/p", repo: "/p/front", + roots: ["/p/front"], branch: branch("feature/ui"), branches: [local("feature/ui", true), local("main")], + available: false, }); // Cancel: only what the dialog was offered matters here, not its outcome. mocks.prompt.mockResolvedValue(null); @@ -74,3 +103,173 @@ it.each([ expect(typed).toBe("fix-login"); expect(validate(typed)).toBeNull(); }); + +const worktree = (path: string, branchName: string | null, main = false): Worktree => ({ + path, + head: "abc1234567890", + branch: branchName, + main, + locked: false, + prunable: false, +}); + +const deleted = { name: "feature/wt", tip: "abc1234567890", upstream: null, unmerged: false }; +const ok = (data: unknown) => ({ status: "ok", data }); + +beforeEach(() => { + mocks.deleteBranch.mockResolvedValue(ok(deleted)); + mocks.removeWorktree.mockResolvedValue(ok(null)); + mocks.restoreBranch.mockResolvedValue(ok(null)); + mocks.branches.mockResolvedValue(ok([])); +}); + +it("deletes a worktree holding the branch before deleting it", async () => { + mocks.worktrees.mockResolvedValue( + ok([worktree("/p/front", "main", true), worktree("/p/front-feature-wt", "feature/wt")]), + ); + mocks.confirm.mockResolvedValue(true); + await deleteBranch("feature/wt"); + expect(mocks.confirm.mock.calls[0]?.[0].message).toContain("/p/front-feature-wt"); + expect(mocks.removeWorktree).toHaveBeenCalledWith("/p/front", "/p/front-feature-wt", false); + expect(mocks.deleteBranch).toHaveBeenCalledWith("/p/front", "feature/wt"); +}); + +it("keeps the branch when the worktree question is turned down", async () => { + mocks.worktrees.mockResolvedValue(ok([worktree("/p/front-feature-wt", "feature/wt")])); + mocks.confirm.mockResolvedValue(false); + await deleteBranch("feature/wt"); + expect(mocks.removeWorktree).not.toHaveBeenCalled(); + expect(mocks.deleteBranch).not.toHaveBeenCalled(); +}); + +it("offers to delete a holding worktree even when the window has it open as a root", async () => { + // A project opened on the parent folder has both worktrees as its roots. + useVcs.setState({ roots: ["/p/front", "/p/front-feature-wt"] }); + mocks.worktrees.mockResolvedValue( + ok([worktree("/p/front", "main", true), worktree("/p/front-feature-wt", "feature/wt")]), + ); + mocks.confirm.mockResolvedValue(true); + await deleteBranch("feature/wt"); + expect(mocks.removeWorktree).toHaveBeenCalledWith("/p/front", "/p/front-feature-wt", false); + expect(mocks.deleteBranch).toHaveBeenCalledWith("/p/front", "feature/wt"); +}); + +it("keeps the branch checked out in the main worktree: git's own error stands", async () => { + mocks.worktrees.mockResolvedValue(ok([worktree("/p/front", "feature/wt", true)])); + await deleteBranch("feature/wt"); + expect(mocks.confirm).not.toHaveBeenCalled(); + expect(mocks.removeWorktree).not.toHaveBeenCalled(); + expect(mocks.deleteBranch).toHaveBeenCalledWith("/p/front", "feature/wt"); +}); + +it.each([ + ["/p/front", "/p/front"], + ["/p/front/", "/p/front/"], + ["/p/front", "/p/front/nested"], + ["/p/front/", "/p/front/nested/"], +])("never offers to remove holding worktree %s containing project %s", async (path, root) => { + useProject.setState({ project: { root, name: "project" } as ProjectInfo }); + mocks.worktrees.mockResolvedValue(ok([worktree(path, "feature/wt")])); + mocks.confirm.mockResolvedValue(true); + mocks.removeWorktree.mockRejectedValue(new Error("would reach Force Remove")); + await deleteBranch("feature/wt"); + expect(mocks.confirm).not.toHaveBeenCalled(); + expect(mocks.removeWorktree).not.toHaveBeenCalled(); + expect(mocks.deleteBranch).toHaveBeenCalledWith("/p/front", "feature/wt"); +}); + +it("offers a prefix-only sibling of the project folder", async () => { + useProject.setState({ project: { root: "/p/front-wt", name: "project" } as ProjectInfo }); + mocks.worktrees.mockResolvedValue(ok([worktree("/p/front", "feature/wt")])); + mocks.confirm.mockResolvedValue(true); + await deleteBranch("feature/wt"); + expect(mocks.removeWorktree).toHaveBeenCalledWith("/p/front", "/p/front", false); +}); + +it("ignores detached worktrees and matches the entire local branch name", async () => { + mocks.worktrees.mockResolvedValue( + ok([worktree("/p/detached", null), worktree("/p/wrong", "wt"), worktree("/p/right", "feature/wt")]), + ); + mocks.confirm.mockResolvedValue(true); + await deleteBranch("feature/wt"); + expect(mocks.removeWorktree).toHaveBeenCalledExactlyOnceWith("/p/front", "/p/right", false); +}); + +it("cancelling Force Remove keeps both the worktree and branch", async () => { + mocks.worktrees.mockResolvedValue(ok([worktree("/p/wt", "feature/wt")])); + mocks.removeWorktree.mockRejectedValue(new Error("fatal: '/p/wt' contains modified or untracked files")); + mocks.confirm.mockResolvedValueOnce(true).mockResolvedValueOnce(false); + await deleteBranch("feature/wt"); + expect(mocks.confirm.mock.calls[1]?.[0]).toMatchObject({ + message: "fatal: '/p/wt' contains modified or untracked files\n\nRemove it anyway, discarding its changes?", + okLabel: "Force Remove", + danger: true, + }); + expect(mocks.removeWorktree).toHaveBeenCalledExactlyOnceWith("/p/front", "/p/wt", false); + expect(mocks.deleteBranch).not.toHaveBeenCalled(); +}); + +it("force removes a dirty worktree before deleting its branch", async () => { + mocks.worktrees.mockResolvedValue(ok([worktree("/p/wt", "feature/wt")])); + mocks.removeWorktree.mockRejectedValueOnce(new Error("dirty")); + mocks.confirm.mockResolvedValue(true); + await deleteBranch("feature/wt"); + expect(mocks.removeWorktree.mock.calls).toEqual([ + ["/p/front", "/p/wt", false], + ["/p/front", "/p/wt", true], + ]); + expect(mocks.deleteBranch).toHaveBeenCalledWith("/p/front", "feature/wt"); +}); + +it("reports a locked worktree's force-removal error and keeps the branch", async () => { + const error = "fatal: cannot remove a locked working tree; use 'remove -f -f' to override"; + mocks.worktrees.mockResolvedValue(ok([{ ...worktree("/p/wt", "feature/wt"), locked: true }])); + mocks.removeWorktree.mockRejectedValue(new Error(error)); + mocks.confirm.mockResolvedValue(true); + await deleteBranch("feature/wt"); + expect(mocks.deleteBranch).not.toHaveBeenCalled(); + expect(mocks.notify).toHaveBeenCalledWith(expect.objectContaining({ level: "error", body: error })); +}); + +it.each([false, true])("continues a multi-repository selection after cancelling (force: %s)", async (force) => { + mocks.worktrees.mockImplementation(async (repo) => ok([worktree(`${repo}/wt`, "feature/wt")])); + if (force) { + mocks.removeWorktree.mockRejectedValueOnce(new Error("dirty")); + mocks.confirm.mockResolvedValueOnce(true).mockResolvedValueOnce(false).mockResolvedValueOnce(true); + } else mocks.confirm.mockResolvedValueOnce(false).mockResolvedValueOnce(true); + await deleteBranches([ + { repo: "/p/front", branch: local("feature/wt") }, + { repo: "/p/back", branch: local("feature/wt") }, + ]); + expect(mocks.worktrees.mock.calls).toEqual([["/p/front"], ["/p/back"]]); + expect(mocks.removeWorktree).toHaveBeenLastCalledWith("/p/back", "/p/back/wt", false); + expect(mocks.deleteBranch).toHaveBeenCalledExactlyOnceWith("/p/back", "feature/wt"); + expect(useVcs.getState().repo).toBe("/p/front"); +}); + +it("restores only the branch after removing its worktree", async () => { + mocks.worktrees.mockResolvedValue(ok([worktree("/p/wt", "feature/wt")])); + mocks.confirm.mockResolvedValue(true); + await deleteBranch("feature/wt"); + const notification = mocks.notify.mock.calls.find(([n]) => n.title === "Deleted branch feature/wt")![0]; + notification.actions.find((a: { label: string }) => a.label === "Restore").run(); + await vi.waitFor(() => expect(mocks.restoreBranch).toHaveBeenCalledWith("/p/front", deleted)); + expect(notification.title).not.toContain("worktree"); +}); + +it("refreshes tracking branches in the deletion repository without replacing the focused branches", async () => { + const focused = [local("main", true)]; + useVcs.setState({ branches: focused }); + mocks.worktrees.mockResolvedValue(ok([])); + mocks.deleteBranch.mockResolvedValue(ok({ ...deleted, upstream: "origin/feature/wt" })); + mocks.branches.mockResolvedValue(ok([{ ...local("other"), upstream: "origin/feature/wt" }])); + await deleteBranch("feature/wt", "/p/back"); + expect(mocks.worktrees).toHaveBeenCalledWith("/p/back"); + expect(mocks.deleteBranch).toHaveBeenCalledWith("/p/back", "feature/wt"); + expect(mocks.branches).toHaveBeenCalledWith("/p/back"); + expect(useVcs.getState().branches).toEqual(focused); + const notification = mocks.notify.mock.calls.find(([n]) => n.title === "Deleted branch feature/wt")![0]; + expect(notification.actions.map((a: { label: string }) => a.label)).not.toContain("Delete Tracked Branch"); + notification.actions.find((a: { label: string }) => a.label === "Restore").run(); + await vi.waitFor(() => expect(mocks.restoreBranch).toHaveBeenCalledWith("/p/back", expect.anything())); +}); diff --git a/src/features/vcs/__tests__/deleteBranches.test.ts b/src/features/vcs/__tests__/deleteBranches.test.ts index 4a410db9b..4d46cdc8d 100644 --- a/src/features/vcs/__tests__/deleteBranches.test.ts +++ b/src/features/vcs/__tests__/deleteBranches.test.ts @@ -8,7 +8,16 @@ const mocks = vi.hoisted(() => ({ focus: vi.fn(), notify: vi.fn(), range: vi.fn(), + deleteBranch: vi.fn(), + branches: vi.fn(), })); +vi.mock("@/platform/ipc", async (original) => { + const real = await original(); + return { + ...real, + commands: { ...real.commands, vcsDeleteBranch: mocks.deleteBranch, vcsBranches: mocks.branches }, + }; +}); vi.mock("../dialogs", async (original) => ({ ...(await original()), confirmDialog: mocks.confirm, @@ -60,24 +69,41 @@ beforeEach(() => { useVcs.setState({ root: "/p", repo: "/p/a", branches: [], branchesLoaded: true }); useBranchesPane.setState({ selected: [] }); mocks.confirm.mockResolvedValue(true); + mocks.branches.mockResolvedValue({ status: "ok", data: [] }); mocks.focus.mockImplementation(async (repo: string) => { useVcs.setState({ repo }); }); }); -it("restores the original repository after deleting across repositories", async () => { - const repos: (string | null)[] = []; - mocks.local.mockImplementation(async () => { - repos.push(useVcs.getState().repo); - }); +it("deletes across repositories without changing focus", async () => { await deleteBranches([target("one", "/p/b"), target("two", "/p/a"), target("three", "/p/b")]); - expect(repos).toEqual(["/p/b", "/p/a", "/p/b"]); + expect(mocks.focus).not.toHaveBeenCalled(); expect(useVcs.getState().repo).toBe("/p/a"); }); -it("restores focus when deletion throws", async () => { +it("keeps deletion targets when an in-flight refresh overwrites focus between deletions", async () => { + mocks.local.mockImplementation(async (_title, run) => { + const result = await run(); + useVcs.setState({ repo: "/p/b" }); + return result; + }); + mocks.focus.mockImplementation(async () => { + // A refresh started in B finishes after the attempt to focus A. + useVcs.setState({ repo: "/p/b" }); + }); + mocks.deleteBranch.mockResolvedValue({ status: "ok", data: null }); + await deleteBranches([target("one", "/p/b"), target("two", "/p/a")]); + expect(mocks.deleteBranch.mock.calls).toEqual([ + ["/p/b", "one"], + ["/p/a", "two"], + ]); + expect(mocks.focus).not.toHaveBeenCalled(); +}); + +it("leaves focus untouched when deletion throws", async () => { mocks.local.mockRejectedValue(new Error("failed")); await expect(deleteBranches([target("one", "/p/b")])).rejects.toThrow("failed"); + expect(mocks.focus).not.toHaveBeenCalled(); expect(useVcs.getState().repo).toBe("/p/a"); }); @@ -119,18 +145,15 @@ it.each([1, 2])("cancelling %s remote branches leaves the whole mixed selection }); it("confirms once for remote branches across repositories and deletes in the right repository", async () => { - const repos: (string | null)[] = []; - mocks.remote.mockImplementation(async () => { - repos.push(useVcs.getState().repo); - return true; - }); + mocks.remote.mockResolvedValue(true); await deleteBranches([ target("origin/one", "/p/a", { remote: true }), target("origin/two", "/p/b", { remote: true }), ]); expect(mocks.confirm).toHaveBeenCalledTimes(1); expect(mocks.remote.mock.calls.map(([op]) => op.branch)).toEqual(["origin/one", "origin/two"]); - expect(repos).toEqual(["/p/a", "/p/b"]); + expect(mocks.remote.mock.calls.map(([, , repos]) => repos)).toEqual([["/p/a"], ["/p/b"]]); + expect(mocks.focus).not.toHaveBeenCalled(); expect(useVcs.getState().repo).toBe("/p/a"); }); @@ -145,7 +168,7 @@ it("the Delete action uses the whole pane selection without an explicit argument } }); -it("Delete Tracked Branch keeps the repository captured before focus was restored", async () => { +it("Delete Tracked Branch keeps the repository captured without changing focus", async () => { mocks.local.mockResolvedValue({ name: "one", tip: "abc", upstream: "origin/one", unmerged: false }); await deleteBranches([target("one", "/p/b")]); expect(useVcs.getState().repo).toBe("/p/a"); @@ -156,7 +179,7 @@ it("Delete Tracked Branch keeps the repository captured before focus was restore ); }); -it("View Commits keeps the deleted branch's repository after focus was restored", async () => { +it("View Commits keeps the deleted branch's repository without changing focus", async () => { mocks.local.mockResolvedValue({ name: "one", tip: "abc", upstream: null, unmerged: true }); await deleteBranches([target("one", "/p/b")]); const notification = mocks.notify.mock.calls[0]![0]; @@ -164,8 +187,9 @@ it("View Commits keeps the deleted branch's repository after focus was restored" expect(mocks.range).toHaveBeenCalledWith("Commits of one", "HEAD", "abc", "/p/b"); }); -it("restores the primary repository when it was focused implicitly", async () => { +it("keeps the primary repository focused implicitly", async () => { useVcs.setState({ repo: null, roots: ["/p/a", "/p/b"] }); await deleteBranches([target("one", "/p/b")]); - expect(useVcs.getState().repo).toBe("/p/a"); + expect(useVcs.getState().repo).toBeNull(); + expect(mocks.focus).not.toHaveBeenCalled(); }); diff --git a/src/features/vcs/branchOps.ts b/src/features/vcs/branchOps.ts index aa489ced9..27b142e5e 100644 --- a/src/features/vcs/branchOps.ts +++ b/src/features/vcs/branchOps.ts @@ -1,6 +1,6 @@ /** Branch operations (checkout, create, merge, rebase, update, delete) with recents and favorites. */ import { create } from "zustand"; -import type { Branch, BranchInfo, CheckoutOptions } from "@/bindings"; +import type { Branch, BranchInfo, CheckoutOptions, Worktree } from "@/bindings"; import { executeAction } from "@/platform/actions"; import { call, commands } from "@/platform/ipc"; import { notifications } from "@/workbench/contributions"; @@ -9,9 +9,10 @@ import { isFavoriteBranch, pushRecent, shortRemoteName } from "./branches"; import { showCommitRange } from "./CompareBranches"; import { confirmDialog, promptText } from "./dialogs"; import { cleanUpBranchNameOnTyping } from "@/platform/gitRefName"; -import { runOp, runRemote, saveAll } from "./ops"; -import { conflictedFiles, currentRepo, currentRepoRoot, focusRepo, refreshNow, repoName, useVcs } from "./store"; +import { errorMessage, runOp, runRemote, saveAll } from "./ops"; +import { conflictedFiles, currentRepo, refreshNow, repoName, useVcs } from "./store"; import { tx } from "@/platform/i18n"; +import { useProject } from "@/platform/project"; const recentKey = () => `openidea.vcs.recentBranches:${useVcs.getState().root ?? ""}`; @@ -279,17 +280,63 @@ export async function renameBranch(name: string) { } } +/** + * A branch checked out in another worktree cannot be deleted: `git branch -d` refuses. + * Offers to delete such a worktree first (its folder goes with it); false when the user + * backed out. The main worktree cannot be removed, so it keeps git's own message on the + * branch deletion. A worktree containing this window's project cannot be removed either; + * worktrees below a project opened on a parent folder can still be offered. + */ +async function deleteHoldingWorktree(repo: string | null, name: string): Promise { + let holding: Worktree | undefined; + try { + holding = (await call(commands.vcsWorktrees(repo))).find((w) => w.branch === name && !w.main); + } catch { + return true; // no worktree list: let the branch deletion speak for itself + } + if (!holding) return true; + const root = useProject.getState().project?.root.replace(/\/+$/, ""); + const path = holding.path.replace(/\/+$/, ""); + if (root !== undefined && (root === path || root.startsWith(path + "/"))) return true; + const ok = await confirmDialog({ + title: "Delete Branch", + message: tx`Branch '${name}' is checked out in the worktree at ${holding.path}.\nDelete the worktree and the branch? Its folder is deleted.`, + okLabel: "Delete", + danger: true, + }); + if (!ok) return false; + try { + await call(commands.vcsRemoveWorktree(repo, holding.path, false)); + } catch (e) { + const force = await confirmDialog({ + title: "Delete Branch", + message: tx`${errorMessage(e)}\n\nRemove it anyway, discarding its changes?`, + okLabel: "Force Remove", + danger: true, + }); + if (!force) return false; + const removed = await runOp(tx`Delete branch ${name}`, async () => { + await call(commands.vcsRemoveWorktree(repo, holding.path, true)); + return true; + }); + if (!removed) return false; + } + return true; +} + /** * Delete Branch as IDEA does it: no confirmation; a branch that is not fully merged is * deleted anyway and the notification says so, offering View Commits. Restore recreates * the branch (with its upstream); Delete Tracked Branch removes the remote branch too. */ -export async function deleteBranch(name: string) { - const repo = currentRepo(); +export async function deleteBranch(name: string, repo = currentRepo()) { + if (!(await deleteHoldingWorktree(repo, name))) return; const deleted = await runOp(tx`Delete branch ${name}`, () => call(commands.vcsDeleteBranch(repo, name))); if (!deleted) return; const tracked = deleted.upstream; - const otherTracking = useVcs.getState().branches.some((b) => !b.remote && b.name !== name && b.upstream === tracked); + const branches = await call(commands.vcsBranches(repo)).catch(() => []); + if (repo === currentRepo()) useVcs.setState({ branches, branchesLoaded: true }); + const otherTracking = branches.some((b) => !b.remote && b.name !== name && b.upstream === tracked); const key = `vcs.deleted.${name}`; const done = () => notifications().expire?.(key); notifications().notify({ @@ -421,22 +468,17 @@ async function confirmRemoteBranches(remote: readonly { branch: Branch }[]): Pro }); } -/** Deletes selected branches, skipping current branches and restoring the focused repository. */ +/** Deletes selected branches in their repositories without changing focus, skipping current branches. */ export async function deleteBranches(targets: readonly { ref?: string; repo?: string; branch?: Branch }[]) { const list = targets.filter((t): t is { repo?: string; branch: Branch } => canDeleteBranch(t)); if (!list.length) return; - const previous = currentRepoRoot(); + const fallback = currentRepo(); const remote = list.filter((t) => t.branch.remote); if (remote.length && !(await confirmRemoteBranches(remote))) return; - try { - for (const t of list) { - const repo = t.repo ?? previous; - if (repo && repo !== currentRepo()) await focusRepo(repo); - if (t.branch.remote) await runRemote({ op: "deleteBranch", branch: t.branch.name }, t.branch.name); - else await deleteBranch(t.branch.name); - } - } finally { - if (previous && previous !== currentRepo()) await focusRepo(previous); + for (const t of list) { + const repo = t.repo ?? fallback; + if (t.branch.remote) await runRemote({ op: "deleteBranch", branch: t.branch.name }, t.branch.name, [repo]); + else await deleteBranch(t.branch.name, repo); } } diff --git a/src/platform/locales/text/en.json b/src/platform/locales/text/en.json index 557dfc470..ed950bac4 100644 --- a/src/platform/locales/text/en.json +++ b/src/platform/locales/text/en.json @@ -429,5 +429,6 @@ "Allowed for this session": "Allowed for this session", "Always allowed": "Always allowed", "Declined": "Declined", - "Always declined": "Always declined" + "Always declined": "Always declined", + "Branch '{name}' is checked out in the worktree at {path}.\nDelete the worktree and the branch? Its folder is deleted.": "Branch '{name}' is checked out in the worktree at {path}.\nDelete the worktree and the branch? Its folder is deleted." } diff --git a/src/platform/locales/text/zh.json b/src/platform/locales/text/zh.json index 38a823ac7..1d47b88d6 100644 --- a/src/platform/locales/text/zh.json +++ b/src/platform/locales/text/zh.json @@ -6782,5 +6782,6 @@ "Allowed for this session": "已在此会话中允许", "Always allowed": "已设为始终允许", "Declined": "已拒绝", - "Always declined": "已设为始终拒绝" + "Always declined": "已设为始终拒绝", + "Branch '{name}' is checked out in the worktree at {path}.\nDelete the worktree and the branch? Its folder is deleted.": "分支 '{name}' 被位于 {path} 的工作树签出。\n删除该工作树和分支？其文件夹将被删除。" }