Rework incremental diffing (#499)

* Improve our deepening logic * Use consistent SHA for PR-related operations in CheckoutPrTool * compute `deepenDepth` at more appropriate time * fix stale comment * add comments for `alreadyOnBranch` * ensure before sha is available * small cleanup * computeIncrementalDiff * move the util * improve algorithm * improve algorithm further * get rid of temp result array * add comment * compute incremental diff and updte instructions * add comment * update stale comment * get rid of redundant rev-parse call * improve comment * strenghten the instructions * make diff paths unique
2026-03-27 16:09:13 +00:00
parent 248d11d73d
commit a7b8dcbced
11 changed files with 993 additions and 316 deletions
@@ -0,0 +1,236 @@
+import { describe, expect, it } from "vitest";
+import { postProcessRangeDiff } from "./rangeDiff.ts";
+
+describe("postProcessRangeDiff", () => {
+  it("returns null for identical patches", () => {
+    const input = "1:  abc1234 = 1:  def5678 x";
+    expect(postProcessRangeDiff(input)).toBeNull();
+  });
+
+  it("returns null for empty input", () => {
+    expect(postProcessRangeDiff("")).toBeNull();
+    expect(postProcessRangeDiff("   ")).toBeNull();
+  });
+
+  it("returns null when no changes exist between versions", () => {
+    const input = [
+      "1:  abc1234 ! 1:  def5678 x",
+      "     ## src/file.ts ##",
+      "     @@ src/file.ts",
+      "     +const a = 1;",
+      "     +const b = 2;",
+    ].join("\n");
+    expect(postProcessRangeDiff(input)).toBeNull();
+  });
+
+  it("strips inner diff prefix from content lines", () => {
+    const input = [
+      "1:  abc1234 ! 1:  def5678 x",
+      "     ## src/math.ts ##",
+      "     @@ src/math.ts",
+      "     +  const a = 1;",
+      "    -+  const b = 2;",
+      "    ++  const b = 3;",
+      "     +  const c = 4;",
+    ].join("\n");
+    expect(postProcessRangeDiff(input)).toMatchInlineSnapshot(`
+			" ## src/math.ts ##
+			 @@ src/math.ts
+			   const a = 1;
+			-  const b = 2;
+			+  const b = 3;
+			   const c = 4;"
+		`);
+  });
+
+  it("handles context lines (inner space prefix)", () => {
+    const input = [
+      "1:  abc1234 ! 1:  def5678 x",
+      "     ## src/file.ts ##",
+      "     @@ src/file.ts",
+      "      const base = true;",
+      "    - const old = true;",
+      "    + const new_ = true;",
+      "      const end = true;",
+    ].join("\n");
+    expect(postProcessRangeDiff(input)).toMatchInlineSnapshot(`
+			" ## src/file.ts ##
+			 @@ src/file.ts
+			 const base = true;
+			-const old = true;
+			+const new_ = true;
+			 const end = true;"
+		`);
+  });
+
+  it("trims context lines around changes", () => {
+    const contextBefore = Array.from(
+      { length: 9 },
+      (_, i) => `     +const line${i + 1} = ${i + 1};`
+    );
+    const contextAfter = Array.from(
+      { length: 6 },
+      (_, i) => `     +const line${i + 11} = ${i + 11};`
+    );
+    const input = [
+      "1:  abc1234 ! 1:  def5678 x",
+      "     ## src/large.ts ##",
+      "     @@ src/large.ts",
+      ...contextBefore,
+      "    -+const line10 = 10;",
+      "    ++const line10 = 100;",
+      ...contextAfter,
+    ].join("\n");
+    expect(postProcessRangeDiff(input)).toMatchInlineSnapshot(`
+			" ## src/large.ts ##
+			 @@ src/large.ts
+			...
+			 const line7 = 7;
+			 const line8 = 8;
+			 const line9 = 9;
+			-const line10 = 10;
+			+const line10 = 100;
+			 const line11 = 11;
+			 const line12 = 12;
+			 const line13 = 13;"
+		`);
+  });
+
+  it("handles multiple files", () => {
+    const input = [
+      "1:  abc ! 1:  def x",
+      "     ## src/a.ts ##",
+      "     @@ src/a.ts",
+      "    -+old line a",
+      "    ++new line a",
+      "     ## src/b.ts ##",
+      "     @@ src/b.ts",
+      "     +unchanged",
+      "    -+old line b",
+      "    ++new line b",
+    ].join("\n");
+    expect(postProcessRangeDiff(input)).toMatchInlineSnapshot(`
+			" ## src/a.ts ##
+			 @@ src/a.ts
+			-old line a
+			+new line a
+			 ## src/b.ts ##
+			 @@ src/b.ts
+			 unchanged
+			-old line b
+			+new line b"
+		`);
+  });
+
+  it("handles new file added in new version", () => {
+    const input = [
+      "1:  abc ! 1:  def x",
+      "    +## src/new.ts (new) ##",
+      "    +@@ src/new.ts (new)",
+      "    ++export const x = 1;",
+      "    ++export const y = 2;",
+    ].join("\n");
+    expect(postProcessRangeDiff(input)).toMatchInlineSnapshot(`
+			"+## src/new.ts (new) ##
+			+@@ src/new.ts (new)
+			+export const x = 1;
+			+export const y = 2;"
+		`);
+  });
+
+  it("handles file removed in new version", () => {
+    const input = [
+      "1:  abc ! 1:  def x",
+      "    -## src/old.ts ##",
+      "    -@@ src/old.ts",
+      "    --export const x = 1;",
+    ].join("\n");
+    expect(postProcessRangeDiff(input)).toMatchInlineSnapshot(`
+			"-## src/old.ts ##
+			-@@ src/old.ts
+			-export const x = 1;"
+		`);
+  });
+
+  it("filters out metadata section via context trimming", () => {
+    const input = [
+      "1:  abc ! 1:  def x",
+      "     @@ Metadata",
+      "      Author: Test <test@test.com>",
+      "     ## Commit message ##",
+      "        x",
+      "     ## src/file.ts ##",
+      "     @@ src/file.ts",
+      "     +const a = 1;",
+      "    -+const b = 2;",
+      "    ++const b = 3;",
+      "     +const c = 4;",
+    ].join("\n");
+    expect(postProcessRangeDiff(input)).toMatchInlineSnapshot(`
+			" ## src/file.ts ##
+			 @@ src/file.ts
+			 const a = 1;
+			-const b = 2;
+			+const b = 3;
+			 const c = 4;"
+		`);
+  });
+
+  it("uses custom context line count", () => {
+    const contextBefore = Array.from(
+      { length: 5 },
+      (_, i) => `     +const line${i + 1} = ${i + 1};`
+    );
+    const contextAfter = Array.from(
+      { length: 5 },
+      (_, i) => `     +const line${i + 7} = ${i + 7};`
+    );
+    const input = [
+      "1:  abc1234 ! 1:  def5678 x",
+      "     ## src/file.ts ##",
+      "     @@ src/file.ts",
+      ...contextBefore,
+      "    -+const changed = old;",
+      "    ++const changed = new;",
+      ...contextAfter,
+    ].join("\n");
+    expect(postProcessRangeDiff(input, 1)).toMatchInlineSnapshot(`
+			" ## src/file.ts ##
+			 @@ src/file.ts
+			...
+			 const line5 = 5;
+			-const changed = old;
+			+const changed = new;
+			 const line7 = 7;"
+		`);
+  });
+
+  it("handles two separate change regions in the same file", () => {
+    const middle = Array.from({ length: 10 }, (_, i) => `     +const mid${i + 1} = ${i + 1};`);
+    const input = [
+      "1:  abc ! 1:  def x",
+      "     ## src/file.ts ##",
+      "     @@ src/file.ts",
+      "    -+const first = old;",
+      "    ++const first = new;",
+      ...middle,
+      "    -+const second = old;",
+      "    ++const second = new;",
+    ].join("\n");
+    expect(postProcessRangeDiff(input)).toMatchInlineSnapshot(`
+			" ## src/file.ts ##
+			 @@ src/file.ts
+			-const first = old;
+			+const first = new;
+			 const mid1 = 1;
+			 const mid2 = 2;
+			 const mid3 = 3;
+			...
+			 const mid8 = 8;
+			 const mid9 = 9;
+			 const mid10 = 10;
+			-const second = old;
+			+const second = new;"
+		`);
+  });
+});
@@ -0,0 +1,182 @@
+import { log } from "./cli.ts";
+import { $ } from "./shell.ts";
+
+type ComputeIncrementalDiffParams = {
+  baseBranch: string;
+  beforeSha: string;
+  headSha: string;
+};
+
+/**
+ * computes the incremental diff between two versions of a PR using range-diff
+ * on virtual squash commits created via `git commit-tree`.
+ *
+ * each PR version is squashed into a single synthetic commit (merge-base → tip tree),
+ * then range-diff compares those two single-commit ranges. this:
+ * - isolates each version's net effect (base branch noise eliminated via per-version merge bases)
+ * - avoids commit-matching issues that raw range-diff has with rebases/squashes/reordering
+ * - creates only loose git objects, no branches or refs (unlike temp-branch squash approaches)
+ *
+ * unlike fetchAndFormatPrDiff/formatFilesWithLineNumbers, this output has no line numbers.
+ * range-diff compares *patches* (diffs-of-diffs), not file trees — its hunk headers are
+ * `@@ file.ts` breadcrumbs, not positional `@@ -X,Y +A,B @@` markers. reconstructing
+ * line numbers would require cross-referencing with the v2 diff or content-matching against
+ * file trees, both of which are fragile (duplicate lines, hunk boundary shifts after rebase).
+ * a structured interdiff approach (diff two parsed patches, compare only +/- keys via Myers)
+ * could approximate line numbers but loses semantic precision: range-diff understands patch
+ * structure natively (rename detection, hunk-aware matching, dual-prefix inner/outer changes),
+ * while flat key-sequence comparison can misalign duplicate lines and can't distinguish
+ * "new addition to the PR" from "existing code newly modified by the PR". range-diff is the
+ * right abstraction here — the incremental diff answers "how did the changeset evolve?",
+ * not "where in the file is this?", and forcing positional line numbers onto it would be
+ * semantically misleading.
+ *
+ * alternatives considered:
+ * - plain git diff (two-tree or three-dot): includes base branch changes, no PR isolation
+ * - patch-text diffing (interdiff / diff-of-diffs): fragile, hunk offset noise on rebase
+ * - range-diff on raw commit ranges: confused by commit reorganization across force-pushes
+ */
+export function computeIncrementalDiff(params: ComputeIncrementalDiffParams): string | null {
+  try {
+    // $1=beforeSha, $2=baseBranch, $3=headSha
+    const raw = $(
+      "sh",
+      [
+        "-c",
+        'old_base=$(git merge-base "$1" "origin/$2") && ' +
+          'new_base=$(git merge-base "$3" "origin/$2") && ' +
+          "git range-diff --no-color " +
+          '"$old_base..$(git commit-tree "$1^{tree}" -p "$old_base" -m x)" ' +
+          '"$new_base..$(git commit-tree "$3^{tree}" -p "$new_base" -m x)"',
+        "--",
+        params.beforeSha,
+        params.baseBranch,
+        params.headSha,
+      ],
+      { log: false }
+    );
+
+    return postProcessRangeDiff(raw);
+  } catch (e) {
+    log.debug(`» range-diff failed: ${e instanceof Error ? e.message : String(e)}`);
+    return null;
+  }
+}
+
+function isDiffPrefix(ch: string): boolean {
+  return ch === " " || ch === "+" || ch === "-";
+}
+
+/**
+ * transforms git range-diff output into a clean incremental diff.
+ *
+ * range-diff content lines have two prefix characters:
+ *   1st (outer): range-diff level — space (same in both), + (new only), - (old only)
+ *   2nd (inner): original diff level — space (context), + (added), - (removed)
+ *
+ * stripping the inner prefix produces a standard unified-diff-like output where
+ * +/- means "changed between PR versions" rather than "changed vs base branch".
+ *
+ * uses a streaming approach: a ring buffer of before-context lines is flushed when
+ * a change is hit, then afterCount lines of after-context are emitted directly.
+ * nearest preceding ## / @@ headers are force-included when outside the context window.
+ */
+export function postProcessRangeDiff(raw: string, contextLines = 3): string | null {
+  if (!raw.trim()) return null;
+  if (/^\d+:\s+\w+\s+=\s+\d+:/m.test(raw)) return null;
+
+  type Line = { prefix: string; from: number; to: number; seq: number };
+
+  const beforeBuf: Line[] = [];
+  let lastFileHdr: Line | null = null;
+  let lastHunkHdr: Line | null = null;
+  let fileHdrEmitted = true;
+  let hunkHdrEmitted = true;
+
+  let out = "";
+  let afterRemaining = 0;
+  let lastEmittedSeq = -2;
+  let seq = 0;
+  let hasChanges = false;
+
+  function emit(line: Line) {
+    if (lastEmittedSeq >= 0 && line.seq > lastEmittedSeq + 1) out += (out ? "\n" : "") + "...";
+    out += (out ? "\n" : "") + line.prefix + raw.slice(line.from, line.to);
+    lastEmittedSeq = line.seq;
+    if (lastFileHdr?.seq === line.seq) fileHdrEmitted = true;
+    if (lastHunkHdr?.seq === line.seq) hunkHdrEmitted = true;
+  }
+
+  function flushBefore() {
+    if (lastFileHdr && !fileHdrEmitted) emit(lastFileHdr);
+    if (lastHunkHdr && !hunkHdrEmitted) emit(lastHunkHdr);
+    for (const line of beforeBuf) {
+      if (line.seq > lastEmittedSeq) emit(line);
+    }
+    beforeBuf.length = 0;
+  }
+
+  let cursor = 0;
+  while (cursor < raw.length) {
+    const eol = raw.indexOf("\n", cursor);
+    const lineEnd = eol === -1 ? raw.length : eol;
+
+    if (raw.charCodeAt(cursor) >= 48 && raw.charCodeAt(cursor) <= 57) {
+      cursor = lineEnd + 1;
+      continue;
+    }
+
+    if (lineEnd - cursor >= 5 && raw.startsWith("    ", cursor)) {
+      const prefix = raw[cursor + 4];
+      if (isDiffPrefix(prefix)) {
+        const contentPos = cursor + 5;
+        const isOuterChange = prefix !== " ";
+        let line: Line;
+        let isChange = false;
+
+        if (contentPos >= lineEnd) {
+          line = { prefix, from: lineEnd, to: lineEnd, seq };
+        } else if (isDiffPrefix(raw[contentPos])) {
+          isChange = isOuterChange;
+          line = { prefix, from: contentPos + 1, to: lineEnd, seq };
+        } else {
+          line = { prefix, from: contentPos, to: lineEnd, seq };
+          if (
+            raw.startsWith("## ", contentPos) &&
+            !raw.startsWith("## Commit message", contentPos)
+          ) {
+            lastFileHdr = line;
+            fileHdrEmitted = false;
+            lastHunkHdr = null;
+            hunkHdrEmitted = true;
+          } else if (
+            raw.startsWith("@@", contentPos) &&
+            !raw.startsWith("@@ Metadata", contentPos)
+          ) {
+            lastHunkHdr = line;
+            hunkHdrEmitted = false;
+          }
+        }
+
+        if (isChange) {
+          hasChanges = true;
+          flushBefore();
+          emit(line);
+          afterRemaining = contextLines;
+        } else if (afterRemaining > 0) {
+          emit(line);
+          afterRemaining--;
+        } else {
+          if (beforeBuf.length >= contextLines) beforeBuf.shift();
+          beforeBuf.push(line);
+        }
+
+        seq++;
+      }
+    }
+
+    cursor = lineEnd + 1;
+  }
+
+  return hasChanges ? out : null;
+}
@@ -64,7 +64,6 @@ export type SetupGitParams = GitContext;
 * setup git configuration and authentication for the repository.
 * - configures git identity (user.email, user.name)
 * - sets up authentication via gitToken (minimal contents:write)
- * - for PR events, checks out the PR branch using shared helper
 *
 * gitToken is a minimal-permission token (contents + workflows) used for git operations.
 * it is assumed to be potentially exfiltratable, so it has limited scope.