From 2e31abcc16dd3042d1bc8ea3550f4eab8a8d9499 Mon Sep 17 00:00:00 2001 From: Matt Anger Date: Sat, 6 Jun 2026 18:29:25 -0700 Subject: [PATCH] feat(coding-agent): address PR comments and improve reftable support --- .../src/modes/components/footer.ts | 4 +- .../modes/components/status-line/component.ts | 11 +- packages/coding-agent/src/utils/git.ts | 126 +++++++++++++----- .../coding-agent/test/git-reftable.test.ts | 84 +++++++++--- 4 files changed, 170 insertions(+), 55 deletions(-) diff --git a/packages/coding-agent/src/modes/components/footer.ts b/packages/coding-agent/src/modes/components/footer.ts index c9e2f9619..e21f58baa 100644 --- a/packages/coding-agent/src/modes/components/footer.ts +++ b/packages/coding-agent/src/modes/components/footer.ts @@ -1,4 +1,5 @@ import * as fs from "node:fs"; +import * as path from "node:path"; import { stripVTControlCharacters } from "node:util"; import { ThinkingLevel } from "@oh-my-pi/pi-agent-core"; import { type Component, padding, truncateToWidth, visibleWidth } from "@oh-my-pi/pi-tui"; @@ -65,7 +66,8 @@ export class FooterComponent implements Component { } try { - this.#gitWatcher = fs.watch(head.headPath, () => { + const watchPath = head.isReftable ? path.join(head.commonDir, "reftable", "tables.list") : head.headPath; + this.#gitWatcher = fs.watch(watchPath, () => { this.#cachedBranch = undefined; // Invalidate cache if (this.#onBranchChange) { this.#onBranchChange(); diff --git a/packages/coding-agent/src/modes/components/status-line/component.ts b/packages/coding-agent/src/modes/components/status-line/component.ts index 434a9d523..1144cff04 100644 --- a/packages/coding-agent/src/modes/components/status-line/component.ts +++ b/packages/coding-agent/src/modes/components/status-line/component.ts @@ -1,4 +1,5 @@ import * as fs from "node:fs"; +import * as path from "node:path"; import type { AgentMessage } from "@oh-my-pi/pi-agent-core"; import { estimateTokens } from "@oh-my-pi/pi-agent-core/compaction"; import { type Component, truncateToWidth, visibleWidth } from "@oh-my-pi/pi-tui"; @@ -238,11 +239,15 @@ export class StatusLineComponent implements Component { this.#gitWatcher = null; } - const gitHeadPath = git.repo.resolveSync(getProjectDir())?.headPath ?? null; - if (!gitHeadPath) return; + const repository = git.repo.resolveSync(getProjectDir()); + if (!repository) return; + + const watchPath = git.repo.isReftableSync(repository) + ? path.join(repository.commonDir, "reftable", "tables.list") + : repository.headPath; try { - this.#gitWatcher = fs.watch(gitHeadPath, () => { + this.#gitWatcher = fs.watch(watchPath, () => { this.#invalidateGitCaches(); if (this.#onBranchChange) { this.#onBranchChange(); diff --git a/packages/coding-agent/src/utils/git.ts b/packages/coding-agent/src/utils/git.ts index bebc0b028..0c1be505b 100644 --- a/packages/coding-agent/src/utils/git.ts +++ b/packages/coding-agent/src/utils/git.ts @@ -27,6 +27,7 @@ export interface GitRepository { gitEntryPath: string; headPath: string; repoRoot: string; + isReftable?: boolean; } export interface GitStatusSummary { @@ -560,8 +561,6 @@ function parsePackedRefs(content: string | null, targetRef: string): string | nu return null; } -const reftableCache = new Map(); - function parseGitConfigHasReftable(content: string): boolean { let inExtensions = false; for (const line of content.split("\n")) { @@ -570,12 +569,35 @@ function parseGitConfigHasReftable(content: string): boolean { const section = trimmed.slice(1, -1).trim().toLowerCase(); inExtensions = section === "extensions"; } else if (inExtensions) { - const parts = trimmed.split("="); - if (parts.length >= 2) { - const key = parts[0].trim().toLowerCase(); - const value = parts.slice(1).join("=").trim().toLowerCase(); - if (key === "refstorage" && value === "reftable") { - return true; + const eqIndex = trimmed.indexOf("="); + if (eqIndex !== -1) { + const key = trimmed.slice(0, eqIndex).trim().toLowerCase(); + const value = trimmed.slice(eqIndex + 1).trim(); + if (key === "refstorage") { + // Strip trailing comments per git-config(5) + let cleanValue = ""; + let inQuotes = false; + for (let i = 0; i < value.length; i++) { + const char = value[i]; + if (char === '"') { + inQuotes = !inQuotes; + cleanValue += char; + } else if (!inQuotes && (char === ";" || char === "#")) { + if (i === 0 || /\s/.test(value[i - 1])) { + break; + } + cleanValue += char; + } else { + cleanValue += char; + } + } + cleanValue = cleanValue.trim().toLowerCase(); + if (cleanValue.startsWith('"') && cleanValue.endsWith('"')) { + cleanValue = cleanValue.slice(1, -1).trim(); + } + if (cleanValue === "reftable") { + return true; + } } } } @@ -584,28 +606,36 @@ function parseGitConfigHasReftable(content: string): boolean { } function isReftableRepoSync(repository: GitRepository): boolean { - const cached = reftableCache.get(repository.commonDir); - if (cached !== undefined) return cached; + if (repository.isReftable !== undefined) return repository.isReftable; const configPath = path.join(repository.commonDir, "config"); const content = readOptionalTextSync(configPath); - const hasReftable = content ? parseGitConfigHasReftable(content) : false; - reftableCache.set(repository.commonDir, hasReftable); - return hasReftable; + repository.isReftable = content ? parseGitConfigHasReftable(content) : false; + return repository.isReftable; } async function isReftableRepo(repository: GitRepository): Promise { - const cached = reftableCache.get(repository.commonDir); - if (cached !== undefined) return cached; + if (repository.isReftable !== undefined) return repository.isReftable; const configPath = path.join(repository.commonDir, "config"); const content = await readOptionalText(configPath); - const hasReftable = content ? parseGitConfigHasReftable(content) : false; - reftableCache.set(repository.commonDir, hasReftable); - return hasReftable; + repository.isReftable = content ? parseGitConfigHasReftable(content) : false; + return repository.isReftable; } -async function resolveHeadStateReftable(repository: GitRepository): Promise { - const symResult = await git(repository.repoRoot, ["symbolic-ref", "HEAD"], { readOnly: true }).catch(() => null); - const revResult = await git(repository.repoRoot, ["rev-parse", "HEAD"], { readOnly: true }).catch(() => null); +async function resolveHeadStateReftable(repository: GitRepository, signal?: AbortSignal): Promise { + throwIfAborted(signal); + const symResult = await git(repository.repoRoot, ["symbolic-ref", "HEAD"], { readOnly: true, signal }).catch(err => { + if (signal?.aborted || (err instanceof Error && (err.name === "AbortError" || err.name === "ToolAbortError"))) { + throw err; + } + return null; + }); + throwIfAborted(signal); + const revResult = await git(repository.repoRoot, ["rev-parse", "HEAD"], { readOnly: true, signal }).catch(err => { + if (signal?.aborted || (err instanceof Error && (err.name === "AbortError" || err.name === "ToolAbortError"))) { + throw err; + } + return null; + }); const commit = revResult && revResult.exitCode === 0 ? revResult.stdout.trim() || null : null; if (symResult && symResult.exitCode === 0) { @@ -707,15 +737,35 @@ function readRefSync(repository: GitRepository, targetRef: string): string | nul return null; } -async function readRef(repository: GitRepository, targetRef: string): Promise { +async function readRef(repository: GitRepository, targetRef: string, signal?: AbortSignal): Promise { if (await isReftableRepo(repository)) { - const symResult = await git(repository.repoRoot, ["symbolic-ref", targetRef], { readOnly: true }).catch( - () => null, + throwIfAborted(signal); + const symResult = await git(repository.repoRoot, ["symbolic-ref", targetRef], { readOnly: true, signal }).catch( + err => { + if ( + signal?.aborted || + (err instanceof Error && (err.name === "AbortError" || err.name === "ToolAbortError")) + ) { + throw err; + } + return null; + }, ); if (symResult && symResult.exitCode === 0) { return `${HEAD_REF_PREFIX} ${symResult.stdout.trim()}`; } - const revResult = await git(repository.repoRoot, ["rev-parse", targetRef], { readOnly: true }).catch(() => null); + throwIfAborted(signal); + const revResult = await git(repository.repoRoot, ["rev-parse", targetRef], { readOnly: true, signal }).catch( + err => { + if ( + signal?.aborted || + (err instanceof Error && (err.name === "AbortError" || err.name === "ToolAbortError")) + ) { + throw err; + } + return null; + }, + ); if (revResult && revResult.exitCode === 0) { return revResult.stdout.trim() || null; } @@ -1146,7 +1196,7 @@ export const branch = { const repository = await resolveRepository(cwd); if (repository) { for (const refPath of DEFAULT_BRANCH_REFS) { - const target = await readRef(repository, refPath); + const target = await readRef(repository, refPath, signal); const branchName = parseDefaultBranchRef(refPath, target); if (branchName) return branchName; } @@ -1244,7 +1294,7 @@ export const ref = { async exists(cwd: string, refName: string, signal?: AbortSignal): Promise { if (refName === "HEAD") return (await head.sha(cwd, signal)) !== null; const repository = await resolveRepository(cwd); - if (repository && refName.startsWith("refs/")) return (await readRef(repository, refName)) !== null; + if (repository && refName.startsWith("refs/")) return (await readRef(repository, refName, signal)) !== null; const result = await git(cwd, ["show-ref", "--verify", "--quiet", refName], { readOnly: true, signal }); return result.exitCode === 0; }, @@ -1253,7 +1303,7 @@ export const ref = { async resolve(cwd: string, refName: string, signal?: AbortSignal): Promise { if (refName === "HEAD") return head.sha(cwd, signal); const repository = await resolveRepository(cwd); - if (repository && refName.startsWith("refs/")) return readRef(repository, refName); + if (repository && refName.startsWith("refs/")) return readRef(repository, refName, signal); const result = await git(cwd, ["rev-parse", refName], { readOnly: true, signal }); if (result.exitCode !== 0) return null; return result.stdout.trim() || null; @@ -1546,11 +1596,11 @@ export const ls = { export const head = { /** Full HEAD state (branch, commit, repo info). */ - async resolve(cwd: string): Promise { + async resolve(cwd: string, signal?: AbortSignal): Promise { const repository = await resolveRepository(cwd); if (!repository) return null; if (await isReftableRepo(repository)) { - return resolveHeadStateReftable(repository); + return resolveHeadStateReftable(repository, signal); } const content = await readOptionalText(repository.headPath); if (content === null) return null; @@ -1571,7 +1621,7 @@ export const head = { /** Current HEAD commit SHA. */ async sha(cwd: string, signal?: AbortSignal): Promise { - const headState = await head.resolve(cwd); + const headState = await head.resolve(cwd, signal); if (headState?.commit) return headState.commit; const result = await git(cwd, ["rev-parse", "HEAD"], { readOnly: true, signal }); if (result.exitCode !== 0) return null; @@ -1626,11 +1676,21 @@ export const repo = { resolve(cwd: string): Promise { return resolveRepository(cwd); }, + + /** Check if the repository uses the reftable reference storage format (sync). */ + isReftableSync(repository: GitRepository): boolean { + return isReftableRepoSync(repository); + }, + + /** Check if the repository uses the reftable reference storage format. */ + isReftable(repository: GitRepository): Promise { + return isReftableRepo(repository); + }, }; // Helper used during head resolution — defined here to reference `head` namespace. -async function resolveHead(cwd: string): Promise { - return head.resolve(cwd); +async function resolveHead(cwd: string, signal?: AbortSignal): Promise { + return head.resolve(cwd, signal); } // ════════════════════════════════════════════════════════════════════════════ diff --git a/packages/coding-agent/test/git-reftable.test.ts b/packages/coding-agent/test/git-reftable.test.ts index 9187d93e6..defcd0e91 100644 --- a/packages/coding-agent/test/git-reftable.test.ts +++ b/packages/coding-agent/test/git-reftable.test.ts @@ -1,15 +1,18 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; import * as fs from "node:fs/promises"; +import * as os from "node:os"; import * as path from "node:path"; import { $ } from "bun"; import * as git from "../src/utils/git"; -describe("git reftable support", () => { +const gitInitHelp = await $`git init -h`.quiet().nothrow().text(); +const supportsReftable = gitInitHelp.includes("--ref-format"); + +describe.skipIf(!supportsReftable)("git reftable support", () => { let testRepoDir: string; beforeEach(async () => { - testRepoDir = path.join(import.meta.dir, `tmp-reftable-test-${Date.now()}`); - await fs.mkdir(testRepoDir, { recursive: true }); + testRepoDir = await fs.mkdtemp(path.join(os.tmpdir(), "omp-reftable-")); }); afterEach(async () => { @@ -18,15 +21,8 @@ describe("git reftable support", () => { test("resolves references in a reftable repository", async () => { // Initialize the repository with reftable format - const initResult = await $`git init --ref-format=reftable --initial-branch=main` - .cwd(testRepoDir) - .quiet() - .nothrow(); - if (initResult.exitCode !== 0) { - // If the installed git doesn't support --ref-format=reftable, skip the test - console.warn("Skipping reftable test: Git does not support --ref-format=reftable"); - return; - } + const initResult = await $`git init --ref-format=reftable --initial-branch=main`.cwd(testRepoDir).quiet(); + expect(initResult.exitCode).toBe(0); // Configure basic user details so we can commit await $`git config user.name "Test User"`.cwd(testRepoDir).quiet(); @@ -66,16 +62,16 @@ describe("git reftable support", () => { // Test HEAD resolution (object shape) const headState = await git.head.resolve(testRepoDir); expect(headState).not.toBeNull(); - expect(headState?.kind).toBe("ref"); - expect((headState as any).branchName).toBe("feature-branch"); - expect(headState?.commit).toBe(headSha); + if (headState?.kind !== "ref") throw new Error("expected ref head"); + expect(headState.branchName).toBe("feature-branch"); + expect(headState.commit).toBe(headSha); // Test HEAD resolution sync const headStateSync = git.head.resolveSync(testRepoDir); expect(headStateSync).not.toBeNull(); - expect(headStateSync?.kind).toBe("ref"); - expect((headStateSync as any).branchName).toBe("feature-branch"); - expect(headStateSync?.commit).toBe(headSha); + if (headStateSync?.kind !== "ref") throw new Error("expected ref head sync"); + expect(headStateSync.branchName).toBe("feature-branch"); + expect(headStateSync.commit).toBe(headSha); // Test exists check const mainExists = await git.ref.exists(testRepoDir, "refs/heads/main"); @@ -83,4 +79,56 @@ describe("git reftable support", () => { expect(mainExists).toBe(true); expect(nonexistentExists).toBe(false); }); + + test("handles git config trailing comments correctly", async () => { + // Initialize the repository with reftable format + const initResult = await $`git init --ref-format=reftable --initial-branch=main`.cwd(testRepoDir).quiet(); + expect(initResult.exitCode).toBe(0); + + const repository = await git.repo.resolve(testRepoDir); + expect(repository).not.toBeNull(); + if (!repository) return; + expect(await git.repo.isReftable(repository)).toBe(true); + + // Now let's manually write to .git/config with comments and test + const configPath = path.join(repository.commonDir, "config"); + const baseConfig = await fs.readFile(configPath, "utf8"); + + // Test trailing semicolon comment + const newConfigWithSemicolon = baseConfig.replace( + "refstorage = reftable", + "refstorage = reftable ; trailing comment", + ); + await fs.writeFile(configPath, newConfigWithSemicolon); + + const repository2 = await git.repo.resolve(testRepoDir); + expect(repository2).not.toBeNull(); + if (repository2) { + expect(await git.repo.isReftable(repository2)).toBe(true); + expect(git.repo.isReftableSync(repository2)).toBe(true); + } + + // Test trailing hash comment + const newConfigWithHash = baseConfig.replace("refstorage = reftable", "refstorage = reftable # trailing hash"); + await fs.writeFile(configPath, newConfigWithHash); + + const repository3 = await git.repo.resolve(testRepoDir); + expect(repository3).not.toBeNull(); + if (repository3) { + expect(await git.repo.isReftable(repository3)).toBe(true); + expect(git.repo.isReftableSync(repository3)).toBe(true); + } + + // Test double-quoted value containing semicolon (not a comment) + const newConfigWithQuotes = baseConfig.replace("refstorage = reftable", 'refstorage = "reftable ; not comment"'); + await fs.writeFile(configPath, newConfigWithQuotes); + + const repository4 = await git.repo.resolve(testRepoDir); + expect(repository4).not.toBeNull(); + if (repository4) { + // This value would be "reftable ; not comment", which shouldn't match "reftable" + expect(await git.repo.isReftable(repository4)).toBe(false); + expect(git.repo.isReftableSync(repository4)).toBe(false); + } + }); });