Skip to content

Commit 97dcc4e

Browse files
perf(gitcafe): summaries skip the file list while head and target tip hold still
Linked pull requests are summarized every minute, and each GitCafe summary listed the pull's changed files just to learn the compared commits. The pull itself reports its head and its target branch tip, which fix the merge base, so line stats are now held by those and reused without another request. A failed hunk read is not held, so it is retried on the next summary. Co-Authored-By: Claude Opus 5.5 <[email protected]>
1 parent 08a463e commit 97dcc4e

2 files changed

Lines changed: 84 additions & 32 deletions

File tree

‎packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts‎

Lines changed: 37 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1226,43 +1226,62 @@ describe("GitCafePullRequestProvider", () => {
12261226

12271227
it.effect("recounts summary lines only when the compared commits move", () => {
12281228
const landedBase = "fedcba9876543210fedcba9876543210fedcba98";
1229+
const mainTip = "0123012301230123012301230123012301230123";
12291230
let comparisonBaseOid = baseOid;
1231+
let tips = { targetBranch: "main", observedBaseOid: baseOid };
12301232
let pullReads = 0;
1233+
let hunksDown = true;
1234+
const hunks = (request: SentRequest) => ({
1235+
version: 4,
1236+
headOid,
1237+
comparisonBaseOid,
1238+
items: (request.body as { paths: ReadonlyArray<string> }).paths.map((path) => ({
1239+
path,
1240+
additions: comparisonBaseOid === baseOid ? 3 : 1,
1241+
deletions: 1,
1242+
})),
1243+
});
12311244
const server = fakeGitCafe({
12321245
// A comment moves the version without moving the compared commits.
1233-
"GET /repos/owner/repo/pulls/7": () => ({ ...pull, version: 4 + pullReads++ }),
1246+
"GET /repos/owner/repo/pulls/7": () => ({ ...pull, ...tips, version: 4 + pullReads++ }),
12341247
"GET /repos/owner/repo/pulls/7/changes": () => ({
12351248
version: 4,
12361249
headOid,
12371250
comparisonBaseOid,
12381251
items: [{ path: "a.ts" }, { path: "b.ts" }],
12391252
next: null,
12401253
}),
1241-
"POST /repos/owner/repo/pulls/7/diff-files": (request: SentRequest) => ({
1242-
version: 4,
1243-
headOid,
1244-
comparisonBaseOid,
1245-
items: (request.body as { paths: ReadonlyArray<string> }).paths.map((path) => ({
1246-
path,
1247-
additions: comparisonBaseOid === baseOid ? 3 : 1,
1248-
deletions: 1,
1249-
})),
1250-
}),
1254+
get "POST /repos/owner/repo/pulls/7/diff-files"() {
1255+
return hunksDown
1256+
? new Response(encodeJson({ type: "https://cafe.sh/errors/conflict" }), { status: 409 })
1257+
: hunks;
1258+
},
12511259
});
1252-
const hunkReads = () =>
1253-
server.sent.filter((request) => request.path.endsWith("/diff-files")).length;
1260+
const reads = (suffix: string) =>
1261+
server.sent.filter((request) => request.path.endsWith(suffix)).length;
12541262
return Effect.gen(function* () {
12551263
const provider = yield* GitCafePullRequestProvider.make;
1264+
// Unread hunks leave the lines unknown, and are read again next time.
1265+
const unread = yield* provider.getChangeRequestSummary!(target);
1266+
assert.strictEqual(unread.additions, undefined);
1267+
hunksDown = false;
12561268
const first = yield* provider.getChangeRequestSummary!(target);
12571269
const second = yield* provider.getChangeRequestSummary!(target);
12581270
assert.deepInclude(first, { additions: 6, deletions: 2, changedFiles: 2 });
12591271
assert.deepInclude(second, { additions: 6, deletions: 2, changedFiles: 2 });
1260-
assert.strictEqual(hunkReads(), 1);
1261-
// A landed parent layer moves the comparison base under the same head.
1262-
comparisonBaseOid = landedBase;
1272+
// Neither the head nor the target tip moved, so the file list is not read again.
1273+
assert.deepStrictEqual([reads("/changes"), reads("/diff-files")], [2, 2]);
1274+
// The target branch advancing keeps the merge base, so only the file list is read.
1275+
tips = { ...tips, observedBaseOid: mainTip };
12631276
const third = yield* provider.getChangeRequestSummary!(target);
1264-
assert.deepInclude(third, { additions: 2, deletions: 2, changedFiles: 2 });
1265-
assert.strictEqual(hunkReads(), 2);
1277+
assert.deepInclude(third, { additions: 6, deletions: 2, changedFiles: 2 });
1278+
assert.deepStrictEqual([reads("/changes"), reads("/diff-files")], [3, 2]);
1279+
// A landed parent layer retargets the pull and moves the comparison base under the same head.
1280+
comparisonBaseOid = landedBase;
1281+
tips = { targetBranch: "trunk", observedBaseOid: mainTip };
1282+
const fourth = yield* provider.getChangeRequestSummary!(target);
1283+
assert.deepInclude(fourth, { additions: 2, deletions: 2, changedFiles: 2 });
1284+
assert.deepStrictEqual([reads("/changes"), reads("/diff-files")], [4, 3]);
12661285
}).pipe(Effect.provide(server.layer));
12671286
});
12681287

‎packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts‎

Lines changed: 47 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -636,11 +636,19 @@ export const make = Effect.gen(function* () {
636636
// so hunk totals are held per compared pair of commits. The file list that names the pair is
637637
// cheap; a retarget or a landed parent layer moves the base without moving the head.
638638
const lineTotals = new Map<string, { readonly additions: number; readonly deletions: number }>();
639+
// The compared pair follows from the head and the target tip the pull reports, so while neither
640+
// moves the stats are reused without listing the files again.
641+
const statsByTips = new Map<
642+
string,
643+
{ readonly changedFiles: number; readonly additions?: number; readonly deletions?: number }
644+
>();
645+
const remember = <K, V>(cache: Map<K, V>, key: K, value: V) => {
646+
if (cache.size >= LINE_STATS_CACHE_SIZE) cache.delete(cache.keys().next().value!);
647+
cache.set(key, value);
648+
};
639649

640-
const readLineStats = Effect.fn("GitCafePullRequestProvider.readLineStats")(function* (
641-
input: PullRef,
642-
version: number,
643-
) {
650+
// readLineStats owns the span.
651+
const countLines = Effect.fnUntraced(function* (input: PullRef, version: number) {
644652
const path = yield* pullPath("getChangeRequest", input);
645653
const files: Array<Json.GitCafeDiffFile> = [];
646654
let revision: { version: number; headOid: string; baseOid: string } | undefined;
@@ -667,13 +675,21 @@ export const make = Effect.gen(function* () {
667675
after = batch.next;
668676
if (after === null || after === previous || batch.items.length === 0) break;
669677
}
670-
if (revision === undefined) return { changedFiles: 0, additions: 0, deletions: 0 };
678+
if (revision === undefined)
679+
return {
680+
headOid: null,
681+
complete: true,
682+
stats: { changedFiles: 0, additions: 0, deletions: 0 },
683+
};
671684
const key = `${input.host} ${input.repository.toLowerCase()} ${input.number} ${revision.baseOid} ${revision.headOid}`;
672685
const held = lineTotals.get(key);
673-
if (held !== undefined) return { changedFiles: files.length, ...held };
686+
const { headOid } = revision;
687+
if (held !== undefined)
688+
return { headOid, complete: true, stats: { changedFiles: files.length, ...held } };
689+
const tooMany = files.length > LINE_STATS_MAX_FILES;
674690
const details = files.some((file) => file.additions !== undefined)
675691
? Option.some(files)
676-
: files.length > LINE_STATS_MAX_FILES
692+
: tooMany
677693
? Option.none()
678694
: yield* Effect.forEach(
679695
chunk(detailablePaths(files), DIFF_FILES_BATCH),
@@ -684,16 +700,33 @@ export const make = Effect.gen(function* () {
684700
Effect.option,
685701
);
686702
// Unread hunks leave the lines unknown rather than zero; the file count still stands.
687-
if (Option.isNone(details)) return { changedFiles: files.length };
703+
if (Option.isNone(details))
704+
return { headOid, complete: tooMany, stats: { changedFiles: files.length } };
688705
const counted = details.value;
689706
const totals = {
690707
additions: counted.reduce((total, file) => total + (file.additions ?? 0), 0),
691708
deletions: counted.reduce((total, file) => total + (file.deletions ?? 0), 0),
692709
};
693-
if (lineTotals.size >= LINE_STATS_CACHE_SIZE)
694-
lineTotals.delete(lineTotals.keys().next().value!);
695-
lineTotals.set(key, totals);
696-
return { changedFiles: files.length, ...totals };
710+
remember(lineTotals, key, totals);
711+
return { headOid, complete: true, stats: { changedFiles: files.length, ...totals } };
712+
});
713+
714+
const readLineStats = Effect.fn("GitCafePullRequestProvider.readLineStats")(function* (
715+
input: PullRef,
716+
pull: Json.GitCafePullDetail,
717+
) {
718+
const tipsKey =
719+
pull.headOid === null || pull.observedBaseOid == null
720+
? null
721+
: `${input.host} ${input.repository.toLowerCase()} ${input.number} ${pull.targetBranch} ${pull.observedBaseOid} ${pull.headOid}`;
722+
const heldStats = tipsKey === null ? undefined : statsByTips.get(tipsKey);
723+
if (heldStats !== undefined) return heldStats;
724+
const stats = yield* countLines(input, pull.version);
725+
// A failed hunk read is retried next time rather than held as unknown, and a head that moved
726+
// between the two reads would hold another pair's stats.
727+
if (tipsKey !== null && stats.complete && stats.headOid === pull.headOid)
728+
remember(statsByTips, tipsKey, stats.stats);
729+
return stats.stats;
697730
});
698731

699732
const merge = Effect.fn("GitCafePullRequestProvider.merge")(function* (
@@ -870,7 +903,7 @@ export const make = Effect.gen(function* () {
870903
const changes =
871904
pull.headOid === null
872905
? null
873-
: yield* readLineStats(input, pull.version).pipe(Effect.orElseSucceed(() => null));
906+
: yield* readLineStats(input, pull).pipe(Effect.orElseSucceed(() => null));
874907
// A count that could not be read is absent rather than zero.
875908
const {
876909
additions: _additions,
@@ -904,7 +937,7 @@ export const make = Effect.gen(function* () {
904937
),
905938
pull.headOid === null
906939
? Effect.succeed(null)
907-
: readLineStats(input, pull.version).pipe(Effect.orElseSucceed(() => null)),
940+
: readLineStats(input, pull).pipe(Effect.orElseSucceed(() => null)),
908941
// Reviewers need `pr:review:read`, which a contents-only token lacks.
909942
readPages("getChangeRequest", input, `${path}/reviewers`, Json.GitCafeActorEntries).pipe(
910943
// @effect-diagnostics-next-line catchConditionalRefailToCatchIf:off - catchIf widens this paginated result to any in tsc-rs.

0 commit comments

Comments
 (0)