diff --git a/e2e/commitInclusion.spec.ts b/e2e/commitInclusion.spec.ts new file mode 100644 index 000000000..b5a0bda45 --- /dev/null +++ b/e2e/commitInclusion.spec.ts @@ -0,0 +1,90 @@ +/** + * #1539: committing a subset of the changed files must not check the ones left out again. + * Git is stubbed at the IPC layer, like `commit.spec.ts`. + */ +import type { Page } from "@playwright/test"; +import { expect, ipcCalls, stubDesktop, test } from "./fixtures"; + +const ROOT = "/dev/acme-api"; +const NAMES = Array.from({ length: 20 }, (_, i) => `src/f${i}.ts`); + +/** Twenty modified tracked files; `vcs_stage` + `vcs_commit` take the staged ones out of the status. */ +async function stubTwentyChanges(page: Page) { + await stubDesktop(page); + await page.evaluate( + async ({ root, names }) => { + const staged = new Set(); + const committed = new Set(); + const all = names.map((rel) => ({ + path: `${root}/${rel}`, + relPath: rel, + staged: null, + unstaged: "modified", + renamedFrom: null, + })); + const h = window.__ipc.handlers; + h.vcs_roots = () => [{ root, operation: null, head: "main", headCommit: "1111111" }]; + h.vcs_branch_info = () => ({ + head: "main", + headCommit: "1111111", + upstream: null, + ahead: 0, + behind: 0, + operation: null, + }); + h.vcs_status = () => all.filter((f) => !committed.has(f.path)); + h.vcs_stage = (args) => { + for (const path of args.paths as string[]) staged.add(path); + return null; + }; + h.vcs_unstage = () => null; + h.vcs_commit = () => { + for (const path of staged) committed.add(path); + staged.clear(); + return "2222222"; + }; + const vcsModule = "/src/features/vcs/store.ts"; + const { useVcs, refreshNow } = await import(/* @vite-ignore */ vcsModule); + useVcs.setState({ available: true, root, repo: root, branchesLoaded: false, stashesLoaded: false }); + await refreshNow(); + }, + { root: ROOT, names: NAMES }, + ); +} + +test("changes left out of a commit stay unchecked (#1539)", async ({ workbench: page }, testInfo) => { + await stubTwentyChanges(page); + await page.keyboard.press("Meta+k"); + const panel = page.getByRole("region", { name: "Commit", exact: true }); + const changes = panel.getByRole("tree", { name: "Changes" }); + + // Uncheck everything, then pick the 5 files to commit. + await changes.getByRole("checkbox", { name: "Include Changes in commit" }).click(); + for (const name of NAMES.slice(0, 5)) { + await changes.getByRole("checkbox", { name: `Include ${name.split("/").pop()} in commit` }).click(); + } + await expect(changes.getByRole("checkbox", { name: "Include f0.ts in commit" })).toHaveAttribute( + "aria-checked", + "true", + ); + + await panel.getByRole("textbox", { name: "Commit message" }).fill("Commit 5 of 20"); + await panel.getByRole("button", { name: "Commit", exact: true }).click(); + await expect(page.getByText(/5 files committed: Commit 5 of 20/)).toBeVisible(); + expect((await ipcCalls(page, "vcs_stage")).flatMap((a) => a.paths as string[]).sort()).toEqual( + NAMES.slice(0, 5) + .map((rel) => `${ROOT}/${rel}`) + .sort(), + ); + + // The 15 files left out are still in the tree, unchecked: the next commit cannot take them by surprise. + const leftOut = changes.getByRole("checkbox", { name: "Include f19.ts in commit" }); + await expect(leftOut).toBeVisible(); + await page.screenshot({ path: testInfo.outputPath("commit-inclusion.png"), animations: "disabled" }); + for (const name of NAMES.slice(5)) { + await expect(changes.getByRole("checkbox", { name: `Include ${name.split("/").pop()} in commit` })).toHaveAttribute( + "aria-checked", + "false", + ); + } +}); diff --git a/src/features/vcs/__tests__/changes.test.ts b/src/features/vcs/__tests__/changes.test.ts index 38ad0400b..679dd59ef 100644 --- a/src/features/vcs/__tests__/changes.test.ts +++ b/src/features/vcs/__tests__/changes.test.ts @@ -3,12 +3,14 @@ import type { FileStatus } from "@/bindings"; import { aggregateCheck, buildChangeTree, + checkOf, commitRepos, ignoreTargets, initialCheck, hunkIncluded, groupFileNodes, planCommit, + remainingOverrides, stagingSide, toggledHunk, type CheckState, @@ -218,6 +220,28 @@ describe("commit inclusion", () => { expect(aggregateCheck([status[1]!], checks)).toMatchObject({ checked: false, indeterminate: true }); }); + it("keeps the changes left out of a commit excluded after it", () => { + const files = Array.from({ length: 20 }, (_, i) => fs(`src/f${i}.kt`, null, "modified")); + const leftOut = files.slice(5); + const checks = new Map(leftOut.map((f) => [f.path, "none"])); + const plan = planCommit(files, checks); + expect(plan.included).toHaveLength(5); + + // The commit takes its 5 files out of the tree; the 15 left out keep their override. + const kept = remainingOverrides(checks, plan.included); + expect(kept.has(files[0]!.path)).toBe(false); + expect(leftOut.every((f) => checkOf(f, kept) === "none")).toBe(true); + expect(aggregateCheck(leftOut, kept)).toMatchObject({ checked: false, indeterminate: false }); + }); + + it("drops only the committed paths from the per-file overrides", () => { + const overrides = new Map([ + ["/r/a.ts", new Set(["h1"])], + ["/r/b.ts/", new Set(["h2"])], + ]); + expect([...remainingOverrides(overrides, [fs("a.ts", null, "modified")])]).toEqual([["/r/b.ts/", new Set(["h2"])]]); + }); + it("commits a file with excluded blocks partially, without staging it", () => { const checks = new Map([["/r/src/main/kotlin/App.kt", "hunks"]]); const plan = planCommit(status, checks); diff --git a/src/features/vcs/__tests__/commitInclusion.test.ts b/src/features/vcs/__tests__/commitInclusion.test.ts new file mode 100644 index 000000000..9b84e9777 --- /dev/null +++ b/src/features/vcs/__tests__/commitInclusion.test.ts @@ -0,0 +1,73 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const unstage = vi.hoisted(() => vi.fn()); +const stage = vi.hoisted(() => vi.fn()); + +vi.mock("@/platform/ipc", async (importOriginal) => { + const real = await importOriginal(); + const ok = (data: unknown) => Promise.resolve({ status: "ok" as const, data }); + return { + ...real, + isTauri: () => false, + call: async (p: Promise<{ status: "ok"; data: unknown }>) => (await p).data, + commands: { + ...real.commands, + vcsUnstage: unstage.mockImplementation(() => ok(null)), + vcsStage: stage.mockImplementation(() => ok(null)), + vcsCommit: () => ok("commit1"), + }, + }; +}); + +import type { FileStatus } from "@/bindings"; +import { commitChanges } from "../changeOps"; +import { useCommit } from "../commitStore"; +import { useVcs } from "../store"; + +const files: FileStatus[] = Array.from({ length: 20 }, (_, i) => ({ + path: `/p/src/f${i}.ts`, + relPath: `src/f${i}.ts`, + staged: null, + unstaged: "modified", + renamedFrom: null, +})); + +const excluded = files.slice(5); + +describe("changes left out of a commit (#1539)", () => { + beforeEach(() => { + localStorage.clear(); + unstage.mockClear(); + stage.mockClear(); + useVcs.setState({ + available: false, + root: "/p", + repo: "/p", + roots: ["/p"], + status: files, + branch: null, + operations: {}, + }); + useCommit.setState({ + message: "Commit 5 of 20", + amend: false, + checks: new Map(excluded.map((f) => [f.path, "none" as const])), + excludedHunks: new Map([[files[5]!.path, new Set(["h1"])]]), + checkProblems: [], + }); + }); + + it("keeps the 15 excluded files excluded instead of checking them again", async () => { + expect(await commitChanges()).toBe("commit1"); + // Only the 5 checked files were staged for the commit. + expect(stage.mock.calls[0]?.[0]).toEqual(files.slice(0, 5).map((f) => f.path)); + expect(unstage).not.toHaveBeenCalled(); + + const { checks, excludedHunks } = useCommit.getState(); + expect(checks.size).toBe(15); + expect(excluded.every((f) => checks.get(f.path) === "none")).toBe(true); + expect(files.slice(0, 5).some((f) => checks.has(f.path))).toBe(false); + // Block exclusions of files still in the tree are kept too. + expect(excludedHunks.get(files[5]!.path)).toEqual(new Set(["h1"])); + }); +}); diff --git a/src/features/vcs/changeOps.tsx b/src/features/vcs/changeOps.tsx index 3a2ad7a01..54569f79e 100644 --- a/src/features/vcs/changeOps.tsx +++ b/src/features/vcs/changeOps.tsx @@ -4,7 +4,7 @@ import type { FileStatus, PartialCommitFile, Stash } from "@/bindings"; import { call, commands } from "@/platform/ipc"; import { Checkbox, Dialog, Icon, Input, T, fileIconName } from "@/ui"; import { notifications } from "@/workbench/contributions"; -import { commitRepos, groupOf, planCommit, renamedFromPath } from "./changes"; +import { commitRepos, groupOf, planCommit, remainingOverrides, renamedFromPath } from "./changes"; import { useChangelists, withListDefaults } from "./changelists"; import { runCommitChecks } from "./commitChecks"; import { forgetAmendDraft, keepMessageAfterCommit, rememberMessage, useCommit } from "./commitStore"; @@ -114,7 +114,14 @@ export async function commitChanges(): Promise { if (id !== undefined) { rememberMessage(message); forgetAmendDraft(); - useCommit.setState({ amend: false, checks: new Map(), excludedHunks: new Map(), checkProblems: [] }); + // What the user left out of this commit stays left out; only the committed changes start over. + const { checks: userChecks, excludedHunks } = useCommit.getState(); + useCommit.setState({ + amend: false, + checks: remainingOverrides(userChecks, plan.included), + excludedHunks: remainingOverrides(excludedHunks, plan.included), + checkProblems: [], + }); keepMessageAfterCommit(message, workflowSettings().vcs.commit.clearInitialMessage); } return id; diff --git a/src/features/vcs/changes.ts b/src/features/vcs/changes.ts index 9559885b5..43301a235 100644 --- a/src/features/vcs/changes.ts +++ b/src/features/vcs/changes.ts @@ -381,6 +381,20 @@ export function planCommit(status: readonly FileStatus[], checks: ReadonlyMap( + overrides: ReadonlyMap, + committed: readonly FileStatus[], +): Map { + const done = new Set(committed.map((f) => stripSlash(f.path))); + return new Map([...overrides].filter(([path]) => !done.has(stripSlash(path)))); +} + /** * Toggling the checkbox of one changed block (`key`, one of `all`) in the diff: the file's * new inclusion and excluded blocks. All blocks back in → `all`; none left → `none`.