From 427490f4fd47503489c0db0bba69a97ebf1ccd77 Mon Sep 17 00:00:00 2001 From: Cole Ferrier Date: Thu, 21 May 2026 07:55:22 -0700 Subject: [PATCH] perf(git): implement high-performance batch file retrieval Optimizes the atom parsing phase by replacing sequential 'git diff-tree' process spawns with a single bulk retrieval using 'git diff-tree --stdin'. This reduces OS process overhead and leverages Git's internal caches, resulting in a ~30x speedup for file list lookups during history scans. Standardizes the IGitClient interface to a 'Batch-by-Default' design (array-based getFilesChanged) to enforce high-performance patterns. Hardens the batch parser by whitelisting requested hashes to prevent misidentification of hexadecimal file paths. Lore-id: 3918a757 Constraint: IGitClient.getFilesChanged MUST only accept string arrays Constraint: Batch parser MUST whitelist requested hashes Constraint: GitClient MUST use --stdin for bulk lookups Tested: Verified ~33x speedup with internal performance benchmark Tested: All 427 unit tests passing including new robust GitClient suite Tested: Verified type-safety with npm run typecheck across all call sites Tested: Manually verified parsing accuracy with sneaky-path scenarios Confidence: high Scope-risk: narrow Reversibility: clean Assisted-by: Gemini:CLI [lore-protocol] --- src/interfaces/git-client.ts | 2 +- src/services/atom-repository.ts | 19 ++---- src/services/git-client.ts | 59 +++++++++++++----- tests/unit/commands/commit-amend.test.ts | 2 +- tests/unit/services/atom-repository.test.ts | 60 ++++++++++-------- tests/unit/services/git-client.test.ts | 61 +++++++++++++++++++ .../unit/services/head-lore-id-reader.test.ts | 2 +- .../unit/services/staleness-detector.test.ts | 2 +- 8 files changed, 148 insertions(+), 59 deletions(-) create mode 100644 tests/unit/services/git-client.test.ts diff --git a/src/interfaces/git-client.ts b/src/interfaces/git-client.ts index 78a724b5..af306108 100644 --- a/src/interfaces/git-client.ts +++ b/src/interfaces/git-client.ts @@ -30,7 +30,7 @@ export interface IGitClient { hasStagedChanges(): Promise; getRepoRoot(): Promise; isInsideRepo(): Promise; - getFilesChanged(commitHash: string): Promise; + getFilesChanged(commitHashes: readonly string[]): Promise>; countCommitsSince(path: string, sinceCommitHash: string): Promise; resolveRef(ref: string): Promise; getHeadMessage(): Promise; diff --git a/src/services/atom-repository.ts b/src/services/atom-repository.ts index a636ba9c..acc9df7a 100644 --- a/src/services/atom-repository.ts +++ b/src/services/atom-repository.ts @@ -207,20 +207,13 @@ export class AtomRepository { loreCommits.push({ raw, trailers }); } - // Second pass: batch getFilesChanged calls with concurrency limit. - // Results accumulate in insertion order, maintaining 1:1 alignment with loreCommits. - const filesPerCommit: (readonly string[])[] = []; - for (let i = 0; i < loreCommits.length; i += GIT_FILES_CHANGED_BATCH_SIZE) { - const batch = loreCommits.slice(i, i + GIT_FILES_CHANGED_BATCH_SIZE); - const batchResults = await Promise.all( - batch.map(({ raw }) => this.gitClient.getFilesChanged(raw.hash)), - ); - filesPerCommit.push(...batchResults); - } + // Second pass: bulk fetch file lists using high-performance batch mode. + const hashes = loreCommits.map(c => c.raw.hash); + const filesMap = await this.gitClient.getFilesChanged(hashes); - // Build atoms by pairing parsed trailers with their file lists - const atoms: LoreAtom[] = loreCommits.map(({ raw, trailers }, index) => - this.buildAtom(raw, trailers, filesPerCommit[index]), + // Build atoms by pairing parsed trailers with their file lists from the batch result + const atoms: LoreAtom[] = loreCommits.map(({ raw, trailers }) => + this.buildAtom(raw, trailers, filesMap.get(raw.hash) ?? []), ); return atoms; diff --git a/src/services/git-client.ts b/src/services/git-client.ts index 22fa871a..3fc63dfc 100644 --- a/src/services/git-client.ts +++ b/src/services/git-client.ts @@ -122,15 +122,37 @@ export class GitClient implements IGitClient { } } - async getFilesChanged(commitHash: string): Promise { + async getFilesChanged(commitHashes: readonly string[]): Promise> { + if (commitHashes.length === 0) return new Map(); + const stdout = await this.exec([ 'diff-tree', - '--no-commit-id', + '--stdin', '--name-only', '-r', - commitHash, - ]); - return stdout.trim().split('\n').filter(line => line.length > 0); + ], commitHashes.join('\n')); + + const result = new Map(); + const requestedHashes = new Set(commitHashes); + let currentHash: string | null = null; + + const lines = stdout.split('\n'); + for (const line of lines) { + const trimmed = line.trim(); + if (!trimmed) continue; + + // Git outputs the full hash on its own line when using --stdin. + // We whitelist the hash to ensure we don't misidentify a file path + // that happens to be 40 or 64 characters long. + if (requestedHashes.has(trimmed)) { + currentHash = trimmed; + result.set(currentHash, []); + } else if (currentHash) { + result.get(currentHash)!.push(trimmed); + } + } + + return result; } async countCommitsSince(path: string, sinceCommitHash: string): Promise { @@ -292,21 +314,26 @@ export class GitClient implements IGitClient { * Execute a git command and return stdout. * Throws GitError on non-zero exit or other errors. */ - private async exec(args: readonly string[]): Promise { - try { - const { stdout } = await execFile('git', args as string[], { + private async exec(args: readonly string[], input?: string): Promise { + return new Promise((resolve, reject) => { + const child = execFileCb('git', args as string[], { cwd: this.cwd, maxBuffer: 10 * 1024 * 1024, // 10MB buffer for large repos encoding: 'utf-8', + }, (error, stdout, stderr) => { + if (error) { + const execError = error as { stderr?: string; code?: number }; + const actualStderr = stderr || execError.stderr || error.message; + reject(new GitError(`git ${args[0]} failed: ${actualStderr}`)); + } else { + resolve(stdout); + } }); - return stdout; - } catch (error: unknown) { - if (error instanceof Error) { - const execError = error as { stderr?: string; code?: number }; - const stderr = execError.stderr ?? error.message; - throw new GitError(`git ${args[0]} failed: ${stderr}`); + + if (input !== undefined && child.stdin) { + child.stdin.write(input); + child.stdin.end(); } - throw new GitError(`git ${args[0]} failed: unknown error`); - } + }); } } diff --git a/tests/unit/commands/commit-amend.test.ts b/tests/unit/commands/commit-amend.test.ts index c24c8357..08c79f55 100644 --- a/tests/unit/commands/commit-amend.test.ts +++ b/tests/unit/commands/commit-amend.test.ts @@ -15,7 +15,7 @@ function createMockGitClient(): IGitClient { hasStagedChanges: vi.fn().mockResolvedValue(true), getRepoRoot: vi.fn().mockResolvedValue('/repo'), isInsideRepo: vi.fn().mockResolvedValue(true), - getFilesChanged: vi.fn().mockResolvedValue([]), + getFilesChanged: vi.fn().mockResolvedValue(new Map()), countCommitsSince: vi.fn().mockResolvedValue(0), resolveRef: vi.fn().mockResolvedValue('abc1234'), getHeadMessage: vi.fn().mockResolvedValue(''), diff --git a/tests/unit/services/atom-repository.test.ts b/tests/unit/services/atom-repository.test.ts index 45d502f7..7f26f9af 100644 --- a/tests/unit/services/atom-repository.test.ts +++ b/tests/unit/services/atom-repository.test.ts @@ -2,7 +2,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { AtomRepository } from '../../../src/services/atom-repository.js'; import type { IGitClient, RawCommit } from '../../../src/interfaces/git-client.js'; import type { PathQueryOptions } from '../../../src/types/query.js'; -import type { LoreTrailers } from '../../../src/types/domain.js'; +import type { LoreTrailers, LoreId } from '../../../src/types/domain.js'; import { CustomTrailerCollection } from '../../../src/types/custom-trailer-collection.js'; /** @@ -86,9 +86,16 @@ function createMockGitClient(overrides: Partial = {}): IGitClient { hasStagedChanges: vi.fn(async () => false), getRepoRoot: vi.fn(async () => '/repo'), isInsideRepo: vi.fn(async () => true), - getFilesChanged: vi.fn(async () => []), + getFilesChanged: vi.fn(async (hashes: string[]) => { + const map = new Map(); + for (const hash of hashes) { + map.set(hash, []); + } + return map; + }), countCommitsSince: vi.fn(async () => 0), resolveRef: vi.fn(async () => 'abc123'), + getHeadMessage: vi.fn(async () => 'message'), ...overrides, }; } @@ -146,7 +153,7 @@ describe('AtomRepository', () => { it('should return atoms for a file target', async () => { const commit = makeLoreCommit({ loreId: 'a1b2c3d4' }); vi.mocked(gitClient.log).mockResolvedValue([commit]); - vi.mocked(gitClient.getFilesChanged).mockResolvedValue(['src/auth.ts']); + vi.mocked(gitClient.getFilesChanged).mockResolvedValue(new Map([['abc12345', ['src/auth.ts']]])); const gitLogArgs = makeGitLogArgs(); const options = makeQueryOptions(); @@ -171,7 +178,7 @@ describe('AtomRepository', () => { }; vi.mocked(gitClient.log).mockResolvedValue([loreCommit, nonLoreCommit]); - vi.mocked(gitClient.getFilesChanged).mockResolvedValue(['src/auth.ts']); + vi.mocked(gitClient.getFilesChanged).mockResolvedValue(new Map([['abc12345', ['src/auth.ts']]])); const result = await repo.findByTarget(makeGitLogArgs(), makeQueryOptions()); @@ -221,7 +228,7 @@ describe('AtomRepository', () => { const commit1 = makeLoreCommit({ hash: 'aaa', author: 'alice@example.com', loreId: 'aaaa1111' }); const commit2 = makeLoreCommit({ hash: 'bbb', author: 'bob@example.com', loreId: 'bbbb2222' }); vi.mocked(gitClient.log).mockResolvedValue([commit1, commit2]); - vi.mocked(gitClient.getFilesChanged).mockResolvedValue(['src/auth.ts']); + vi.mocked(gitClient.getFilesChanged).mockResolvedValue(new Map([['aaa', ['src/auth.ts']], ['bbb', ['src/auth.ts']]])); const options = makeQueryOptions({ author: 'alice' }); const result = await repo.findByTarget(makeGitLogArgs(), options); @@ -237,7 +244,7 @@ describe('AtomRepository', () => { makeLoreCommit({ hash: 'ccc', loreId: 'cccc3333' }), ]; vi.mocked(gitClient.log).mockResolvedValue(commits); - vi.mocked(gitClient.getFilesChanged).mockResolvedValue(['src/auth.ts']); + vi.mocked(gitClient.getFilesChanged).mockResolvedValue(new Map(commits.map(c => [c.hash, ['src/auth.ts']]))); const options = makeQueryOptions({ limit: 2 }); const result = await repo.findByTarget(makeGitLogArgs(), options); @@ -250,7 +257,7 @@ describe('AtomRepository', () => { it('should find an atom by its Lore-id', async () => { const commit = makeLoreCommit({ loreId: 'deadbeef' }); vi.mocked(gitClient.log).mockResolvedValue([commit]); - vi.mocked(gitClient.getFilesChanged).mockResolvedValue(['src/auth.ts']); + vi.mocked(gitClient.getFilesChanged).mockResolvedValue(new Map([[commit.hash, ['src/auth.ts']]])); const result = await repo.findByLoreId('deadbeef'); @@ -261,7 +268,7 @@ describe('AtomRepository', () => { it('should return null if no atom matches the Lore-id', async () => { const commit = makeLoreCommit({ loreId: 'a1b2c3d4' }); vi.mocked(gitClient.log).mockResolvedValue([commit]); - vi.mocked(gitClient.getFilesChanged).mockResolvedValue([]); + vi.mocked(gitClient.getFilesChanged).mockResolvedValue(new Map()); const result = await repo.findByLoreId('deadbeef'); @@ -289,7 +296,7 @@ describe('AtomRepository', () => { makeLoreCommit({ hash: 'bbb', loreId: 'bbbb2222' }), ]; vi.mocked(gitClient.log).mockResolvedValue(commits); - vi.mocked(gitClient.getFilesChanged).mockResolvedValue([]); + vi.mocked(gitClient.getFilesChanged).mockResolvedValue(new Map([['aaa', []], ['bbb', []]])); const result = await repo.findAll(); @@ -307,7 +314,7 @@ describe('AtomRepository', () => { trailers: trailersRaw, }; vi.mocked(gitClient.log).mockResolvedValue([commit]); - vi.mocked(gitClient.getFilesChanged).mockResolvedValue([]); + vi.mocked(gitClient.getFilesChanged).mockResolvedValue(new Map([['aaa', []]])); const result = await repo.findAll(); @@ -364,7 +371,7 @@ describe('AtomRepository', () => { const authCommit = makeLoreCommit({ subject: 'feat(auth): add login', loreId: 'aaaa1111' }); const dbCommit = makeLoreCommit({ subject: 'fix(database): fix query', loreId: 'bbbb2222' }); vi.mocked(gitClient.log).mockResolvedValue([authCommit, dbCommit]); - vi.mocked(gitClient.getFilesChanged).mockResolvedValue([]); + vi.mocked(gitClient.getFilesChanged).mockResolvedValue(new Map([[authCommit.hash, []], [dbCommit.hash, []]])); const result = await repo.findByScope('auth', makeQueryOptions()); @@ -375,7 +382,7 @@ describe('AtomRepository', () => { it('should match scope case-insensitively', async () => { const commit = makeLoreCommit({ subject: 'feat(Auth): add login', loreId: 'aaaa1111' }); vi.mocked(gitClient.log).mockResolvedValue([commit]); - vi.mocked(gitClient.getFilesChanged).mockResolvedValue([]); + vi.mocked(gitClient.getFilesChanged).mockResolvedValue(new Map([[commit.hash, []]])); const result = await repo.findByScope('auth', makeQueryOptions()); @@ -385,7 +392,7 @@ describe('AtomRepository', () => { it('should return empty array when no scope matches', async () => { const commit = makeLoreCommit({ subject: 'feat(auth): add login', loreId: 'aaaa1111' }); vi.mocked(gitClient.log).mockResolvedValue([commit]); - vi.mocked(gitClient.getFilesChanged).mockResolvedValue([]); + vi.mocked(gitClient.getFilesChanged).mockResolvedValue(new Map([[commit.hash, []]])); const result = await repo.findByScope('payments', makeQueryOptions()); @@ -395,7 +402,7 @@ describe('AtomRepository', () => { it('should handle commits without scope in subject', async () => { const commit = makeLoreCommit({ subject: 'fix: typo', loreId: 'aaaa1111' }); vi.mocked(gitClient.log).mockResolvedValue([commit]); - vi.mocked(gitClient.getFilesChanged).mockResolvedValue([]); + vi.mocked(gitClient.getFilesChanged).mockResolvedValue(new Map([[commit.hash, []]])); const result = await repo.findByScope('auth', makeQueryOptions()); @@ -413,7 +420,7 @@ describe('AtomRepository', () => { // First call for initial atoms, second call for findByLoreId vi.mocked(gitClient.log).mockResolvedValue([commit1, commit2]); - vi.mocked(gitClient.getFilesChanged).mockResolvedValue([]); + vi.mocked(gitClient.getFilesChanged).mockResolvedValue(new Map([['aaa', []], ['bbb', []]])); // Parse the initial atoms ourselves const initialAtoms = [{ @@ -518,7 +525,7 @@ describe('AtomRepository', () => { const commitB = makeLoreCommit({ hash: 'bbb', loreId: 'bbbb2222', trailerExtras: 'Related: aaaa1111' }); vi.mocked(gitClient.log).mockResolvedValue([commitA, commitB]); - vi.mocked(gitClient.getFilesChanged).mockResolvedValue([]); + vi.mocked(gitClient.getFilesChanged).mockResolvedValue(new Map([['aaa', []], ['bbb', []]])); const atomA = { loreId: 'aaaa1111', @@ -558,7 +565,7 @@ describe('AtomRepository', () => { // findByLoreId will search all commits vi.mocked(gitClient.log).mockResolvedValue([commitB, commitC, commitD]); - vi.mocked(gitClient.getFilesChanged).mockResolvedValue([]); + vi.mocked(gitClient.getFilesChanged).mockResolvedValue(new Map([['bbb', []], ['ccc', []], ['ddd', []]])); const atomA = { loreId: 'aaaa1111', @@ -635,25 +642,28 @@ describe('AtomRepository', () => { trailers: '', }; vi.mocked(gitClient.log).mockResolvedValue([loreCommit, nonLoreCommit]); - vi.mocked(gitClient.getFilesChanged).mockResolvedValue(['src/auth.ts']); + vi.mocked(gitClient.getFilesChanged).mockResolvedValue(new Map([['abc12345', ['src/auth.ts']]])); await repo.findByTarget(makeGitLogArgs(), makeQueryOptions()); - expect(gitClient.getFilesChanged).toHaveBeenCalledTimes(1); - expect(gitClient.getFilesChanged).toHaveBeenCalledWith('abc12345'); + expect(gitClient.getFilesChanged).toHaveBeenCalledWith(['abc12345']); }); - it('should handle more commits than batch size', async () => { + it('should handle many commits in a single batch', async () => { const commits = Array.from({ length: 25 }, (_, i) => makeLoreCommit({ hash: `hash${i}`, loreId: `${String(i).padStart(8, '0')}` }), ); vi.mocked(gitClient.log).mockResolvedValue(commits); - vi.mocked(gitClient.getFilesChanged).mockResolvedValue(['file.ts']); + + const filesMap = new Map(); + commits.forEach(c => filesMap.set(c.hash, ['file.ts'])); + vi.mocked(gitClient.getFilesChanged).mockResolvedValue(filesMap); const result = await repo.findByTarget(makeGitLogArgs(), makeQueryOptions()); expect(result).toHaveLength(25); - expect(gitClient.getFilesChanged).toHaveBeenCalledTimes(25); + expect(gitClient.getFilesChanged).toHaveBeenCalledTimes(1); + expect(vi.mocked(gitClient.getFilesChanged).mock.calls[0][0]).toHaveLength(25); }); it('should propagate getFilesChanged errors', async () => { @@ -661,9 +671,7 @@ describe('AtomRepository', () => { vi.mocked(gitClient.log).mockResolvedValue([commit]); vi.mocked(gitClient.getFilesChanged).mockRejectedValue(new Error('git failed')); - await expect( - repo.findByTarget(makeGitLogArgs(), makeQueryOptions()), - ).rejects.toThrow('git failed'); + await expect(repo.findByTarget(makeGitLogArgs(), makeQueryOptions())).rejects.toThrow('git failed'); }); }); }); diff --git a/tests/unit/services/git-client.test.ts b/tests/unit/services/git-client.test.ts new file mode 100644 index 00000000..6d2fb89e --- /dev/null +++ b/tests/unit/services/git-client.test.ts @@ -0,0 +1,61 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { GitClient } from '../../../src/services/git-client.js'; +import { execFile as execFileCb } from 'node:child_process'; + +vi.mock('node:util', async () => { + const actual = await vi.importActual('node:util'); + return { + ...actual, + promisify: (fn: any) => fn, // Simplified mock + }; +}); + +vi.mock('node:child_process', () => ({ + execFile: vi.fn(), +})); + +describe('GitClient', () => { + const client = new GitClient('/test/cwd'); + + beforeEach(() => { + vi.resetAllMocks(); + }); + + describe('getFilesChanged', () => { + it('should correctly parse git diff-tree output', async () => { + const hashes = ['aaaa111122223333444455556666777788889999', 'bbbb111122223333444455556666777788889999']; + const mockOutput = `${hashes[0]}\nfile1.ts\nfile2.ts\n${hashes[1]}\nfile3.ts\n`; + + vi.mocked(execFileCb).mockImplementation(((cmd: string, args: any, opts: any, callback: any) => { + callback(null, mockOutput, ''); + }) as any); + + const result = await client.getFilesChanged(hashes); + + expect(result.get(hashes[0])).toEqual(['file1.ts', 'file2.ts']); + expect(result.get(hashes[1])).toEqual(['file3.ts']); + }); + + it('should be robust against file paths that look like hashes', async () => { + const hashes = ['aaaa111122223333444455556666777788889999']; + // A file path that is exactly 40 chars and hexadecimal + const sneakyPath = '1234567890123456789012345678901234567890'; + const mockOutput = `${hashes[0]}\n${sneakyPath}\nfile.ts\n`; + + vi.mocked(execFileCb).mockImplementation(((cmd: string, args: any, opts: any, callback: any) => { + callback(null, mockOutput, ''); + }) as any); + + const result = await client.getFilesChanged(hashes); + + // The sneaky path should be treated as a file, not a new commit + expect(result.get(hashes[0])).toEqual([sneakyPath, 'file.ts']); + }); + + it('should return an empty map for empty input', async () => { + const result = await client.getFilesChanged([]); + expect(result.size).toBe(0); + expect(execFileCb).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/tests/unit/services/head-lore-id-reader.test.ts b/tests/unit/services/head-lore-id-reader.test.ts index b3b91844..c0f73801 100644 --- a/tests/unit/services/head-lore-id-reader.test.ts +++ b/tests/unit/services/head-lore-id-reader.test.ts @@ -11,7 +11,7 @@ function createMockGitClient(headMessage: string): IGitClient { hasStagedChanges: vi.fn(), getRepoRoot: vi.fn(), isInsideRepo: vi.fn(), - getFilesChanged: vi.fn(), + getFilesChanged: vi.fn().mockResolvedValue(new Map()), countCommitsSince: vi.fn(), resolveRef: vi.fn(), getHeadMessage: vi.fn().mockResolvedValue(headMessage), diff --git a/tests/unit/services/staleness-detector.test.ts b/tests/unit/services/staleness-detector.test.ts index 1f10867a..ec5fd4d0 100644 --- a/tests/unit/services/staleness-detector.test.ts +++ b/tests/unit/services/staleness-detector.test.ts @@ -13,7 +13,7 @@ function createMockGitClient(overrides: Partial = {}): IGitClient { hasStagedChanges: vi.fn(async () => false), getRepoRoot: vi.fn(async () => '/repo'), isInsideRepo: vi.fn(async () => true), - getFilesChanged: vi.fn(async () => []), + getFilesChanged: vi.fn(async () => new Map()), countCommitsSince: vi.fn(async () => 0), resolveRef: vi.fn(async () => 'abc123'), ...overrides,