Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/interfaces/git-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export interface IGitClient {
hasStagedChanges(): Promise<boolean>;
getRepoRoot(): Promise<string>;
isInsideRepo(): Promise<boolean>;
getFilesChanged(commitHash: string): Promise<readonly string[]>;
getFilesChanged(commitHashes: readonly string[]): Promise<ReadonlyMap<string, readonly string[]>>;
countCommitsSince(path: string, sinceCommitHash: string): Promise<number>;
resolveRef(ref: string): Promise<string>;
getHeadMessage(): Promise<string>;
Expand Down
19 changes: 6 additions & 13 deletions src/services/atom-repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
59 changes: 43 additions & 16 deletions src/services/git-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,15 +122,37 @@ export class GitClient implements IGitClient {
}
}

async getFilesChanged(commitHash: string): Promise<readonly string[]> {
async getFilesChanged(commitHashes: readonly string[]): Promise<ReadonlyMap<string, readonly string[]>> {
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<string, string[]>();
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<number> {
Expand Down Expand Up @@ -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<string> {
try {
const { stdout } = await execFile('git', args as string[], {
private async exec(args: readonly string[], input?: string): Promise<string> {
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`);
}
});
}
}
2 changes: 1 addition & 1 deletion tests/unit/commands/commit-amend.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(''),
Expand Down
60 changes: 34 additions & 26 deletions tests/unit/services/atom-repository.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

/**
Expand Down Expand Up @@ -86,9 +86,16 @@ function createMockGitClient(overrides: Partial<IGitClient> = {}): 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<string, string[]>();
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,
};
}
Expand Down Expand Up @@ -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();
Expand All @@ -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());

Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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');

Expand All @@ -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');

Expand Down Expand Up @@ -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();

Expand All @@ -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();

Expand Down Expand Up @@ -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());

Expand All @@ -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());

Expand All @@ -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());

Expand All @@ -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());

Expand All @@ -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 = [{
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -635,35 +642,36 @@ 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<string, string[]>();
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 () => {
const commit = makeLoreCommit({ loreId: 'deadbeef' });
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');
});
});
});
61 changes: 61 additions & 0 deletions tests/unit/services/git-client.test.ts
Original file line number Diff line number Diff line change
@@ -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();
});
});
});
Loading
Loading