diff --git a/src/application/use-cases/delete-branch.test.ts b/src/application/use-cases/delete-branch.test.ts new file mode 100644 index 0000000..97bffc1 --- /dev/null +++ b/src/application/use-cases/delete-branch.test.ts @@ -0,0 +1,147 @@ +import { describe, expect, test } from "bun:test"; +import { createFakeGit } from "../../test-utils/fake-git.ts"; +import { deleteBranch } from "./delete-branch.ts"; + +describe("deleteBranch", () => { + test("clean delete → deleted, remote skipped", async () => { + const git = createFakeGit({ + branches: ["main", "feature"], + mergedBranches: ["feature"], + }); + const outcome = await deleteBranch({ branch: "feature", force: false, deleteRemote: false }, { git }); + expect(outcome).toEqual({ status: "deleted", remote: { status: "skipped" } }); + }); + + test("not-merged + force=false → not-merged (no force fallback)", async () => { + const git = createFakeGit({ + branches: ["main", "feature"], + // not in mergedBranches → deleteBranch returns BRANCH_NOT_MERGED + }); + const outcome = await deleteBranch({ branch: "feature", force: false, deleteRemote: false }, { git }); + expect(outcome).toEqual({ status: "not-merged" }); + + // force-delete must NOT have been invoked — branch still listable + const branches = await git.listBranches(); + expect(branches.success && branches.data.includes("feature")).toBe(true); + }); + + test("not-merged + force=true → deleted (force fallback)", async () => { + const git = createFakeGit({ + branches: ["main", "feature"], + }); + const outcome = await deleteBranch({ branch: "feature", force: true, deleteRemote: false }, { git }); + expect(outcome).toEqual({ status: "deleted", remote: { status: "skipped" } }); + + const branches = await git.listBranches(); + expect(branches.success && branches.data.includes("feature")).toBe(false); + }); + + test("delete fails with non-NOT_MERGED error → failed with message", async () => { + const git = createFakeGit({ + branches: ["main"], // "feature" missing → BRANCH_NOT_FOUND + }); + const outcome = await deleteBranch({ branch: "feature", force: false, deleteRemote: false }, { git }); + expect(outcome.status).toBe("failed"); + if (outcome.status === "failed") { + expect(outcome.message).toContain("feature"); + } + }); + + test("delete fails with non-NOT_MERGED error → failed even when force=true", async () => { + const git = createFakeGit({ + branches: ["main"], // BRANCH_NOT_FOUND is NOT recoverable via force + }); + const outcome = await deleteBranch({ branch: "ghost", force: true, deleteRemote: false }, { git }); + // deleteBranch returns BRANCH_NOT_FOUND, which is not BRANCH_NOT_MERGED, + // so force fallback must NOT be attempted. + expect(outcome.status).toBe("failed"); + }); + + test("force fallback fails → failed with force error message", async () => { + // "feature" is not merged, so the normal delete returns BRANCH_NOT_MERGED and + // triggers the force fallback. Stub deleteBranchForce to fail so we exercise the + // "force fallback itself failed → failed" path. + const baseGit = createFakeGit({ branches: ["main", "feature"] }); + const git = { + ...baseGit, + async deleteBranchForce(_branch: string) { + return { success: false as const, error: { code: "UNKNOWN" as const, message: "force boom" } }; + }, + }; + const outcome = await deleteBranch({ branch: "feature", force: true, deleteRemote: false }, { git }); + expect(outcome).toEqual({ status: "failed", message: "force boom" }); + }); + + test("deleted + remote delete requested + remote ref exists → remote=deleted", async () => { + const git = createFakeGit({ + branches: ["main", "feature"], + mergedBranches: ["feature"], + remoteBranches: ["feature"], + }); + const outcome = await deleteBranch({ branch: "feature", force: false, deleteRemote: true }, { git }); + expect(outcome).toEqual({ status: "deleted", remote: { status: "deleted" } }); + }); + + test("deleted + remote delete requested + remote ref missing → remote=not-found", async () => { + const git = createFakeGit({ + branches: ["main", "feature"], + mergedBranches: ["feature"], + remoteBranches: [], // empty, but state-tracking is enabled because the option is provided + }); + const outcome = await deleteBranch({ branch: "feature", force: false, deleteRemote: true }, { git }); + expect(outcome).toEqual({ status: "deleted", remote: { status: "not-found" } }); + }); + + test("deleted + remote delete fails with other error → remote=failed", async () => { + const git = createFakeGit({ + branches: ["main", "feature"], + mergedBranches: ["feature"], + deleteRemoteBranchFail: { code: "UNKNOWN", message: "network down" }, + }); + const outcome = await deleteBranch({ branch: "feature", force: false, deleteRemote: true }, { git }); + expect(outcome.status).toBe("deleted"); + if (outcome.status === "deleted") { + expect(outcome.remote).toEqual({ status: "failed", message: "network down" }); + } + }); + + test("force fallback + remote delete → deleted + remote=deleted", async () => { + const git = createFakeGit({ + branches: ["main", "feature"], + // not merged → force fallback + remoteBranches: ["feature"], + }); + const outcome = await deleteBranch({ branch: "feature", force: true, deleteRemote: true }, { git }); + expect(outcome).toEqual({ status: "deleted", remote: { status: "deleted" } }); + }); + + test("not-merged outcome must not attempt remote deletion", async () => { + let remoteCalls = 0; + const baseGit = createFakeGit({ branches: ["main", "feature"] }); + const git = { + ...baseGit, + async deleteRemoteBranch(branch: string, remote?: string) { + remoteCalls += 1; + return baseGit.deleteRemoteBranch(branch, remote); + }, + }; + const outcome = await deleteBranch({ branch: "feature", force: false, deleteRemote: true }, { git }); + expect(outcome).toEqual({ status: "not-merged" }); + expect(remoteCalls).toBe(0); + }); + + test("failed outcome must not attempt remote deletion", async () => { + let remoteCalls = 0; + const baseGit = createFakeGit({ branches: ["main"] }); + const git = { + ...baseGit, + async deleteRemoteBranch(branch: string, remote?: string) { + remoteCalls += 1; + return baseGit.deleteRemoteBranch(branch, remote); + }, + }; + const outcome = await deleteBranch({ branch: "ghost", force: false, deleteRemote: true }, { git }); + expect(outcome.status).toBe("failed"); + expect(remoteCalls).toBe(0); + }); +}); diff --git a/src/application/use-cases/delete-branch.ts b/src/application/use-cases/delete-branch.ts new file mode 100644 index 0000000..23cd9aa --- /dev/null +++ b/src/application/use-cases/delete-branch.ts @@ -0,0 +1,78 @@ +import type { GitPort } from "../../domain/ports/git-port.ts"; +import { Result } from "../../shared/result.ts"; + +/** + * Result of attempting to delete the remote-tracking branch, when requested. + * + * - `deleted` — remote ref was removed successfully + * - `not-found` — remote returned REMOTE_REF_NOT_FOUND (treated as success: nothing to delete) + * - `failed` — any other remote failure (rendered as a warning in the CLI) + * - `skipped` — caller did not request remote deletion + */ +export type RemoteDeletionOutcome = + | { status: "deleted" } + | { status: "not-found" } + | { status: "failed"; message: string } + | { status: "skipped" }; + +/** + * Typed outcome of the delete-branch policy. + * + * - `deleted` — local branch was removed (either by the normal delete or by + * the force fallback). `remote` carries the remote-deletion + * sub-outcome. + * - `not-merged` — `deleteBranch` reported BRANCH_NOT_MERGED and the caller did + * not request force; no remote deletion was attempted. + * - `failed` — local deletion failed for any other reason, including a force + * fallback that itself failed. `message` is the git error message. + */ +export type DeleteBranchOutcome = + | { status: "deleted"; remote: RemoteDeletionOutcome } + | { status: "not-merged" } + | { status: "failed"; message: string }; + +export interface DeleteBranchInput { + branch: string; + /** When true, fall back to deleteBranchForce on BRANCH_NOT_MERGED. */ + force: boolean; + /** When true, attempt to delete the remote-tracking branch after the local delete. */ + deleteRemote: boolean; +} + +export interface DeleteBranchDeps { + git: GitPort; +} + +export async function deleteBranch(input: DeleteBranchInput, deps: DeleteBranchDeps): Promise { + const { git } = deps; + const { branch, force, deleteRemote } = input; + + const deleteResult = await git.deleteBranch(branch); + + if (Result.isErr(deleteResult)) { + if (deleteResult.error.code !== "BRANCH_NOT_MERGED") { + return { status: "failed", message: deleteResult.error.message }; + } + if (!force) { + return { status: "not-merged" }; + } + const forceResult = await git.deleteBranchForce(branch); + if (Result.isErr(forceResult)) { + return { status: "failed", message: forceResult.error.message }; + } + } + + const remote = deleteRemote ? await deleteRemoteBranch(branch, git) : ({ status: "skipped" } as const); + return { status: "deleted", remote }; +} + +async function deleteRemoteBranch(branch: string, git: GitPort): Promise { + const remoteResult = await git.deleteRemoteBranch(branch); + if (Result.isOk(remoteResult)) { + return { status: "deleted" }; + } + if (remoteResult.error.code === "REMOTE_REF_NOT_FOUND") { + return { status: "not-found" }; + } + return { status: "failed", message: remoteResult.error.message }; +} diff --git a/src/cli/commands/remove.test.ts b/src/cli/commands/remove.test.ts index 7cfa7dc..91da1ab 100644 --- a/src/cli/commands/remove.test.ts +++ b/src/cli/commands/remove.test.ts @@ -27,13 +27,21 @@ interface FakeMultiSpinnerLog { fail: MultiSpinnerCall[]; } +interface FakeSpinnerLog { + start: string[]; + message: string[]; + stop: string[]; +} + function createFakeUi(opts: { nonInteractive?: boolean; confirm?: boolean; multiselectResult?: string[] } = {}): { ui: UiPort; log: FakeUiLog; multiSpinnerLog: FakeMultiSpinnerLog; + spinnerLog: FakeSpinnerLog; } { const log: FakeUiLog = { info: [], success: [], warn: [], error: [], outro: [] }; const multiSpinnerLog: FakeMultiSpinnerLog = { complete: [], fail: [] }; + const spinnerLog: FakeSpinnerLog = { start: [], message: [], stop: [] }; const ui = { nonInteractive: opts.nonInteractive ?? false, @@ -57,7 +65,17 @@ function createFakeUi(opts: { nonInteractive?: boolean; confirm?: boolean; multi return fn(); }, createSpinner() { - return { start() {}, message() {}, stop() {} }; + return { + start(message: string) { + spinnerLog.start.push(message); + }, + message(message: string) { + spinnerLog.message.push(message); + }, + stop(message?: string) { + if (message !== undefined) spinnerLog.stop.push(message); + }, + }; }, createMultiSpinner(_keys: string[]) { return { @@ -89,7 +107,7 @@ function createFakeUi(opts: { nonInteractive?: boolean; confirm?: boolean; multi cancel() {}, } satisfies UiPort; - return { ui, log, multiSpinnerLog }; + return { ui, log, multiSpinnerLog, spinnerLog }; } function buildContainer( @@ -251,3 +269,42 @@ describe("remove — multi path branch delete failures", () => { expect(code).toBe(0); }); }); + +describe("remove — single path force on unmerged branch", () => { + // Regression for the branch-deletion-policy extraction: with --force/--yes on an + // unmerged branch the CLI must still surface the "not merged" warning and the + // "Force deleting" step before deleting, not silently force on the first attempt. + test("--yes on unmerged branch → shows 'not merged' + 'Force deleting', then deletes", async () => { + const fs = createFakeFilesystem({ + files: { [`${ROOT}/${CONFIG_FILENAME}`]: JSON.stringify({ rootDir: ".worktrees" }) }, + directories: [ROOT, `${ROOT}/.worktrees`, featureWt.path], + }); + const git = createFakeGit({ + root: ROOT, + mainRoot: ROOT, + worktrees: [mainWt, featureWt], + branches: ["main", "feature"], + mergedBranches: [], // "feature" not merged → BRANCH_NOT_MERGED on the normal delete + }); + const { ui, spinnerLog } = createFakeUi(); + const container = buildContainer(ui, git, fs); + + const code = await runRemove(container, { + branch: "feature", + "delete-branch": true, + "delete-remote-branch": false, + yes: true, + force: false, + "dry-run": false, + }); + + expect(spinnerLog.stop.some((m) => m.includes("not merged"))).toBe(true); + expect(spinnerLog.start.some((m) => m.includes("Force deleting"))).toBe(true); + expect(spinnerLog.stop.some((m) => m.includes("deleted (local)"))).toBe(true); + + const branches = await git.listBranches(); + expect(branches.success && branches.data.includes("feature")).toBe(false); + + expect(code).toBe(0); + }); +}); diff --git a/src/cli/commands/remove.ts b/src/cli/commands/remove.ts index 52f17c3..86f265e 100644 --- a/src/cli/commands/remove.ts +++ b/src/cli/commands/remove.ts @@ -2,6 +2,7 @@ import { resolve } from "node:path"; import { defineCommand } from "citty"; import pc from "picocolors"; import * as v from "valibot"; +import { deleteBranch } from "../../application/use-cases/delete-branch.ts"; import { listWorktrees } from "../../application/use-cases/list-worktrees.ts"; import { loadConfig } from "../../application/use-cases/load-config.ts"; import { removeWorktree } from "../../application/use-cases/remove-worktree.ts"; @@ -181,53 +182,45 @@ export function removeCommand(container: Container) { if (shouldDeleteBranches && wt.branch) { spinner.start(`Deleting branch "${wt.branch}"...`); - let localDeleted = false; - const deleteResult = await git.deleteBranch(wt.branch); + let outcome = await deleteBranch( + { branch: wt.branch, force: false, deleteRemote: shouldDeleteRemoteBranches }, + { git }, + ); - if (Result.isErr(deleteResult)) { - if (deleteResult.error.code === "BRANCH_NOT_MERGED") { - spinner.stop(pc.yellow(`Branch "${wt.branch}" not merged`)); + if (outcome.status === "not-merged") { + spinner.stop(pc.yellow(`Branch "${wt.branch}" not merged`)); - let shouldForce = force || yes; - if (!shouldForce && !ui.nonInteractive) { - const forceConfirm = await ui.confirm({ - message: `Branch "${wt.branch}" is not merged. Force delete?`, - initialValue: false, - }); - shouldForce = !ui.isCancel(forceConfirm) && forceConfirm; - } + let shouldForce = force || yes; + if (!shouldForce && !ui.nonInteractive) { + const forceConfirm = await ui.confirm({ + message: `Branch "${wt.branch}" is not merged. Force delete?`, + initialValue: false, + }); + shouldForce = !ui.isCancel(forceConfirm) && forceConfirm; + } - if (shouldForce) { - spinner.start(`Force deleting branch "${wt.branch}"...`); - const forceResult = await git.deleteBranchForce(wt.branch); - if (Result.isErr(forceResult)) { - spinner.stop(pc.red(`Failed to delete branch "${wt.branch}"`)); - } else { - localDeleted = true; - } - } else { - ui.info(`Branch "${wt.branch}" was not deleted`); - } + if (shouldForce) { + spinner.start(`Force deleting branch "${wt.branch}"...`); + outcome = await deleteBranch( + { branch: wt.branch, force: true, deleteRemote: shouldDeleteRemoteBranches }, + { git }, + ); } else { - spinner.stop(pc.red(`Failed to delete branch "${wt.branch}"`)); + ui.info(`Branch "${wt.branch}" was not deleted`); } - } else { - localDeleted = true; } - if (localDeleted && shouldDeleteRemoteBranches) { - spinner.message(`Deleting remote branch "${wt.branch}"...`); - const deleteRemoteResult = await git.deleteRemoteBranch(wt.branch); - if (Result.isErr(deleteRemoteResult)) { + if (outcome.status === "deleted") { + if (outcome.remote.status === "deleted") { + spinner.stop(pc.green(`Branch "${wt.branch}" deleted (local & remote)`)); + } else { spinner.stop(pc.green(`Branch "${wt.branch}" deleted (local)`)); - if (deleteRemoteResult.error.code !== "REMOTE_REF_NOT_FOUND") { - ui.warn(`Failed to delete remote branch: ${deleteRemoteResult.error.message}`); + if (outcome.remote.status === "failed") { + ui.warn(`Failed to delete remote branch: ${outcome.remote.message}`); } - } else { - spinner.stop(pc.green(`Branch "${wt.branch}" deleted (local & remote)`)); } - } else if (localDeleted) { - spinner.stop(pc.green(`Branch "${wt.branch}" deleted (local)`)); + } else if (outcome.status === "failed") { + spinner.stop(pc.red(`Failed to delete branch "${wt.branch}"`)); } } } @@ -275,42 +268,26 @@ export function removeCommand(container: Container) { let branchStatus = ""; if (shouldDeleteBranches && wt.branch) { ms.update(wt.path, "deleting branch"); - let localDeleted = false; - const deleteResult = await git.deleteBranch(wt.branch); - - if (Result.isErr(deleteResult)) { - if (deleteResult.error.code === "BRANCH_NOT_MERGED") { - if (force || yes) { - const forceResult = await git.deleteBranchForce(wt.branch); - localDeleted = Result.isOk(forceResult); - } - if (!localDeleted) { - unmergedBranches.push(wt.branch); - branchStatus = " (branch kept — not fully merged)"; - } - } else { - branchStatus = " (branch delete failed)"; - warnings.push(`Failed to delete branch "${wt.branch}": ${deleteResult.error.message}`); - } - } else { - localDeleted = true; - } + const outcome = await deleteBranch( + { branch: wt.branch, force: force || yes, deleteRemote: shouldDeleteRemoteBranches }, + { git }, + ); - if (localDeleted && shouldDeleteRemoteBranches) { - ms.update(wt.path, "deleting remote branch"); - const deleteRemoteResult = await git.deleteRemoteBranch(wt.branch); - if (Result.isOk(deleteRemoteResult)) { + if (outcome.status === "deleted") { + if (outcome.remote.status === "deleted") { branchStatus = " + branch deleted (local & remote)"; } else { branchStatus = " + branch deleted (local)"; - if (deleteRemoteResult.error.code !== "REMOTE_REF_NOT_FOUND") { - warnings.push( - `Failed to delete remote branch "${wt.branch}": ${deleteRemoteResult.error.message}`, - ); + if (outcome.remote.status === "failed") { + warnings.push(`Failed to delete remote branch "${wt.branch}": ${outcome.remote.message}`); } } - } else if (localDeleted) { - branchStatus = " + branch deleted (local)"; + } else if (outcome.status === "not-merged") { + unmergedBranches.push(wt.branch); + branchStatus = " (branch kept — not fully merged)"; + } else { + branchStatus = " (branch delete failed)"; + warnings.push(`Failed to delete branch "${wt.branch}": ${outcome.message}`); } } @@ -331,21 +308,18 @@ export function removeCommand(container: Container) { for (const branch of unmergedBranches) { const spinner = ui.createSpinner(); spinner.start(`Force deleting branch "${branch}"...`); - const forceResult = await git.deleteBranchForce(branch); - if (Result.isOk(forceResult)) { - if (shouldDeleteRemoteBranches) { - spinner.message(`Deleting remote branch "${branch}"...`); - const remoteResult = await git.deleteRemoteBranch(branch); - if (Result.isOk(remoteResult)) { - spinner.stop(pc.green(`Branch "${branch}" deleted (local & remote)`)); - } else { - spinner.stop(pc.green(`Branch "${branch}" deleted (local)`)); - if (remoteResult.error.code !== "REMOTE_REF_NOT_FOUND") { - ui.warn(`Failed to delete remote branch: ${remoteResult.error.message}`); - } - } + const outcome = await deleteBranch( + { branch, force: true, deleteRemote: shouldDeleteRemoteBranches }, + { git }, + ); + if (outcome.status === "deleted") { + if (outcome.remote.status === "deleted") { + spinner.stop(pc.green(`Branch "${branch}" deleted (local & remote)`)); } else { spinner.stop(pc.green(`Branch "${branch}" deleted (local)`)); + if (outcome.remote.status === "failed") { + ui.warn(`Failed to delete remote branch: ${outcome.remote.message}`); + } } } else { spinner.stop(pc.red(`Failed to delete branch "${branch}"`)); @@ -353,9 +327,7 @@ export function removeCommand(container: Container) { } }; - if (yes) { - await forceDeleteUnmerged(); - } else if (!ui.nonInteractive) { + if (!ui.nonInteractive) { const confirmMessage = unmergedBranches.length === 1 ? `Branch "${unmergedBranches[0]}" is not fully merged. Force delete?`