Skip to content

Commit d4d856e

Browse files
johannesjojohannesjo
committed
fix(diff): align changed line counts with working tree
1 parent b10447a commit d4d856e

2 files changed

Lines changed: 154 additions & 73 deletions

File tree

‎electron/ipc/git.test.ts‎

Lines changed: 123 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ vi.mock('fs', async () => {
3131
const actual = await vi.importActual<typeof import('fs')>('fs');
3232
const mockStat = vi.fn().mockRejectedValue(new Error('ENOENT'));
3333
const mockReadFile = vi.fn().mockRejectedValue(new Error('ENOENT'));
34+
const mockOpen = vi.fn().mockRejectedValue(new Error('ENOENT'));
3435
const mockRealpath = vi.fn().mockImplementation((p: string) => Promise.resolve(p));
3536
return {
3637
...actual,
@@ -40,6 +41,7 @@ vi.mock('fs', async () => {
4041
...actual.promises,
4142
stat: mockStat,
4243
readFile: mockReadFile,
44+
open: mockOpen,
4345
realpath: mockRealpath,
4446
},
4547
},
@@ -437,6 +439,7 @@ function uniqueWorktreePath(): string {
437439
*/
438440
function buildWorktreeMockHandler(opts: {
439441
mergeBase?: string;
442+
finalRawNumstat?: string;
440443
committedRawNumstat?: string;
441444
uncommittedRawNumstat?: string;
442445
untrackedFiles?: string;
@@ -454,6 +457,7 @@ function buildWorktreeMockHandler(opts: {
454457
refinedRangeCount?: string;
455458
}): MockHandler {
456459
const mergeBase = opts.mergeBase ?? MERGE_BASE;
460+
let singlePointRawNumstatCalls = 0;
457461

458462
return (args, cb) => {
459463
const cmd = args[0];
@@ -520,6 +524,23 @@ function buildWorktreeMockHandler(opts: {
520524
return;
521525
}
522526

527+
// diff --raw --numstat <baseSha> — final working tree changes against base.
528+
// In getChangedFiles this is the first single-point raw+numstat diff;
529+
// the later HEAD single-point raw+numstat diff detects local dirtiness.
530+
if (
531+
cmd === 'diff' &&
532+
args.includes('--raw') &&
533+
args.includes('--numstat') &&
534+
!args.some((arg) => arg.includes('...')) &&
535+
singlePointRawNumstatCalls++ === 0
536+
) {
537+
const fallback = [opts.committedRawNumstat, opts.uncommittedRawNumstat]
538+
.filter((part) => part && part.length > 0)
539+
.join('\n');
540+
cb(null, opts.finalRawNumstat ?? fallback, '');
541+
return;
542+
}
543+
523544
// diff --raw --numstat <head> (no mergeBase) — uncommitted changes
524545
if (
525546
cmd === 'diff' &&
@@ -682,6 +703,7 @@ describe('getChangedFiles (worktree-based, merge-base diff)', () => {
682703
setupMock(
683704
calls,
684705
buildWorktreeMockHandler({
706+
finalRawNumstat: rawNumstatEntry('dirty.ts', 5, 1),
685707
committedRawNumstat: rawNumstatEntry('dirty.ts', 4, 1),
686708
uncommittedRawNumstat: rawNumstatEntry('dirty.ts', 1, 0),
687709
}),
@@ -692,6 +714,48 @@ describe('getChangedFiles (worktree-based, merge-base diff)', () => {
692714
expect(files[0].committed).toBe(false);
693715
});
694716

717+
it('should report final line counts when committed files also have local changes', async () => {
718+
const calls: string[][] = [];
719+
setupMock(
720+
calls,
721+
buildWorktreeMockHandler({
722+
finalRawNumstat: rawNumstatEntry('dirty.ts', 7, 2),
723+
committedRawNumstat: rawNumstatEntry('dirty.ts', 4, 1),
724+
uncommittedRawNumstat: rawNumstatEntry('dirty.ts', 10, 9),
725+
}),
726+
);
727+
728+
const files = await getChangedFiles(uniqueWorktreePath(), 'main');
729+
730+
expect(files[0].path).toBe('dirty.ts');
731+
expect(files[0].lines_added).toBe(7);
732+
expect(files[0].lines_removed).toBe(2);
733+
expect(files[0].committed).toBe(false);
734+
});
735+
736+
it('should compare changed-file stats from merge-base to the working tree', async () => {
737+
const calls: string[][] = [];
738+
setupMock(
739+
calls,
740+
buildWorktreeMockHandler({
741+
finalRawNumstat: rawNumstatEntry('file.ts', 1, 0),
742+
}),
743+
);
744+
745+
await getChangedFiles(uniqueWorktreePath(), 'main');
746+
747+
const diffCall = calls.find(
748+
(a) =>
749+
a[0] === 'diff' &&
750+
a.includes('--raw') &&
751+
a.includes('--numstat') &&
752+
a.includes(MERGE_BASE),
753+
);
754+
expect(diffCall).toBeDefined();
755+
expect(diffCall).toContain(MERGE_BASE);
756+
expect(diffCall).not.toContain('main...');
757+
});
758+
695759
it('should return empty list when no changes since merge-base', async () => {
696760
const calls: string[][] = [];
697761
setupMock(
@@ -706,7 +770,7 @@ describe('getChangedFiles (worktree-based, merge-base diff)', () => {
706770
expect(files).toEqual([]);
707771
});
708772

709-
it('should use a base-to-head one-way range, not branch-tip-to-base', async () => {
773+
it('should not use a branch-tip-to-base range for changed-file stats', async () => {
710774
const calls: string[][] = [];
711775
setupMock(
712776
calls,
@@ -717,17 +781,17 @@ describe('getChangedFiles (worktree-based, merge-base diff)', () => {
717781

718782
await getChangedFiles(uniqueWorktreePath(), 'main');
719783

720-
// The committed diff must be ordered base...head, not head...base.
721-
const diffCall = calls.find(
784+
// Diff-base detection/refinement may inspect base...head ranges, but
785+
// changed-file counts use the picked base SHA against the working tree.
786+
const statsCall = calls.find(
722787
(a) =>
723788
a[0] === 'diff' &&
724789
a.includes('--raw') &&
725790
a.includes('--numstat') &&
726-
a.some((arg) => arg.endsWith(`...${HEAD_HASH}`)),
791+
a.includes(MERGE_BASE),
727792
);
728-
expect(diffCall).toBeDefined();
729-
expect(diffCall).toContain(`main...${HEAD_HASH}`);
730-
expect(diffCall).not.toContain(`${HEAD_HASH}...main`);
793+
expect(statsCall).toBeDefined();
794+
expect(statsCall).not.toContain(`${HEAD_HASH}...main`);
731795
});
732796

733797
it('probes both local and origin refs when both exist', async () => {
@@ -1030,6 +1094,36 @@ describe('getFileDiff (worktree-based, merge-base diff)', () => {
10301094

10311095
expect(result.newContent).toBe('disk content with local edits');
10321096
});
1097+
1098+
it('should not add a phantom line in pseudo-diffs for new files ending in a newline', async () => {
1099+
const calls: string[][] = [];
1100+
setupMock(
1101+
calls,
1102+
buildWorktreeMockHandler({
1103+
showOutputs: {},
1104+
diffOutput: '',
1105+
}),
1106+
);
1107+
1108+
vi.mocked(fs.promises.stat).mockResolvedValueOnce({
1109+
isFile: () => true,
1110+
size: 100,
1111+
} as unknown as Awaited<ReturnType<typeof fs.promises.stat>>);
1112+
vi.mocked(fs.promises.readFile).mockResolvedValueOnce('one\ntwo\n');
1113+
vi.mocked(fs.promises.open).mockResolvedValueOnce({
1114+
read: vi.fn(async (buffer: Buffer) => {
1115+
buffer.write('one');
1116+
return { bytesRead: 3, buffer };
1117+
}),
1118+
close: vi.fn().mockResolvedValue(undefined),
1119+
} as unknown as Awaited<ReturnType<typeof fs.promises.open>>);
1120+
1121+
const result = await getFileDiff(uniqueWorktreePath(), 'new-file.ts', 'main');
1122+
1123+
expect(result.diff).toContain('@@ -0,0 +1,2 @@');
1124+
expect(result.diff).toContain('+one\n+two\n');
1125+
expect(result.diff).not.toContain('+two\n+\n');
1126+
});
10331127
});
10341128

10351129
// ---------------------------------------------------------------------------
@@ -1058,16 +1152,16 @@ describe('refineDiffBaseWithCherryPick (via getChangedFiles)', () => {
10581152

10591153
await getChangedFiles(uniqueWorktreePath(), 'main');
10601154

1061-
// The committed-diff range should now use the refined parent SHA, not main.
1062-
const committedDiffCall = calls.find(
1155+
const statsCall = calls.find(
10631156
(a) =>
10641157
a[0] === 'diff' &&
10651158
a.includes('--raw') &&
10661159
a.includes('--numstat') &&
1067-
a.some((arg) => arg.endsWith(`...${HEAD_HASH}`)),
1160+
a.includes(UNIQUE_PARENT),
10681161
);
1069-
expect(committedDiffCall).toBeDefined();
1070-
expect(committedDiffCall).toContain(`${UNIQUE_PARENT}...${HEAD_HASH}`);
1162+
expect(statsCall).toBeDefined();
1163+
expect(statsCall).toContain(UNIQUE_PARENT);
1164+
expect(statsCall).not.toContain(MERGE_BASE);
10711165
});
10721166

10731167
it('refines correctly with multiple contiguous unique commits', async () => {
@@ -1090,15 +1184,16 @@ describe('refineDiffBaseWithCherryPick (via getChangedFiles)', () => {
10901184

10911185
await getChangedFiles(uniqueWorktreePath(), 'main');
10921186

1093-
const committedDiffCall = calls.find(
1187+
const statsCall = calls.find(
10941188
(a) =>
10951189
a[0] === 'diff' &&
10961190
a.includes('--raw') &&
10971191
a.includes('--numstat') &&
1098-
a.some((arg) => arg.endsWith(`...${HEAD_HASH}`)),
1192+
a.includes(UNIQUE_PARENT),
10991193
);
1100-
expect(committedDiffCall).toBeDefined();
1101-
expect(committedDiffCall).toContain(`${UNIQUE_PARENT}...${HEAD_HASH}`);
1194+
expect(statsCall).toBeDefined();
1195+
expect(statsCall).toContain(UNIQUE_PARENT);
1196+
expect(statsCall).not.toContain(MERGE_BASE);
11021197
});
11031198

11041199
it('falls back to picked base when unique commits are interleaved with patch-equivalent ones', async () => {
@@ -1114,16 +1209,14 @@ describe('refineDiffBaseWithCherryPick (via getChangedFiles)', () => {
11141209

11151210
await getChangedFiles(uniqueWorktreePath(), 'main');
11161211

1117-
const committedDiffCall = calls.find(
1212+
const statsCall = calls.find(
11181213
(a) =>
1119-
a[0] === 'diff' &&
1120-
a.includes('--raw') &&
1121-
a.includes('--numstat') &&
1122-
a.some((arg) => arg.endsWith(`...${HEAD_HASH}`)),
1214+
a[0] === 'diff' && a.includes('--raw') && a.includes('--numstat') && a.includes(MERGE_BASE),
11231215
);
1124-
expect(committedDiffCall).toBeDefined();
1125-
// Falls back to the original picked ref (main), not the refined SHA.
1126-
expect(committedDiffCall).toContain(`main...${HEAD_HASH}`);
1216+
expect(statsCall).toBeDefined();
1217+
// Falls back to the original picked merge-base, not the refined SHA.
1218+
expect(statsCall).toContain(MERGE_BASE);
1219+
expect(statsCall).not.toContain(UNIQUE_PARENT);
11271220
});
11281221

11291222
it('collapses diff range to head...head when branch is fully merged upstream', async () => {
@@ -1138,17 +1231,17 @@ describe('refineDiffBaseWithCherryPick (via getChangedFiles)', () => {
11381231

11391232
await getChangedFiles(uniqueWorktreePath(), 'main');
11401233

1141-
const committedDiffCall = calls.find(
1234+
const statsCall = calls.find(
11421235
(a) =>
11431236
a[0] === 'diff' &&
11441237
a.includes('--raw') &&
11451238
a.includes('--numstat') &&
1146-
a.some((arg) => arg.endsWith(`...${HEAD_HASH}`)),
1239+
!a.some((arg) => arg.includes('...')),
11471240
);
1148-
expect(committedDiffCall).toBeDefined();
1149-
// Diff range collapses to head...head — no patch-equivalent noise.
1150-
expect(committedDiffCall).toContain(`${HEAD_HASH}...${HEAD_HASH}`);
1151-
expect(committedDiffCall).not.toContain(`main...${HEAD_HASH}`);
1241+
expect(statsCall).toBeDefined();
1242+
// Stats anchor collapses to HEAD — no patch-equivalent noise.
1243+
expect(statsCall).toContain(HEAD_HASH);
1244+
expect(statsCall).not.toContain(MERGE_BASE);
11521245

11531246
// Refinement short-circuits: no rev-list count needed.
11541247
const revListCounts = calls.filter((a) => a[0] === 'rev-list' && a.includes('--count'));

0 commit comments

Comments
 (0)