perf(gitcafe): PR sync makes 2x fewer GitCafe requests for unchanged PRs

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

@@ -1226,43 +1226,70 @@ describe("GitCafePullRequestProvider", () => {
 
   it.effect("recounts summary lines only when the compared commits move", () => {
     const landedBase = "fedcba9876543210fedcba9876543210fedcba98";
+    const mainTip = "0123012301230123012301230123012301230123";
     let comparisonBaseOid = baseOid;
+    let tips = { targetBranch: "main", observedBaseOid: baseOid };
     let pullReads = 0;
+    let listingCut = true;
+    let hunksDown = false;
+    const hunks = (request: SentRequest) => ({
+      version: 4,
+      headOid,
+      comparisonBaseOid,
+      items: (request.body as { paths: ReadonlyArray<string> }).paths.map((path) => ({
+        path,
+        additions: comparisonBaseOid === baseOid ? 3 : 1,
+        deletions: 1,
+      })),
+    });
     const server = fakeGitCafe({
       // A comment moves the version without moving the compared commits.
-      "GET /repos/owner/repo/pulls/7": () => ({ ...pull, version: 4 + pullReads++ }),
-      "GET /repos/owner/repo/pulls/7/changes": () => ({
-        version: 4,
-        headOid,
-        comparisonBaseOid,
-        items: [{ path: "a.ts" }, { path: "b.ts" }],
-        next: null,
-      }),
-      "POST /repos/owner/repo/pulls/7/diff-files": (request: SentRequest) => ({
+      "GET /repos/owner/repo/pulls/7": () => ({ ...pull, ...tips, version: 4 + pullReads++ }),
+      "GET /repos/owner/repo/pulls/7/changes": (request: SentRequest) => ({
         version: 4,
         headOid,
         comparisonBaseOid,
-        items: (request.body as { paths: ReadonlyArray<string> }).paths.map((path) => ({
-          path,
-          additions: comparisonBaseOid === baseOid ? 3 : 1,
-          deletions: 1,
-        })),
+        ...(!listingCut
+          ? { items: [{ path: "a.ts" }, { path: "b.ts" }], next: null }
+          : request.query.has("after")
+            ? { items: [], next: "more" }
+            : { items: [{ path: "a.ts" }], next: "more" }),
       }),
+      get "POST /repos/owner/repo/pulls/7/diff-files"() {
+        return hunksDown
+          ? new Response(encodeJson({ type: "https://cafe.sh/errors/conflict" }), { status: 409 })
+          : hunks;
+      },
     });
-    const hunkReads = () =>
-      server.sent.filter((request) => request.path.endsWith("/diff-files")).length;
+    const reads = (suffix: string) =>
+      server.sent.filter((request) => request.path.endsWith(suffix)).length;
     return Effect.gen(function* () {
       const provider = yield* GitCafePullRequestProvider.make;
+      // A listing that stops before its last page holds nothing for its commits.
+      yield* provider.getChangeRequestSummary!(target);
+      listingCut = false;
+      hunksDown = true;
+      // Unread hunks leave the lines unknown, and are read again next time.
+      const unread = yield* provider.getChangeRequestSummary!(target);
+      assert.strictEqual(unread.additions, undefined);
+      hunksDown = false;
       const first = yield* provider.getChangeRequestSummary!(target);
       const second = yield* provider.getChangeRequestSummary!(target);
       assert.deepInclude(first, { additions: 6, deletions: 2, changedFiles: 2 });
       assert.deepInclude(second, { additions: 6, deletions: 2, changedFiles: 2 });
-      assert.strictEqual(hunkReads(), 1);
-      // A landed parent layer moves the comparison base under the same head.
-      comparisonBaseOid = landedBase;
+      // Neither the head nor the target tip moved, so the file list is not read again.
+      assert.deepStrictEqual([reads("/changes"), reads("/diff-files")], [4, 3]);
+      // The target branch advancing keeps the merge base, so only the file list is read.
+      tips = { ...tips, observedBaseOid: mainTip };
       const third = yield* provider.getChangeRequestSummary!(target);
-      assert.deepInclude(third, { additions: 2, deletions: 2, changedFiles: 2 });
-      assert.strictEqual(hunkReads(), 2);
+      assert.deepInclude(third, { additions: 6, deletions: 2, changedFiles: 2 });
+      assert.deepStrictEqual([reads("/changes"), reads("/diff-files")], [5, 3]);
+      // A landed parent layer retargets the pull and moves the comparison base under the same head.
+      comparisonBaseOid = landedBase;
+      tips = { targetBranch: "trunk", observedBaseOid: mainTip };
+      const fourth = yield* provider.getChangeRequestSummary!(target);
+      assert.deepInclude(fourth, { additions: 2, deletions: 2, changedFiles: 2 });
+      assert.deepStrictEqual([reads("/changes"), reads("/diff-files")], [6, 4]);
     }).pipe(Effect.provide(server.layer));
   });
 

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

@@ -636,11 +636,19 @@ export const make = Effect.gen(function* () {
   // so hunk totals are held per compared pair of commits. The file list that names the pair is
   // cheap; a retarget or a landed parent layer moves the base without moving the head.
   const lineTotals = new Map<string, { readonly additions: number; readonly deletions: number }>();
+  // The compared pair follows from the head and the target tip the pull reports, so while neither
+  // moves the stats are reused without listing the files again.
+  const statsByTips = new Map<
+    string,
+    { readonly changedFiles: number; readonly additions?: number; readonly deletions?: number }
+  >();
+  const remember = <K, V>(cache: Map<K, V>, key: K, value: V) => {
+    if (cache.size >= LINE_STATS_CACHE_SIZE) cache.delete(cache.keys().next().value!);
+    cache.set(key, value);
+  };
 
-  const readLineStats = Effect.fn("GitCafePullRequestProvider.readLineStats")(function* (
-    input: PullRef,
-    version: number,
-  ) {
+  // readLineStats owns the span.
+  const countLines = Effect.fnUntraced(function* (input: PullRef, version: number) {
     const path = yield* pullPath("getChangeRequest", input);
     const files: Array<Json.GitCafeDiffFile> = [];
     let revision: { version: number; headOid: string; baseOid: string } | undefined;
@@ -667,13 +675,23 @@ export const make = Effect.gen(function* () {
       after = batch.next;
       if (after === null || after === previous || batch.items.length === 0) break;
     }
-    if (revision === undefined) return { changedFiles: 0, additions: 0, deletions: 0 };
+    if (revision === undefined)
+      return {
+        headOid: null,
+        complete: true,
+        stats: { changedFiles: 0, additions: 0, deletions: 0 },
+      };
     const key = `${input.host} ${input.repository.toLowerCase()} ${input.number} ${revision.baseOid} ${revision.headOid}`;
     const held = lineTotals.get(key);
-    if (held !== undefined) return { changedFiles: files.length, ...held };
+    const { headOid } = revision;
+    // A listing that stopped before its last page counts only part of the diff, so nothing is held.
+    const listed = after === null;
+    if (held !== undefined)
+      return { headOid, complete: listed, stats: { changedFiles: files.length, ...held } };
+    const tooMany = files.length > LINE_STATS_MAX_FILES;
     const details = files.some((file) => file.additions !== undefined)
       ? Option.some(files)
-      : files.length > LINE_STATS_MAX_FILES
+      : tooMany
         ? Option.none()
         : yield* Effect.forEach(
             chunk(detailablePaths(files), DIFF_FILES_BATCH),
@@ -684,16 +702,33 @@ export const make = Effect.gen(function* () {
             Effect.option,
           );
     // Unread hunks leave the lines unknown rather than zero; the file count still stands.
-    if (Option.isNone(details)) return { changedFiles: files.length };
+    if (Option.isNone(details))
+      return { headOid, complete: listed && tooMany, stats: { changedFiles: files.length } };
     const counted = details.value;
     const totals = {
       additions: counted.reduce((total, file) => total + (file.additions ?? 0), 0),
       deletions: counted.reduce((total, file) => total + (file.deletions ?? 0), 0),
     };
-    if (lineTotals.size >= LINE_STATS_CACHE_SIZE)
-      lineTotals.delete(lineTotals.keys().next().value!);
-    lineTotals.set(key, totals);
-    return { changedFiles: files.length, ...totals };
+    if (listed) remember(lineTotals, key, totals);
+    return { headOid, complete: listed, stats: { changedFiles: files.length, ...totals } };
+  });
+
+  const readLineStats = Effect.fn("GitCafePullRequestProvider.readLineStats")(function* (
+    input: PullRef,
+    pull: Json.GitCafePullDetail,
+  ) {
+    const tipsKey =
+      pull.headOid === null || pull.observedBaseOid == null
+        ? null
+        : `${input.host} ${input.repository.toLowerCase()} ${input.number} ${pull.targetBranch} ${pull.observedBaseOid} ${pull.headOid}`;
+    const heldStats = tipsKey === null ? undefined : statsByTips.get(tipsKey);
+    if (heldStats !== undefined) return heldStats;
+    const stats = yield* countLines(input, pull.version);
+    // A failed hunk read is retried next time rather than held as unknown, and a head that moved
+    // between the two reads would hold another pair's stats.
+    if (tipsKey !== null && stats.complete && stats.headOid === pull.headOid)
+      remember(statsByTips, tipsKey, stats.stats);
+    return stats.stats;
   });
 
   const merge = Effect.fn("GitCafePullRequestProvider.merge")(function* (
@@ -870,7 +905,7 @@ export const make = Effect.gen(function* () {
         const changes =
           pull.headOid === null
             ? null
-            : yield* readLineStats(input, pull.version).pipe(Effect.orElseSucceed(() => null));
+            : yield* readLineStats(input, pull).pipe(Effect.orElseSucceed(() => null));
         // A count that could not be read is absent rather than zero.
         const {
           additions: _additions,
@@ -904,7 +939,7 @@ export const make = Effect.gen(function* () {
               ),
           pull.headOid === null
             ? Effect.succeed(null)
-            : readLineStats(input, pull.version).pipe(Effect.orElseSucceed(() => null)),
+            : readLineStats(input, pull).pipe(Effect.orElseSucceed(() => null)),
           // Reviewers need `pr:review:read`, which a contents-only token lacks.
           readPages("getChangeRequest", input, `${path}/reviewers`, Json.GitCafeActorEntries).pipe(
             // @effect-diagnostics-next-line catchConditionalRefailToCatchIf:off - catchIf widens this paginated result to any in tsc-rs.