From 95102b8d78fe6c5d91951c4b15afe6b0a0fe028d Mon Sep 17 00:00:00 2001 From: Federico Jaramillo Martinez Date: Fri, 24 Jul 2026 11:43:58 +0200 Subject: [PATCH] fix(git): show staged submodule pointer moves and support spaced submodule paths - parseStatus: detect staged submodule pointer moves by comparing the recorded HEAD/index OIDs (porcelain reports S... for a staged move, so the c flag never fires); staged moves previously vanished from the status response (PR #92 review finding B1). - parseStatus: keep deleted gitlinks (index or working tree) as plain deletion rows instead of deferring them as submodules. Unstaged deletions vanished entirely, and staged deletions would render as a bogus pointer move to the zero OID. The finding assumed N... porcelain; git 2.54 actually emits .D/D. S... (finding S3's stated outcome). - submodulePaths: parse 'git config -z' records so submodule paths with spaces survive .gitmodules key parsing instead of splitting lines at the first space (finding S1). - tests: strip inherited GIT_* env vars in the fixture helper so the suite also passes when run from a git hook (pre-commit sets GIT_DIR). Adds real-git fixture tests for staged moves, staged+dirty combos, deleted submodules, inner renames, and spaced submodule/file paths. --- src/server/git/gitService.test.ts | 126 +++++++++++++++++++++++++++++- src/server/git/gitService.ts | 39 +++++---- 2 files changed, 150 insertions(+), 15 deletions(-) diff --git a/src/server/git/gitService.test.ts b/src/server/git/gitService.test.ts index a5336bf..c1dce50 100644 --- a/src/server/git/gitService.test.ts +++ b/src/server/git/gitService.test.ts @@ -8,7 +8,15 @@ import { gitDiff, gitStatus } from "./gitService.js"; // Isolate from any global/system git config and force a deterministic identity; // `protocol.file.allow` is required for `submodule add` from a local path. const GIT_FLAGS = ["-c", "user.name=Test", "-c", "user.email=test@example.com", "-c", "protocol.file.allow=always", "-c", "commit.gpgsign=false"]; -const GIT_ENV = { ...process.env, GIT_CONFIG_GLOBAL: "/dev/null", GIT_CONFIG_SYSTEM: "/dev/null", GIT_TERMINAL_PROMPT: "0" }; +// Strip all GIT_* variables (e.g. GIT_DIR/GIT_INDEX_FILE, set by git hooks such +// as this repo's pre-commit verify run) so fixture commands never pick up an +// outer repository's environment, then pin the handful we rely on. +const GIT_ENV = Object.fromEntries([ + ...Object.entries(process.env).filter(([key]) => !key.startsWith("GIT_")), + ["GIT_CONFIG_GLOBAL", "/dev/null"], + ["GIT_CONFIG_SYSTEM", "/dev/null"], + ["GIT_TERMINAL_PROMPT", "0"], +]); const created: string[] = []; afterAll(() => { for (const dir of created) rmSync(dir, { recursive: true, force: true }); }); @@ -43,6 +51,27 @@ function createFixture(): { dir: string; c1: string; c2: string } { return { dir: sup, c1, c2 }; } +/** Superproject at `dir` whose only submodule lives at the spaced path + * `my sub`; the submodule origin has a single commit (a.txt=v1). */ +function createSpacedPathFixture(): { dir: string } { + const base = mkdtempSync(join(tmpdir(), "pi-web-sub-space-")); + created.push(base); + const origin = join(base, "origin"); + const sup = join(base, "sup"); + + git(base, ["init", "-b", "main", origin]); + writeFileSync(join(origin, "a.txt"), "v1\n"); + git(origin, ["add", "-A"]); + git(origin, ["commit", "-m", "c1"]); + + git(base, ["init", "-b", "main", sup]); + git(sup, ["submodule", "add", origin, "my sub"]); + writeFileSync(join(sup, "root.txt"), "root\n"); + git(sup, ["add", "-A"]); + git(sup, ["commit", "-m", "init"]); + return { dir: sup }; +} + describe("gitStatus with submodules", () => { it("surfaces a moved commit pointer with short SHAs and no inner files", async () => { const { dir, c1, c2 } = createFixture(); @@ -69,6 +98,85 @@ describe("gitStatus with submodules", () => { expect(inner).toContain("HARL/new.txt"); }); + it("surfaces a staged pointer move with the recorded OID as from and the staged OID as to", async () => { + const { dir, c1, c2 } = createFixture(); + git(join(dir, "HARL"), ["checkout", c1]); // move the pointer + git(dir, ["add", "HARL"]); // stage the move: porcelain `1 M. S... HARL` + + const status = await gitStatus(dir); + expect(status.submodules).toContain("HARL"); + const pointer = status.files.find((file) => file.path === "HARL"); + expect(pointer?.index).toBe("modified"); + expect(pointer?.workingTree).toBe("unmodified"); + expect(pointer?.submoduleFromCommit).toBe(c2.slice(0, 7)); + expect(pointer?.submoduleToCommit).toBe(c1.slice(0, 7)); + }); + + it("reports both the pointer entry and inner files for a staged move with dirty content", async () => { + const { dir, c1, c2 } = createFixture(); + git(join(dir, "HARL"), ["checkout", c1]); + git(dir, ["add", "HARL"]); + writeFileSync(join(dir, "HARL", "a.txt"), "v1\ndirty\n"); // combined `1 MM S.M.` + + const status = await gitStatus(dir); + const pointer = status.files.find((file) => file.path === "HARL"); + expect(pointer?.index).toBe("modified"); + expect(pointer?.workingTree).toBe("modified"); + expect(pointer?.submoduleFromCommit).toBe(c2.slice(0, 7)); + expect(pointer?.submoduleToCommit).toBe(c1.slice(0, 7)); + const inner = status.files.find((file) => file.path === "HARL/a.txt"); + expect(inner?.workingTree).toBe("modified"); + }); + + it("reports a deleted submodule as a plain deleted row", async () => { + const { dir } = createFixture(); + rmSync(join(dir, "HARL"), { recursive: true, force: true }); // unstaged deletion: `1 .D S...` + + const status = await gitStatus(dir); + const row = status.files.find((file) => file.path === "HARL"); + expect(row?.workingTree).toBe("deleted"); + expect(row?.submoduleFromCommit).toBeUndefined(); + expect(status.submodules).not.toContain("HARL"); + expect(status.files.some((file) => file.path.startsWith("HARL/"))).toBe(false); + }); + + it("reports a staged submodule deletion as a plain deleted row, not a pointer move", async () => { + const { dir } = createFixture(); + git(dir, ["rm", "-q", "HARL"]); // staged deletion: `1 D. S...` with a zero index OID + + const status = await gitStatus(dir); + const row = status.files.find((file) => file.path === "HARL"); + expect(row?.index).toBe("deleted"); + expect(row?.submoduleFromCommit).toBeUndefined(); + expect(status.submodules).not.toContain("HARL"); + }); + + it("prefixes oldPath with the submodule path for renames inside a submodule", async () => { + const { dir } = createFixture(); + git(join(dir, "HARL"), ["mv", "a.txt", "renamed.txt"]); + + const status = await gitStatus(dir); + const renamed = status.files.find((file) => file.path === "HARL/renamed.txt"); + expect(renamed?.index).toBe("renamed"); + expect(renamed?.oldPath).toBe("HARL/a.txt"); + }); + + it("keeps inner filenames with spaces intact through expansion", async () => { + const { dir } = createFixture(); + writeFileSync(join(dir, "HARL", "my file.txt"), "tracked\n"); + git(join(dir, "HARL"), ["add", "my file.txt"]); + git(join(dir, "HARL"), ["commit", "-m", "track spaced file"]); + git(dir, ["add", "HARL"]); + git(dir, ["commit", "-m", "record new pointer"]); // HARL clean at the new recorded commit + writeFileSync(join(dir, "HARL", "my file.txt"), "tracked\nchanged\n"); + writeFileSync(join(dir, "HARL", "untracked file.txt"), "new\n"); + + const status = await gitStatus(dir); + expect(status.files.find((file) => file.path === "HARL/my file.txt")?.workingTree).toBe("modified"); + expect(status.files.some((file) => file.path === "HARL/untracked file.txt")).toBe(true); + expect(status.files.find((file) => file.path === "HARL")).toBeUndefined(); // pointer unchanged + }); + it("skips inner recursion without throwing when the submodule repo is unreadable", async () => { const { dir } = createFixture(); writeFileSync(join(dir, "HARL", "new.txt"), "brand-new\n"); // untracked → would trigger recursion @@ -80,6 +188,22 @@ describe("gitStatus with submodules", () => { }); }); +describe("submodule paths containing spaces", () => { + it("expands status and routes diffs into the space-named submodule", async () => { + const { dir } = createSpacedPathFixture(); + writeFileSync(join(dir, "my sub", "a.txt"), "v1\nchanged\n"); + + const status = await gitStatus(dir); + expect(status.submodules).toContain("my sub"); + expect(status.files.some((file) => file.path === "my sub/a.txt")).toBe(true); + + const diff = await gitDiff(dir, { path: "my sub/a.txt" }); + expect(diff.path).toBe("my sub/a.txt"); + expect(diff.diff).toContain("@@"); + expect(diff.diff).toContain("changed"); + }); +}); + describe("gitDiff routing into submodules", () => { it("returns real content for a tracked file inside the submodule", async () => { const { dir } = createFixture(); diff --git a/src/server/git/gitService.ts b/src/server/git/gitService.ts index 7113a44..9dd2199 100644 --- a/src/server/git/gitService.ts +++ b/src/server/git/gitService.ts @@ -155,15 +155,17 @@ async function isUntracked(cwd: string, path: string): Promise { /** Configured direct-submodule paths (depth 1), read from `.gitmodules`. */ async function submodulePaths(cwd: string): Promise { - const result = await runGit(cwd, ["config", "--file", ".gitmodules", "--get-regexp", "^submodule\\..+\\.path$"]); + // `-z` emits `\n\0` records; keys may themselves contain spaces + // (`submodule.my sub.path`), so splitting lines at the first space mangles + // paths with spaces in them. + const result = await runGit(cwd, ["config", "-z", "--file", ".gitmodules", "--get-regexp", "^submodule\\..+\\.path$"]); if (result.code !== 0) return []; const paths: string[] = []; - for (const line of result.stdout.split("\n")) { - const trimmed = line.trim(); - if (trimmed === "") continue; - const spaceAt = trimmed.indexOf(" "); - if (spaceAt === -1) continue; - paths.push(trimmed.slice(spaceAt + 1)); + for (const record of result.stdout.split("\0")) { + if (record === "") continue; + const newlineAt = record.indexOf("\n"); + if (newlineAt === -1) continue; + paths.push(record.slice(newlineAt + 1)); } return paths; } @@ -201,19 +203,28 @@ function parseStatus(raw: string, options: { deferSubmodules: boolean }): Parsed const parts = record.split(" "); const sub = parts[2]; const path = parts.slice(8).join(" "); - if (options.deferSubmodules && sub?.startsWith("S") === true) { + const index = stateFor(parts[1]?.[0]); + const workingTree = stateFor(parts[1]?.[1]); + // A deleted gitlink has no pointer move or inner content to expand (a + // staged deletion even reports the index OID as all zeros), so keep it + // as a plain row instead of deferring it as a submodule. + if (options.deferSubmodules && sub?.startsWith("S") === true && index !== "deleted" && workingTree !== "deleted") { + const headOid = parts[6] ?? ""; + const indexOid = parts[7] ?? ""; submodules.push({ path, - index: stateFor(parts[1]?.[0]), - workingTree: stateFor(parts[1]?.[1]), - commitChanged: sub[1] === "C", + index, + workingTree, + // `c` only flags unstaged moves (submodule HEAD left the index OID); + // a staged move leaves HEAD == index, so compare the recorded OIDs. + commitChanged: sub[1] === "C" || headOid !== indexOid, hasModifiedContent: sub[2] === "M", hasUntrackedContent: sub[3] === "U", - headOid: parts[6] ?? "", - indexOid: parts[7] ?? "", + headOid, + indexOid, }); } else { - files.push({ path, index: stateFor(parts[1]?.[0]), workingTree: stateFor(parts[1]?.[1]) }); + files.push({ path, index, workingTree }); } } else if (record.startsWith("2 ")) { const parts = record.split(" ");