diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts index 212f87d2657f9..9404b516195ee 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts @@ -40,6 +40,18 @@ function outputWithin(maxOutputBytes: number | undefined, response: string) { const json = (value: Record) => JSON.stringify(value); +/** The `az` call for a files diff, when the organization does not offer one. */ +const filesDiffRefused = () => + Effect.fail( + new AzureDevOpsCli.AzureDevOpsCommandFailedError({ + operation: "execute", + command: "az", + cwd: "/w", + argumentCount: 1, + cause: "the files diff is not offered here", + }), + ); + const pullRequestRow = { pullRequestId: 42, title: "Add the page", @@ -594,6 +606,7 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { mockedExecute .mockReturnValueOnce(Effect.succeed(output(json(pullRequestRow)))) .mockReturnValueOnce(Effect.succeed(output(json(oneIteration)))) + .mockReturnValueOnce(filesDiffRefused()) .mockReturnValueOnce( Effect.succeed( output( @@ -638,6 +651,7 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { mockedExecute .mockReturnValueOnce(Effect.succeed(output(json(pullRequestRow)))) .mockReturnValueOnce(Effect.succeed(output(json(oneIteration)))) + .mockReturnValueOnce(filesDiffRefused()) .mockReturnValueOnce( Effect.succeed( output( @@ -704,6 +718,7 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { ), ), ) + .mockReturnValueOnce(filesDiffRefused()) .mockReturnValueOnce( Effect.succeed( output( @@ -729,7 +744,7 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { number: 42, }); - expect(argsOfCall(3)).toContain("includeContentMetadata=true"); + expect(argsOfCall(4)).toContain("includeContentMetadata=true"); expect(slice.patch).toContain("Binary files a/logo.png and b/logo.png differ"); assert.isTrue(slice.truncated); }), @@ -868,6 +883,101 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { }), ); + it.effect("asks for a page of files with their diffs through the preview route", () => + Effect.gen(function* () { + mockedExecute.mockReturnValueOnce( + Effect.succeed( + output( + // `to_string(@)`: the page's JSON as a string. + JSON.stringify( + json({ + fileDiffs: [{ path: "/a.ts", originalPath: "/a.ts", changeType: "edit" }], + }), + ), + ), + ), + ); + const cli = yield* AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli; + + const files = yield* cli.listFilesDiff({ + cwd: "/w", + location: { project: "platform", repository: "web" }, + number: 42, + top: 50, + skip: 100, + }); + + assert.deepStrictEqual(argsOfCall(0), [ + "devops", + "invoke", + "--detect", + "true", + "--area", + "git", + "--resource", + "pullRequestFilesDiff", + "--api-version", + "7.2-preview", + "--route-parameters", + "project=platform", + "repositoryId=web", + "pullRequestId=42", + "--query-parameters", + "$top=50", + "$skip=100", + "--query", + "to_string(@)", + "--only-show-errors", + "--output", + "json", + ]); + assert.deepStrictEqual( + files.map((file) => file.change.path), + ["a.ts"], + ); + }), + ); + + it.effect("reads a page too large for its output limit as unreadable rather than short", () => + Effect.gen(function* () { + const response = JSON.stringify( + json({ + fileDiffs: Array.from({ length: 50 }, (_, index) => ({ + path: `/file-${index}.ts`, + originalPath: `/file-${index}.ts`, + changeType: "add", + lineDiffBlocks: [ + { + changeType: "add", + originalLineNumberStart: 0, + originalLinesCount: 0, + modifiedLineNumberStart: 1, + modifiedLinesCount: 1, + modifiedLines: ["x".repeat(400_000)], + }, + ], + })), + }), + ); + mockedExecute.mockImplementationOnce((input) => + Effect.succeed(outputWithin(input.maxOutputBytes, response)), + ); + const cli = yield* AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli; + + const error = yield* Effect.flip( + cli.listFilesDiff({ + cwd: "/w", + location: { project: "platform", repository: "web" }, + number: 42, + top: 50, + skip: 0, + }), + ); + + assert.strictEqual(error._tag, "AzureDevOpsPullRequestReadError"); + }), + ); + it.effect("reports a pull request it cannot place as its own outcome", () => Effect.gen(function* () { mockedExecute.mockReturnValueOnce( @@ -924,8 +1034,27 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { }); expect(statuses.map((status) => status.description)).toEqual(["Build"]); - // A status carries every identity Azure knows of it, which is most of a large response. - expect(argsOfCall(0)).toContain("--query"); + expect(argsOfCall(0)).toEqual([ + "devops", + "invoke", + "--detect", + "true", + "--area", + "git", + "--resource", + "pullRequestStatuses", + "--api-version", + "7.1", + "--route-parameters", + "project=platform", + "repositoryId=web", + "pullRequestId=42", + "--query", + "{value:value[].{id:id,state:state,description:description,targetUrl:targetUrl,context:context}}", + "--only-show-errors", + "--output", + "json", + ]); }), ); diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.ts index 435d2bb5f89b5..5e2fb045d7b7e 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.ts @@ -13,6 +13,7 @@ import type { import * as AzureDevOpsCli from "./AzureDevOpsCli.ts"; import { + decodeFilesDiffJson, decodeItemContentJson, decodeIterationChangesJson, decodeIterationsJson, @@ -23,6 +24,7 @@ import { decodeThreadsJson, decodeViewerJson, type AzureDevOpsChangeEntry, + type AzureDevOpsFileDiff, type AzureDevOpsItemContent, type AzureDevOpsIteration, type AzureDevOpsPolicyEvaluation, @@ -107,7 +109,14 @@ export type AzureDevOpsPullRequestCliError = | AzureDevOpsViewerUnavailableError; const REST_API_VERSION = "7.1"; +/** + * The pull request files diff is registered as a preview. The extension rejects `7.2-preview.1` + * (it parses the version as a float) and Azure rejects a plain `7.2`, so only this spelling works. + */ +const FILES_DIFF_API_VERSION = "7.2-preview"; const PULL_REQUEST_LIST_MAX_OUTPUT_BYTES = 16 * 1024 * 1024; +/** A page of diffs carries line text, which `az` prints with the escapes of a JSON string. */ +const FILES_DIFF_MAX_OUTPUT_BYTES = 16 * 1024 * 1024; /** A 2,000-file page exceeds the default 1 MiB output budget. */ const CHANGE_ENTRIES_MAX_OUTPUT_BYTES = 8 * 1024 * 1024; /** JSON escaping and base64 can exceed the decoded file's size. */ @@ -207,6 +216,15 @@ export class AzureDevOpsPullRequestCli extends Context.Service< readonly commit: string; }) => Effect.Effect; + /** The latest iteration's files and diff blocks in one call, `top` files from `skip`. */ + readonly listFilesDiff: (input: { + readonly cwd: string; + readonly location: AzureDevOpsRepositoryLocation; + readonly number: number; + readonly top: number; + readonly skip: number; + }) => Effect.Effect, AzureDevOpsPullRequestCliError>; + readonly runPullRequestAction: (input: { readonly cwd: string; readonly number: number; @@ -319,11 +337,12 @@ export const make = Effect.gen(function* () { readonly cwd: string; readonly operation: string; readonly resource: string; + readonly apiVersion?: string; readonly routeParameters: ReadonlyArray; readonly queryParameters?: ReadonlyArray; + /** A JMESPath query applied to the result before it is printed. */ + readonly query?: string; readonly maxOutputBytes?: number; - /** A JMESPath projection `az` applies before printing, to keep large responses small. */ - readonly jmesQuery?: string; readonly decode: (raw: string) => Result.Result; }): Effect.Effect => executeJson({ @@ -338,13 +357,13 @@ export const make = Effect.gen(function* () { "--resource", input.resource, "--api-version", - REST_API_VERSION, + input.apiVersion ?? REST_API_VERSION, "--route-parameters", ...input.routeParameters, ...(input.queryParameters === undefined ? [] : ["--query-parameters", ...input.queryParameters]), - ...(input.jmesQuery === undefined ? [] : ["--query", input.jmesQuery]), + ...(input.query === undefined ? [] : ["--query", input.query]), ], }).pipe( Effect.flatMap((result) => { @@ -604,7 +623,7 @@ export const make = Effect.gen(function* () { maxOutputBytes: REVIEW_HISTORY_MAX_OUTPUT_BYTES, // Each status carries its creator's profile and links, several times the fields read // here, which costs `az` seconds to print on a busy pull request. - jmesQuery: PULL_REQUEST_STATUS_FIELDS, + query: PULL_REQUEST_STATUS_FIELDS, decode: decodePullRequestStatusesJson, }); }, @@ -683,6 +702,21 @@ export const make = Effect.gen(function* () { }); }), + listFilesDiff: Effect.fn("AzureDevOpsPullRequestCli.listFilesDiff")(function (input) { + return invoke({ + cwd: input.cwd, + operation: "listFilesDiff", + resource: "pullRequestFilesDiff", + apiVersion: FILES_DIFF_API_VERSION, + routeParameters: pullRequestRoute(input), + queryParameters: [`$top=${input.top}`, `$skip=${input.skip}`], + // The page as a string of JSON text, which `az` prints in ASCII whatever the code page. + query: "to_string(@)", + maxOutputBytes: FILES_DIFF_MAX_OUTPUT_BYTES, + decode: decodeFilesDiffJson, + }); + }), + setPullRequestReviewers: Effect.fn("AzureDevOpsPullRequestCli.setPullRequestReviewers")( function* (input) { if (input.reviewers.some((reviewer) => !isReviewerName(reviewer))) { diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts index 3ee1dbafb56ce..920b1984d3c63 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts @@ -2,8 +2,10 @@ import { assert, describe, expect, it } from "@effect/vitest"; import * as Effect from "effect/Effect"; import * as Layer from "effect/Layer"; +import * as AzureDevOpsCli from "./AzureDevOpsCli.ts"; import * as AzureDevOpsPullRequestCli from "./AzureDevOpsPullRequestCli.ts"; import { + FILES_DIFF_PAGE_SIZE, LOCATION_CACHE_CAPACITY, make, MAX_DIFF_SPAWNS, @@ -17,6 +19,7 @@ import { } from "./azureDevOpsDiff.ts"; import type { AzureDevOpsChangeEntry, + AzureDevOpsFileDiff, AzureDevOpsPolicyEvaluation, } from "./azureDevOpsPullRequestJson.ts"; @@ -42,6 +45,16 @@ const PULL_REQUEST = { autoMergeEnabled: false, }; +/** Azure could not give a files diff, so a walk reads the iteration changes as it always did. */ +const filesDiffUnavailable = Effect.fail( + new AzureDevOpsPullRequestCli.AzureDevOpsPullRequestReadError({ + command: "az", + cwd: "/w", + operation: "listFilesDiff", + cause: "unavailable", + }), +); + const CI_EVALUATION: AzureDevOpsPolicyEvaluation = { status: "approved", configuration: { @@ -93,6 +106,7 @@ const readSlice = (input: { Layer.mock(AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli)({ getPullRequest: () => Effect.succeed(PULL_REQUEST), listIterations: () => Effect.succeed([ITERATION]), + listFilesDiff: () => filesDiffUnavailable, listIterationChanges: () => Effect.succeed({ changes: input.paths.map((path) => @@ -233,6 +247,7 @@ describe("getChangeRequest", () => { return { ...PULL_REQUEST, title: `Read ${reads}` }; }), listIterations: () => Effect.succeed([ITERATION]), + listFilesDiff: () => Effect.succeed([newFile("a.ts", ["x"])]), listIterationChanges: () => Effect.succeed({ changes: [change("a.ts")], truncated: false }), readItemContent: () => @@ -252,7 +267,8 @@ describe("getChangeRequest", () => { expect(yield* readPullRequest(input)).toMatchObject( method === "getChangeRequestChecks" ? { state: "open", checks: [] } : { title: "Read 1" }, ); - yield* provider.getDiff(input); + const slice = yield* provider.getDiff(input); + expect(patchedPaths(slice.patch)).toEqual(["a.ts"]); yield* provider.getDiffFileContents({ ...input, oldPath: "a.ts", @@ -375,6 +391,7 @@ describe("getDiff reads", () => { Layer.mock(AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli)({ getPullRequest: () => Effect.succeed(PULL_REQUEST), listIterations: () => Effect.succeed([ITERATION]), + listFilesDiff: () => filesDiffUnavailable, listIterationChanges: () => Effect.succeed({ changes: paths.map((path) => change(path)), truncated: false }), readItemContent: () => @@ -502,6 +519,7 @@ describe("what one diff slice spends", () => { return { ...PULL_REQUEST, number: input.number }; }), listIterations: () => Effect.succeed([ITERATION]), + listFilesDiff: () => filesDiffUnavailable, listIterationChanges: () => Effect.succeed({ changes: [change("a.ts")], truncated: false }), readItemContent: () => @@ -537,3 +555,299 @@ describe("what one diff slice spends", () => { }), ); }); + +/** A file Azure lists as new, with the blocks it gives for text that ends in a newline. */ +function newFile(path: string, lines: ReadonlyArray): AzureDevOpsFileDiff { + return { + change: { path, oldPath: path, changeKind: "new", objectId: null, originalObjectId: null }, + blocks: [ + { + kind: "add", + oldStart: 0, + oldCount: 0, + newStart: 1, + newCount: lines.length + 1, + oldLines: [], + newLines: [...lines, ""], + }, + ], + }; +} + +/** Azure lists the files of each page by path, whatever order the change holds them in. */ +const orderedByPath = (files: ReadonlyArray) => + files.toSorted((left, right) => (left.change.path < right.change.path ? -1 : 1)); + +const lightFiles = (count: number) => + Array.from({ length: count }, (_, at) => + newFile(`src/file-${String(at).padStart(4, "0")}.ts`, ["x", "y"]), + ); + +/** A provider whose files diff answers from `files`, a page at a time, as Azure does. */ +const filesDiffProvider = (input: { + readonly files: ReadonlyArray; + readonly failure?: Effect.Effect; + readonly iterations?: ReadonlyArray; +}) => + Effect.gen(function* () { + const asks: number[] = []; + const reads: string[] = []; + const counts = { iterationChanges: 0 }; + const provider = yield* make.pipe( + Effect.provide( + Layer.mock(AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli)({ + getPullRequest: () => Effect.succeed(PULL_REQUEST), + listIterations: () => Effect.succeed(input.iterations ?? [ITERATION]), + listFilesDiff: ({ skip, top }) => { + asks.push(skip); + if (input.failure !== undefined) return input.failure; + return Effect.succeed(orderedByPath(input.files.slice(skip, skip + top))); + }, + listIterationChanges: () => { + counts.iterationChanges += 1; + return Effect.succeed({ + changes: input.files.map((file) => file.change), + truncated: false, + }); + }, + readItemContent: (item) => { + reads.push(item.path); + const isOldSide = item.commit !== ITERATION.headCommit; + return Effect.succeed({ + contents: side(isOldSide ? "old" : "new", 2, 4), + isBinary: false, + }); + }, + }), + ), + ); + const diff = (cursor?: string) => + provider.getDiff({ + cwd: "/w", + repository: "acme/web", + host: "dev.azure.com", + number: 7, + ...(cursor === undefined ? {} : { cursor }), + }); + return { diff, asks, reads, counts }; + }); + +describe("getDiff from the files diff", () => { + it.effect("reads a page of files with one call and no file reads", () => + Effect.gen(function* () { + const files = lightFiles(3); + const read = yield* filesDiffProvider({ files }); + + const slice = yield* read.diff(); + + expect(patchedPaths(slice.patch)).toEqual(files.map((file) => file.change.path)); + expect(slice.patch).toContain("@@ -0,0 +1,2 @@\n+x\n+y\n"); + expect(slice.truncated).toBe(false); + expect(slice.nextCursor).toBeNull(); + expect(read.asks).toEqual([0]); + expect(read.reads).toEqual([]); + expect(read.counts.iterationChanges).toBe(0); + }), + ); + + it.effect.each([{ count: 2 * FILES_DIFF_PAGE_SIZE }, { count: 2 * FILES_DIFF_PAGE_SIZE + 1 }])( + "walks a list of $count files a page to a slice, ending without calling it truncated", + ({ count }) => + Effect.gen(function* () { + const files = lightFiles(count); + const read = yield* filesDiffProvider({ files }); + + const paths: string[] = []; + let cursor: string | undefined; + do { + const slice = yield* read.diff(cursor); + const slicePaths = patchedPaths(slice.patch); + expect(slicePaths).toHaveLength(Math.min(FILES_DIFF_PAGE_SIZE, count - paths.length)); + paths.push(...slicePaths); + expect(slice.truncated).toBe(false); + cursor = slice.nextCursor ?? undefined; + } while (cursor !== undefined); + + expect(paths).toEqual(files.map((file) => file.change.path)); + expect(read.asks).toEqual([0, FILES_DIFF_PAGE_SIZE, 2 * FILES_DIFF_PAGE_SIZE]); + expect(read.reads).toEqual([]); + }), + ); + + it.effect("reads a page once for the slices that stop inside it", () => + Effect.gen(function* () { + const lines = Array.from({ length: 100 }, (_, line) => `${line} ${"z".repeat(1_300)}`); + const files = Array.from({ length: 60 }, (_, at) => + newFile(`src/file-${String(at).padStart(4, "0")}.ts`, lines), + ); + const read = yield* filesDiffProvider({ files }); + + const paths: string[] = []; + let slices = 0; + let cursor: string | undefined; + let lastCursor: string | undefined; + do { + lastCursor = cursor; + const slice = yield* read.diff(cursor); + paths.push(...patchedPaths(slice.patch)); + cursor = slice.nextCursor ?? undefined; + slices += 1; + } while (cursor !== undefined); + + expect(paths).toEqual(files.map((file) => file.change.path)); + expect(slices).toBeGreaterThan(2); + expect(read.asks).toEqual([0, FILES_DIFF_PAGE_SIZE]); + const lastSlice = yield* read.diff(lastCursor); + expect(lastSlice.nextCursor).toBeNull(); + expect(read.asks).toEqual([0, FILES_DIFF_PAGE_SIZE]); + }), + ); + + it.effect("resumes inside a page from the page's start, where Azure's order for it holds", () => + Effect.gen(function* () { + const files = lightFiles(2 * FILES_DIFF_PAGE_SIZE).toReversed(); + const read = yield* filesDiffProvider({ files }); + const page = orderedByPath(files.slice(0, FILES_DIFF_PAGE_SIZE)); + + const slice = yield* read.diff(`${ITERATION.id}:17:d`); + + expect(patchedPaths(slice.patch)).toEqual(page.slice(17).map((file) => file.change.path)); + expect(slice.nextCursor).toBe(`${ITERATION.id}:${FILES_DIFF_PAGE_SIZE}:d`); + expect(read.asks).toEqual([0]); + }), + ); + + it.effect("carries nothing of a walk whose pull request took another push", () => + Effect.gen(function* () { + const read = yield* filesDiffProvider({ + files: lightFiles(3), + iterations: [{ ...ITERATION, id: ITERATION.id + 1 }], + }); + + const slice = yield* read.diff(`${ITERATION.id}:2:d`); + + expect(slice).toEqual({ patch: "", truncated: true, nextCursor: null }); + }), + ); + + it.effect("finishes the held iteration after a push and truncates at the next page", () => + Effect.gen(function* () { + const iterations = [ITERATION]; + const lines = ["z".repeat(MAX_DIFF_SLICE_BYTES)]; + const files = Array.from({ length: FILES_DIFF_PAGE_SIZE + 1 }, (_, at) => + newFile(`src/file-${String(at).padStart(4, "0")}.ts`, lines), + ); + const read = yield* filesDiffProvider({ files, iterations }); + const first = yield* read.diff(); + expect(first.nextCursor).toBe(`${ITERATION.id}:1:d`); + iterations.push({ ...ITERATION, id: ITERATION.id + 1, headCommit: "new-head" }); + + const held = yield* read.diff(first.nextCursor ?? undefined); + expect(patchedPaths(held.patch)).toEqual(["src/file-0001.ts"]); + expect(held.nextCursor).toBe(`${ITERATION.id}:2:d`); + expect(held.truncated).toBe(false); + expect(read.asks).toEqual([0]); + + const nextPage = yield* read.diff(`${ITERATION.id}:${FILES_DIFF_PAGE_SIZE}:d`); + expect(nextPage).toEqual({ patch: "", truncated: true, nextCursor: null }); + expect(read.asks).toEqual([0, FILES_DIFF_PAGE_SIZE]); + }), + ); + + it.effect("reads the long way a file whose blocks it cannot trust", () => + Effect.gen(function* () { + const files = [ + newFile("a.ts", ["x"]), + { ...newFile("b.ts", ["x"]), blocks: null }, + newFile("c.ts", ["x"]), + ]; + const read = yield* filesDiffProvider({ files }); + + const slice = yield* read.diff(); + + expect(patchedPaths(slice.patch)).toEqual(["a.ts", "b.ts", "c.ts"]); + expect(new Set(read.reads)).toEqual(new Set(["b.ts"])); + expect(slice.patch.match(/^@@ /gmu)).toHaveLength(3); + }), + ); + + it.effect.each([ + { name: "Azure lists no files", files: [] as ReadonlyArray }, + { + name: "Azure cannot give the files diff", + files: lightFiles(2), + failure: filesDiffUnavailable, + }, + ])("reads the iteration changes when $name", ({ name: _name, ...input }) => + Effect.gen(function* () { + const read = yield* filesDiffProvider(input); + + const slice = yield* read.diff(); + + expect(read.counts.iterationChanges).toBe(1); + expect(read.asks).toEqual([0]); + expect(slice.nextCursor).toBeNull(); + }), + ); + + it.effect("takes a cursor from before the files diff for the iteration changes", () => + Effect.gen(function* () { + const read = yield* filesDiffProvider({ files: lightFiles(3) }); + + const slice = yield* read.diff(`${ITERATION.id}:1`); + + expect(patchedPaths(slice.patch)).toEqual(["src/file-0001.ts", "src/file-0002.ts"]); + expect(read.asks).toEqual([]); + expect(read.counts.iterationChanges).toBe(1); + }), + ); + + it.effect("skips a page of a walk under way that cannot be read, and says so", () => + Effect.gen(function* () { + const read = yield* filesDiffProvider({ + files: lightFiles(3 * FILES_DIFF_PAGE_SIZE), + failure: filesDiffUnavailable, + }); + + const slice = yield* read.diff(`${ITERATION.id}:${FILES_DIFF_PAGE_SIZE}:d`); + + expect(slice).toEqual({ + patch: "", + truncated: true, + nextCursor: `${ITERATION.id}:${2 * FILES_DIFF_PAGE_SIZE}:d`, + }); + expect(read.counts.iterationChanges).toBe(0); + }), + ); + + it.effect("fails a walk under way when its next page cannot be asked for", () => + Effect.gen(function* () { + const read = yield* filesDiffProvider({ + files: lightFiles(3 * FILES_DIFF_PAGE_SIZE), + failure: Effect.fail( + new AzureDevOpsCli.AzureDevOpsCliRateLimitError({ + operation: "execute", + command: "az", + cwd: "/w", + argumentCount: 0, + cause: "slow down", + }), + ), + }); + + const error = yield* Effect.flip(read.diff(`${ITERATION.id}:${FILES_DIFF_PAGE_SIZE}:d`)); + + expect(error.cause).toMatchObject({ _tag: "AzureDevOpsCliRateLimitError" }); + }), + ); + + it.effect("ends a walk as truncated when the page it was part-way through has shrunk", () => + Effect.gen(function* () { + const read = yield* filesDiffProvider({ files: lightFiles(20) }); + + const slice = yield* read.diff(`${ITERATION.id}:30:d`); + + expect(slice).toEqual({ patch: "", truncated: true, nextCursor: null }); + }), + ); +}); diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts index fdd14db3d16c3..4989cff0b5b2b 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts @@ -1,10 +1,12 @@ import { resolvePullRequestAuthorFilter, SourceControlProviderKind } from "@t3tools/contracts"; import * as Effect from "effect/Effect"; +import * as Result from "effect/Result"; import * as Semaphore from "effect/Semaphore"; import type { PullRequestCapabilities, PullRequestViewerPermissions } from "@t3tools/contracts"; import * as AzureDevOpsPullRequestCli from "./AzureDevOpsPullRequestCli.ts"; import { + azureDevOpsFileDiffPatch, azureDevOpsFilePatch, azureDevOpsUnreadableFilePatch, formatAzureDevOpsDiffCursor, @@ -14,6 +16,7 @@ import { MAX_DIFF_SLICE_FILES, byteLength, MAX_FILE_DIFF_EDITS, + type AzureDevOpsDiffCursor, type AzureDevOpsFileTexts, } from "./azureDevOpsDiff.ts"; import { @@ -29,6 +32,7 @@ import { import { toPullRequestChecks, type AzureDevOpsChangeEntry, + type AzureDevOpsFileDiff, type AzureDevOpsItemContent, type AzureDevOpsIteration, type AzureDevOpsPullRequest, @@ -42,8 +46,34 @@ const DIFF_FILE_CONCURRENCY = 4; /** Readers share a spawn budget across requests. */ export const MAX_DIFF_SPAWNS = 2 * DIFF_FILE_CONCURRENCY; +/** Files asked of the files diff per call; one call is one `az` startup. */ +export const FILES_DIFF_PAGE_SIZE = 50; + export const LOCATION_CACHE_CAPACITY = 128; +/** A page of the files diff and where in its list it began. */ +interface FilesDiffPage { + readonly cwd: string; + readonly number: number; + readonly iteration: AzureDevOpsIteration; + readonly start: number; + readonly files: ReadonlyArray; +} + +/** What a diff request reads: nothing to go on, or the files from its cursor's position on. */ +type OpenedDiff = + | { readonly _tag: "closed"; readonly slice: ProviderDiffSlice } + | { + readonly _tag: "open"; + readonly source: AzureDevOpsDiffCursor["source"]; + readonly iteration: AzureDevOpsIteration; + /** A file without blocks has to be read to be diffed. */ + readonly files: ReadonlyArray; + /** The files diff comes a page at a time; the iteration changes come whole. */ + readonly hasMore: boolean; + readonly truncated: boolean; + }; + const CAPABILITIES: PullRequestCapabilities = { diff: true, comment: false, @@ -268,6 +298,36 @@ export const make = Effect.gen(function* () { })), ); + /** A file's patch from Azure's blocks when they stand for it, otherwise from reading the file. */ + const patchFile = Effect.fn("AzureDevOpsPullRequestProvider.patchFile")(function* (input: { + readonly cwd: string; + readonly location: AzureDevOpsRepositoryLocation; + readonly iteration: AzureDevOpsIteration; + readonly file: AzureDevOpsFileDiff; + }) { + const { change, blocks } = input.file; + // Synchronous diffs need yields to let other server work run. + yield* Effect.yieldNow; + const fromBlocks = blocks === null ? null : azureDevOpsFileDiffPatch({ change, blocks }); + if (fromBlocks !== null) return fromBlocks; + const texts = yield* readTexts({ + cwd: input.cwd, + location: input.location, + iteration: input.iteration, + change, + }).pipe( + // Auth, rate-limit and CLI failures must reach the caller. + Effect.catchTags({ + AzureDevOpsPullRequestNotFoundError: () => Effect.succeed(null), + AzureDevOpsCommandFailedError: () => Effect.succeed(null), + AzureDevOpsPullRequestReadError: () => Effect.succeed(null), + }), + ); + return texts === null + ? azureDevOpsUnreadableFilePatch(change) + : azureDevOpsFilePatch({ change, texts }); + }); + const listLatestChanges = (input: { readonly cwd: string; readonly location: AzureDevOpsRepositoryLocation; @@ -288,6 +348,115 @@ export const make = Effect.gen(function* () { }); }; + /** The last page the files diff gave, kept until the next one replaces it. */ + let heldPage: FilesDiffPage | undefined; + + /** + * Where a diff walk reads from. It begins on the files diff, one `az` call for a page of files + * and their blocks, and reads each file instead when Azure cannot give one. A cursor names the + * source it was written for, because the two list files in different orders. + */ + const openDiff = Effect.fn("AzureDevOpsPullRequestProvider.openDiff")(function* (input: { + readonly cwd: string; + readonly location: AzureDevOpsRepositoryLocation; + readonly number: number; + readonly cursor: AzureDevOpsDiffCursor | null; + }): Effect.fn.Return { + const { cursor, ...scope } = input; + const start = cursor?.fileIndex ?? 0; + // Azure orders each page by itself, so a file index only means something in the page it was + // counted in, and a walk always asks for pages from a multiple of the page size. + const pageStart = start - (start % FILES_DIFF_PAGE_SIZE); + + const openChanges = (iteration: AzureDevOpsIteration) => + cli.listIterationChanges({ ...scope, iterationId: iteration.id }).pipe( + Effect.map((listed): OpenedDiff => ({ + _tag: "open", + source: "changes", + iteration, + files: listed.changes.slice(start).map((change) => ({ change, blocks: null })), + hasMore: false, + truncated: listed.truncated, + })), + ); + + const openPage = (page: FilesDiffPage): OpenedDiff => ({ + _tag: "open", + source: "filesdiff", + iteration: page.iteration, + files: page.files.slice(start - page.start), + hasMore: page.files.length === FILES_DIFF_PAGE_SIZE, + truncated: false, + }); + + // A walk that began on the iteration changes carries on there. + if (cursor?.source === "changes") { + const iteration = (yield* cli.listIterations(scope)).find( + (candidate) => candidate.id === cursor.iterationId, + ); + return iteration === undefined + ? { _tag: "closed", slice: EMPTY_DIFF_SLICE } + : yield* openChanges(iteration); + } + + const held = heldPage; + if ( + cursor !== null && + held !== undefined && + held.cwd === scope.cwd && + held.number === scope.number && + held.iteration.id === cursor.iterationId && + held.start <= start && + start < held.start + held.files.length + ) { + return openPage(held); + } + + // The latest iteration pins the walk, so it is read alongside the page. A push between the + // two reads can still mix iterations in one page, which nothing later catches. + const [iterations, listed] = yield* Effect.all( + [ + cli.listIterations(scope), + cli + .listFilesDiff({ ...scope, top: FILES_DIFF_PAGE_SIZE, skip: pageStart }) + .pipe(Effect.result), + ], + { concurrency: 2 }, + ); + const iteration = iterations.at(-1); + if (iteration === undefined) return { _tag: "closed", slice: EMPTY_DIFF_SLICE }; + if (cursor !== null && iteration.id !== cursor.iterationId) { + // A push since the walk began; the files diff now lists another change. + return { _tag: "closed", slice: { ...EMPTY_DIFF_SLICE, truncated: true } }; + } + if (Result.isFailure(listed)) { + if (cursor === null) return yield* openChanges(iteration); + // A walk under way has no other order to carry on in. A page that cannot be read, being + // too large, will not read on a retry, so it is skipped rather than ending the walk. + if (listed.failure._tag !== "AzureDevOpsPullRequestReadError") return yield* listed.failure; + return { + _tag: "closed", + slice: { + patch: "", + truncated: true, + nextCursor: formatAzureDevOpsDiffCursor({ + iterationId: iteration.id, + fileIndex: pageStart + FILES_DIFF_PAGE_SIZE, + source: "filesdiff", + }), + }, + }; + } + // An empty first page cannot be told from one Azure failed to fill. + if (cursor === null && listed.success.length === 0) return yield* openChanges(iteration); + // A page that has shrunk under a walk part-way through it has lost the walk's place. + if (start > pageStart && listed.success.length <= start - pageStart) { + return { _tag: "closed", slice: { ...EMPTY_DIFF_SLICE, truncated: true } }; + } + heldPage = { ...scope, iteration, start: pageStart, files: listed.success }; + return openPage(heldPage); + }); + const provider: PullRequestProviderApi = { kind: SourceControlProviderKind.make("azure-devops"), capabilities: CAPABILITIES, @@ -464,27 +633,25 @@ export const make = Effect.gen(function* () { // the Code tab has nothing to scope itself to and always asks for the whole change. getDiff: Effect.fn("AzureDevOpsPullRequestProvider.getDiff")( function* (input) { - const scope = yield* diffScope(input); - if (scope === null) return EMPTY_DIFF_SLICE; + const location = yield* locationOf(input); + if (location === null) return EMPTY_DIFF_SLICE; const cursor = parseAzureDevOpsDiffCursor(input.cursor); - const iteration = - cursor === null - ? scope.iterations.at(-1) - : scope.iterations.find((candidate) => candidate.id === cursor.iterationId); - if (iteration === undefined) return EMPTY_DIFF_SLICE; - const listed = yield* cli.listIterationChanges({ + + const opened = yield* openDiff({ cwd: input.cwd, - location: scope.location, + location, number: input.number, - iterationId: iteration.id, + cursor, }); - const changes = listed.changes; + if (opened._tag === "closed") return opened.slice; + const { iteration, source, files } = opened; const sections: string[] = []; - let truncated = listed.truncated; + let truncated = opened.truncated; let bytes = 0; let edits = 0; - let index = cursor?.fileIndex ?? 0; + // Files of this request patched so far; the rest wait for the next slice. + let patched = 0; let full = false; // Prefetched files beyond the slice must be read again. const batchWidth = () => { @@ -501,34 +668,19 @@ export const make = Effect.gen(function* () { ), ); }; - while (!full && index < changes.length) { - const batch = changes.slice(index, index + batchWidth()); - const read = yield* Effect.forEach( + while (!full && patched < files.length) { + const batch = files.slice(patched, patched + batchWidth()); + const patches = yield* Effect.forEach( batch, - (change) => - readTexts({ cwd: input.cwd, location: scope.location, iteration, change }).pipe( - // Auth, rate-limit and CLI failures must reach the caller. - Effect.catchTags({ - AzureDevOpsPullRequestNotFoundError: () => Effect.succeed(null), - AzureDevOpsCommandFailedError: () => Effect.succeed(null), - AzureDevOpsPullRequestReadError: () => Effect.succeed(null), - }), - Effect.map((texts) => ({ change, texts })), - ), + (file) => patchFile({ cwd: input.cwd, location, iteration, file }), { concurrency: DIFF_FILE_CONCURRENCY }, ); - for (const { change, texts } of read) { - // Synchronous diffs need yields to let other server work run. - yield* Effect.yieldNow; - const file = - texts === null - ? azureDevOpsUnreadableFilePatch(change) - : azureDevOpsFilePatch({ change, texts }); + for (const file of patches) { sections.push(file.section); bytes += byteLength(file.section); edits += file.edits; truncated = truncated || file.truncated; - index += 1; + patched += 1; // Add a file before checking budgets so oversized files cannot stall the cursor. if ( bytes >= MAX_DIFF_SLICE_BYTES || @@ -546,9 +698,13 @@ export const make = Effect.gen(function* () { patch: sections.join(""), truncated, nextCursor: - index >= changes.length + patched >= files.length && !opened.hasMore ? null - : formatAzureDevOpsDiffCursor({ iterationId: iteration.id, fileIndex: index }), + : formatAzureDevOpsDiffCursor({ + iterationId: iteration.id, + fileIndex: (cursor?.fileIndex ?? 0) + patched, + source, + }), }; return slice; }, diff --git a/packages/source-control-azure-devops/src/server/azureDevOpsDiff.test.ts b/packages/source-control-azure-devops/src/server/azureDevOpsDiff.test.ts index d82d586f5a69f..0077c3f3303ca 100644 --- a/packages/source-control-azure-devops/src/server/azureDevOpsDiff.test.ts +++ b/packages/source-control-azure-devops/src/server/azureDevOpsDiff.test.ts @@ -1,13 +1,16 @@ import { unquoteGitPatchPath } from "@t3tools/shared/gitPatchPath"; +import { applyPatch } from "diff"; import { describe, expect, it } from "vite-plus/test"; import { + azureDevOpsFileDiffPatch, azureDevOpsFilePatch, byteLength, + formatAzureDevOpsDiffCursor, MAX_FILE_DIFF_EDITS, parseAzureDevOpsDiffCursor, } from "./azureDevOpsDiff.ts"; -import type { AzureDevOpsChangeEntry } from "./azureDevOpsPullRequestJson.ts"; +import type { AzureDevOpsChangeEntry, AzureDevOpsDiffBlock } from "./azureDevOpsPullRequestJson.ts"; function change(overrides: Partial = {}): AzureDevOpsChangeEntry { return { @@ -236,8 +239,296 @@ describe("a file Azure names something a patch header cannot carry plainly", () describe("a diff cursor", () => { it("reads anything it did not write as no position at all", () => { - for (const raw of [undefined, null, "", "abc", "1", "0:4", "1:-2", "1:2:3"]) { + for (const raw of [ + undefined, + null, + "", + "abc", + "1", + "0:4", + "1:-2", + "1:2:3", + "1:2:x", + "1:2:d:1", + ]) { expect(parseAzureDevOpsDiffCursor(raw)).toBeNull(); } }); + + it("names the source of the file list its position counts in", () => { + expect(parseAzureDevOpsDiffCursor("3:12")).toEqual({ + iterationId: 3, + fileIndex: 12, + source: "changes", + }); + expect(parseAzureDevOpsDiffCursor("3:12:d")).toEqual({ + iterationId: 3, + fileIndex: 12, + source: "filesdiff", + }); + for (const source of ["changes", "filesdiff"] as const) { + const cursor = { iterationId: 5, fileIndex: 0, source }; + expect(parseAzureDevOpsDiffCursor(formatAzureDevOpsDiffCursor(cursor))).toEqual(cursor); + } + }); +}); + +/** The blocks Azure gives for text it split on "\n", which leaves a last line of "" after a newline. */ +function block( + kind: AzureDevOpsDiffBlock["kind"], + oldStart: number, + newStart: number, + oldLines: ReadonlyArray, + newLines: ReadonlyArray, + counts: { readonly old?: number; readonly new?: number } = {}, +): AzureDevOpsDiffBlock { + return { + kind, + oldStart, + newStart, + oldLines, + newLines, + oldCount: counts.old ?? oldLines.length, + newCount: counts.new ?? newLines.length, + }; +} + +const unchanged = ( + oldStart: number, + newStart: number, + lines: ReadonlyArray, + count = lines.length, +) => block("none", oldStart, newStart, lines, lines, { old: count, new: count }); + +interface BlockRow { + readonly name: string; + readonly change?: Partial; + readonly oldText: string; + readonly newText: string; + readonly blocks: ReadonlyArray; + readonly hunks: ReadonlyArray; + readonly edits: number; +} + +const NO_NEWLINE = "\\ No newline at end of file"; + +const blockRows: ReadonlyArray = [ + { + name: "an edit with its context", + oldText: "one\ntwo\nthree\n", + newText: "one\ntwo again\nthree\n", + blocks: [ + unchanged(1, 1, ["one"]), + block("edit", 2, 2, ["two"], ["two again"]), + unchanged(3, 3, ["three", ""]), + ], + hunks: ["@@ -1,3 +1,3 @@", " one", "-two", "+two again", " three"], + edits: 2, + }, + { + name: "a new file, which Azure numbers from 0", + change: { path: "NEW.md", oldPath: "NEW.md", changeKind: "new" }, + oldText: "", + newText: "a\nb\n", + blocks: [block("add", 0, 1, [], ["a", "b", ""])], + hunks: ["@@ -0,0 +1,2 @@", "+a", "+b"], + edits: 2, + }, + { + name: "a deleted file, which Azure numbers from 0", + change: { path: "OLD.md", oldPath: "OLD.md", changeKind: "deleted" }, + oldText: "gone\n", + newText: "", + blocks: [block("delete", 1, 0, ["gone", ""], [])], + hunks: ["@@ -1 +0,0 @@", "-gone"], + edits: 1, + }, + { + name: "a change a plain diff would slide past the line Azure keeps unchanged", + oldText: "p\n}\n}\n{\nL\n", + newText: "p\n}\n{\n{\nL\n", + blocks: [ + unchanged(1, 1, ["p", "}"]), + block("edit", 3, 3, ["}"], ["{"]), + unchanged(4, 4, ["{", "L", ""]), + ], + hunks: ["@@ -1,5 +1,5 @@", " p", " }", "-}", "+{", " {", " L"], + edits: 2, + }, + { + name: "two edits seven lines apart, which Azure writes with a gap between", + oldText: "a\nb\nc\nd\ne\nf\ng\nh\ni\nj\nk\n", + newText: "a\nB\nc\nd\ne\nf\ng\nh\ni\nJ\nk\n", + blocks: [ + unchanged(1, 1, ["a"]), + block("edit", 2, 2, ["b"], ["B"]), + unchanged(3, 3, ["c", "d", "e"], 7), + unchanged(7, 7, ["g", "h", "i"]), + block("edit", 10, 10, ["j"], ["J"]), + unchanged(11, 11, ["k", ""]), + ], + hunks: [ + "@@ -1,5 +1,5 @@", + " a", + "-b", + "+B", + " c", + " d", + " e", + "@@ -7,5 +7,5 @@", + " g", + " h", + " i", + "-j", + "+J", + " k", + ], + edits: 4, + }, + { + name: "an empty line that ends a window is a line of the file, not its end", + oldText: "a\nb\nc\n\nd\ne\nf\ng\nh\ni\n", + newText: "A\nb\nc\n\nd\ne\nf\ng\nh\ni\n", + blocks: [block("edit", 1, 1, ["a"], ["A"]), unchanged(2, 2, ["b", "c", ""], 9)], + hunks: ["@@ -1,4 +1,4 @@", "-a", "+A", " b", " c", " "], + edits: 2, + }, + { + name: "a last line edited in a file with no newline at its end", + oldText: "a\nb", + newText: "a\nc", + blocks: [unchanged(1, 1, ["a"]), block("edit", 2, 2, ["b"], ["c"])], + hunks: ["@@ -1,2 +1,2 @@", " a", "-b", NO_NEWLINE, "+c", NO_NEWLINE], + edits: 2, + }, + { + name: "a newline added after the last line", + oldText: "a\nb", + newText: "a\nb\n", + blocks: [unchanged(1, 1, ["a", "b"]), block("add", 3, 3, [], [""])], + hunks: ["@@ -1,2 +1,2 @@", " a", "-b", NO_NEWLINE, "+b"], + edits: 2, + }, + { + name: "the newline taken from after the last line", + oldText: "a\nb\n", + newText: "a\nb", + blocks: [unchanged(1, 1, ["a", "b"]), block("delete", 3, 3, [""], [])], + hunks: ["@@ -1,2 +1,2 @@", " a", "-b", "+b", NO_NEWLINE], + edits: 2, + }, + { + name: "a line appended to a file that had no newline", + oldText: "a", + newText: "a\nb\n", + blocks: [unchanged(1, 1, ["a"]), block("edit", 2, 2, [], ["b", ""])], + hunks: ["@@ -1 +1,2 @@", "-a", NO_NEWLINE, "+a", "+b"], + edits: 3, + }, + { + name: "a last window that counts the newline the new side gained, which no line shows", + oldText: "a\nb", + newText: "A\nb\n", + blocks: [ + block("edit", 1, 1, ["a"], ["A"]), + block("none", 2, 2, ["b"], ["b"], { old: 1, new: 2 }), + ], + hunks: ["@@ -1,2 +1,2 @@", "-a", "-b", NO_NEWLINE, "+A", "+b"], + edits: 4, + }, + { + name: "a new file with no newline at its end", + change: { path: "NEW.md", oldPath: "NEW.md", changeKind: "new" }, + oldText: "", + newText: "x\ny", + blocks: [block("add", 0, 1, [], ["x", "y"])], + hunks: ["@@ -0,0 +1,2 @@", "+x", "+y", NO_NEWLINE], + edits: 2, + }, +]; + +describe("azureDevOpsFileDiffPatch", () => { + it.each(blockRows)("writes $name as the unified patch", (row) => { + const patch = azureDevOpsFileDiffPatch({ + change: change(row.change), + blocks: row.blocks, + }); + + expect(patch).not.toBeNull(); + expect(patch?.truncated).toBe(false); + expect(patch?.edits).toBe(row.edits); + const section = patch?.section ?? ""; + expect(section.slice(section.indexOf("@@"))).toBe(`${row.hunks.join("\n")}\n`); + // The hunks must say what the files say, whatever they look like. + expect(applyPatch(row.oldText, section.slice(section.indexOf("--- ")))).toBe(row.newText); + }); + + it("keeps a file that only moved, which lists no blocks", () => { + const patch = azureDevOpsFileDiffPatch({ + change: change({ path: "docs/new.md", oldPath: "docs/old.md", changeKind: "rename-pure" }), + blocks: [], + }); + + expect(patch?.truncated).toBe(false); + expect(patch?.section).toContain("rename from docs/old.md"); + expect(patch?.section).not.toContain("@@"); + }); + + it.each([ + ["blocks it could not read", null], + ["no blocks for a file that did not just move", []], + [ + "a window whose two sides differ in length", + [block("none", 1, 1, ["a", "b"], ["a"], { old: 2, new: 1 })], + ], + [ + "an edit whose lines are not the count it states", + [block("edit", 1, 1, ["a"], ["b"], { old: 2 })], + ], + [ + "blocks that run backwards", + [block("edit", 5, 5, ["a"], ["b"]), block("edit", 2, 2, ["c"], ["d"])], + ], + ["blocks that change nothing", [unchanged(1, 1, ["same", ""])]], + [ + "blocks whose only change is indentation, which Azure takes for equal", + [block("none", 1, 1, [" keep", ""], ["keep", ""])], + ], + [ + "an edit with less context than a diff gives, before text Azure elided", + [block("edit", 1, 1, ["a"], ["A"]), unchanged(2, 2, ["b"], 9), unchanged(8, 8, ["h", ""])], + ], + [ + "an edit with less context than a diff gives, before the end of a file Azure elided", + [block("edit", 1, 1, ["a"], ["A"]), unchanged(2, 2, ["b"], 9)], + ], + [ + "a last window that hides more lines on one side, whose newline change no line shows", + [ + block("edit", 1, 1, ["a"], ["A"]), + block("none", 2, 2, ["b", "c", "d"], ["b", "c", "d"], { old: 5, new: 6 }), + ], + ], + [ + "a line further down than a file may be long", + [block("edit", 2_000_000, 2_000_000, ["a"], ["b"])], + ], + [ + "a line holding one of the tags a rebuilt side marks its lines with", + [block("edit", 1, 1, ["a\u0002"], ["a\u0003"])], + ], + ] as const)("leaves %s for the caller to read", (_name, blocks) => { + expect(azureDevOpsFileDiffPatch({ change: change(), blocks })).toBeNull(); + }); + + it("lists a file too long to write out without its hunks, as the diff of its contents does", () => { + const lines = Array.from({ length: 1_000 }, (_, line) => `${line} ${"z".repeat(600)}`); + const patch = azureDevOpsFileDiffPatch({ + change: change({ path: "big.txt", oldPath: "big.txt", changeKind: "new" }), + blocks: [block("add", 0, 1, [], [...lines, ""])], + }); + + expect(patch?.truncated).toBe(true); + expect(patch?.section).not.toContain("@@"); + }); }); diff --git a/packages/source-control-azure-devops/src/server/azureDevOpsDiff.ts b/packages/source-control-azure-devops/src/server/azureDevOpsDiff.ts index 15bddb5a63183..2d94a02848fd9 100644 --- a/packages/source-control-azure-devops/src/server/azureDevOpsDiff.ts +++ b/packages/source-control-azure-devops/src/server/azureDevOpsDiff.ts @@ -1,35 +1,45 @@ import { quoteGitPatchPath } from "@t3tools/shared/gitPatchPath"; import { structuredPatch } from "diff"; -import type { AzureDevOpsChangeEntry } from "./azureDevOpsPullRequestJson.ts"; +import type { AzureDevOpsChangeEntry, AzureDevOpsDiffBlock } from "./azureDevOpsPullRequestJson.ts"; -/** Pin the iteration to prevent pushes from renumbering files during pagination. */ +/** + * Pin the iteration to prevent pushes from renumbering files during pagination. The two sources + * list files in different orders, so a position only holds for the source that wrote it. + */ export interface AzureDevOpsDiffCursor { readonly iterationId: number; readonly fileIndex: number; + readonly source: "changes" | "filesdiff"; } const CURSOR_SEPARATOR = ":"; const CURSOR_COMPONENT = /^\d+$/; +const FILES_DIFF_MARKER = "d"; + export function formatAzureDevOpsDiffCursor(cursor: AzureDevOpsDiffCursor): string { - return `${cursor.iterationId}${CURSOR_SEPARATOR}${cursor.fileIndex}`; + const position = `${cursor.iterationId}${CURSOR_SEPARATOR}${cursor.fileIndex}`; + return cursor.source === "filesdiff" + ? `${position}${CURSOR_SEPARATOR}${FILES_DIFF_MARKER}` + : position; } export function parseAzureDevOpsDiffCursor( raw: string | null | undefined, ): AzureDevOpsDiffCursor | null { if (raw === null || raw === undefined) return null; - const [iteration, file, ...rest] = raw.split(CURSOR_SEPARATOR); + const [iteration, file, marker, ...rest] = raw.split(CURSOR_SEPARATOR); if (rest.length > 0) return null; + if (marker !== undefined && marker !== FILES_DIFF_MARKER) return null; if (iteration === undefined || file === undefined) return null; if (!CURSOR_COMPONENT.test(iteration) || !CURSOR_COMPONENT.test(file)) return null; const iterationId = Number(iteration); const fileIndex = Number(file); if (!Number.isSafeInteger(iterationId) || iterationId <= 0) return null; if (!Number.isSafeInteger(fileIndex) || fileIndex < 0) return null; - return { iterationId, fileIndex }; + return { iterationId, fileIndex, source: marker === undefined ? "changes" : "filesdiff" }; } export interface AzureDevOpsFileTexts { @@ -200,3 +210,104 @@ export function azureDevOpsUnreadableFilePatch( ): AzureDevOpsFilePatch { return { section: `${patchHeader(change)}\n`, truncated: true, abandoned: false, edits: 0 }; } + +/** Stands for a line Azure left out, so the lines after it keep their numbers. */ +const ELIDED_LINE = "\u0001"; + +/** Prefixes the lines Azure changed on each side, so a diff cannot match them to each other. */ +const OLD_CHANGED = "\u0002"; +const NEW_CHANGED = "\u0003"; +const TAGS = [ELIDED_LINE, OLD_CHANGED, NEW_CHANGED]; + +/** A window shows equal lines on both sides, maybe fewer than it counts; a change shows all it counts. */ +function isCoherentBlock(block: AzureDevOpsDiffBlock): boolean { + if (block.kind === "none") { + return ( + block.oldLines.length === block.newLines.length && block.oldLines.length <= block.oldCount + ); + } + return block.oldLines.length === block.oldCount && block.newLines.length === block.newCount; +} + +/** + * Both sides of a file as far as the blocks tell it: the changed lines, the unchanged lines Azure + * shows around them, and ELIDED_LINE for everything it left out. Null when the blocks do not add + * up to a file. + */ +function rebuildSides( + blocks: ReadonlyArray, +): { readonly oldLines: string[]; readonly newLines: string[] } | null { + const oldLines: string[] = []; + const newLines: string[] = []; + const place = (side: string[], start: number, lines: ReadonlyArray, tag: string) => { + if (lines.length === 0) return true; + // A file with more lines than bytes it may have is too long to rebuild. + if (start <= side.length || start > MAX_FILE_BYTES) return false; + // A line holding a tag could be taken for one. + if (lines.some((line) => TAGS.some((reserved) => line.includes(reserved)))) return false; + while (side.length + 1 < start) side.push(ELIDED_LINE); + for (const line of lines) side.push(tag + line); + return true; + }; + + // The lines Azure counts after the last it shows on a side, because it shows only a window. + let oldHidden = 0; + let newHidden = 0; + for (const block of blocks) { + if (!isCoherentBlock(block)) return null; + const window = block.kind === "none"; + // Lines Azure matched stand for both sides, whatever their indentation. + const oldShown = window ? block.newLines : block.oldLines; + if (!place(oldLines, block.oldStart, oldShown, window ? "" : OLD_CHANGED)) return null; + if (!place(newLines, block.newStart, block.newLines, window ? "" : NEW_CHANGED)) return null; + if (oldShown.length > 0) oldHidden = block.oldCount - block.oldLines.length; + if (block.newLines.length > 0) newHidden = block.newCount - block.newLines.length; + } + + // Azure splits on "\n", so a file ending in a newline ends in one more, empty, line. + // That line is the end of the file, not a changed line. + if (oldLines.at(-1) === OLD_CHANGED) oldLines[oldLines.length - 1] = ""; + if (newLines.at(-1) === NEW_CHANGED) newLines[newLines.length - 1] = ""; + // Tails of unlike lengths hide a change in how the file ends, which no line shows. + if (oldHidden !== newHidden && Math.min(oldHidden, newHidden) > 1) return null; + // One hidden line is the empty one after a final newline; more is text Azure left out. + for (const [side, hidden] of [ + [oldLines, oldHidden], + [newLines, newHidden], + ] as const) { + if (hidden > 1) side.push(ELIDED_LINE); + if (hidden > 0) side.push(""); + } + return { oldLines, newLines }; +} + +/** + * The patch from Azure's diff blocks, or null when they cannot stand for the file. Both sides + * are rebuilt and diffed as whole files, with the changed lines tagged to keep Azure's alignment. + * Azure ignores indentation, so an indentation-only edit never arrives, as on its own page. + */ +export function azureDevOpsFileDiffPatch(input: { + readonly change: AzureDevOpsChangeEntry; + readonly blocks: ReadonlyArray | null; +}): AzureDevOpsFilePatch | null { + const { change, blocks } = input; + const sides = blocks === null ? null : rebuildSides(blocks); + if (sides === null) return null; + + const patch = azureDevOpsFilePatch({ + change, + texts: { + oldContents: sides.oldLines.join("\n"), + newContents: sides.newLines.join("\n"), + binary: false, + }, + }); + // A hunk that reaches text Azure left out cannot be trusted. + if (patch.section.includes(ELIDED_LINE)) return null; + // No blocks is a binary or empty file, which only a read can tell from a move. + if (patch.edits === 0 && !patch.truncated && change.changeKind !== "rename-pure") return null; + return { + ...patch, + section: patch.section.replaceAll(OLD_CHANGED, "").replaceAll(NEW_CHANGED, ""), + }; +} diff --git a/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.test.ts b/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.test.ts index 75746ea0881de..defc475722376 100644 --- a/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.test.ts +++ b/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.test.ts @@ -2,6 +2,7 @@ import * as Result from "effect/Result"; import { describe, expect, it } from "vite-plus/test"; import { + decodeFilesDiffJson, decodeItemContentJson, decodeIterationChangesJson, decodeIterationsJson, @@ -435,6 +436,113 @@ describe("decodeItemContentJson", () => { }); }); +/** A page's JSON as a string, in ASCII: what `az` prints for `--query to_string(@)`. */ +const asQuotedPage = (page: unknown) => + JSON.stringify(JSON.stringify(page)).replace( + /[\u0080-\uffff]/g, + (char) => `\\u${char.charCodeAt(0).toString(16).padStart(4, "0")}`, + ); + +describe("decodeFilesDiffJson", () => { + const editBlock = { + changeType: "edit", + originalLineNumberStart: 2, + originalLinesCount: 1, + originalLines: ["two"], + modifiedLineNumberStart: 2, + modifiedLinesCount: 1, + modifiedLines: ["two again — §6"], + }; + + it("reads each file with its blocks, naming a deleted file by the path it had", () => { + const files = expectSuccess( + decodeFilesDiffJson( + asQuotedPage({ + fileDiffs: [ + { + path: "/docs/new.md", + originalPath: "/docs/old.md", + changeType: "edit, rename", + lineDiffBlocks: [editBlock], + }, + { path: null, originalPath: "/gone.md", changeType: "delete" }, + ], + }), + ), + ); + + expect(files).toEqual([ + { + change: { + path: "docs/new.md", + oldPath: "docs/old.md", + changeKind: "rename-changed", + objectId: null, + originalObjectId: null, + }, + blocks: [ + { + kind: "edit", + oldStart: 2, + oldCount: 1, + newStart: 2, + newCount: 1, + oldLines: ["two"], + newLines: ["two again — §6"], + }, + ], + }, + { + change: { + path: "gone.md", + oldPath: "gone.md", + changeKind: "deleted", + objectId: null, + originalObjectId: null, + }, + blocks: [], + }, + ]); + }); + + it("keeps a file whose blocks it cannot trust, so the caller can read it another way", () => { + const files = expectSuccess( + decodeFilesDiffJson( + asQuotedPage({ + fileDiffs: [ + { + path: "/a.md", + originalPath: "/a.md", + changeType: "edit", + lineDiffBlocks: [{ ...editBlock, changeType: "move" }], + }, + { + path: "/b.md", + originalPath: "/b.md", + changeType: "edit", + lineDiffBlocks: [{ ...editBlock, modifiedLinesCount: undefined }], + }, + ], + }), + ), + ); + + expect(files.map((file) => file.blocks)).toEqual([null, null]); + }); + + it.each([ + ["a page without its files", asQuotedPage({ pullRequestTitle: "Anything" })], + ["a file that names no path", asQuotedPage({ fileDiffs: [{ changeType: "edit" }] })], + [ + "a file that is not one", + asQuotedPage({ fileDiffs: [{ path: "/a.md", originalPath: "/a.md" }, "unexpected"] }), + ], + ["a page that is not quoted", asJson({ fileDiffs: [] })], + ])("fails the whole page for %s, rather than shift the files after it", (_name, raw) => { + expect(Result.isFailure(decodeFilesDiffJson(raw))).toBe(true); + }); +}); + describe("what az devops invoke answers with", () => { it("reads the envelope the extension adds its own continuation token to", () => { expect( diff --git a/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.ts b/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.ts index cecb98a603df7..86f05c8fb9346 100644 --- a/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.ts +++ b/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.ts @@ -621,6 +621,25 @@ const RawChangePageSchema = Schema.Struct({ nextSkip: Schema.optional(Schema.NullOr(Schema.Number)), }); +const RawFileDiffBlockSchema = Schema.Struct({ + changeType: Schema.Literals(["none", "add", "delete", "edit"]), + originalLineNumberStart: Schema.Int, + originalLinesCount: Schema.Int, + modifiedLineNumberStart: Schema.Int, + modifiedLinesCount: Schema.Int, + originalLines: Schema.optional(Schema.NullOr(Schema.Array(Schema.String))), + modifiedLines: Schema.optional(Schema.NullOr(Schema.Array(Schema.String))), +}); + +const RawFileDiffSchema = Schema.Struct({ + path: Schema.optional(Schema.NullOr(Schema.String)), + originalPath: Schema.optional(Schema.NullOr(Schema.String)), + changeType: Schema.optional(Schema.NullOr(Schema.String)), + lineDiffBlocks: Schema.optional(Schema.NullOr(Schema.Array(Schema.Unknown))), +}); + +const RawFilesDiffPageSchema = Schema.Struct({ fileDiffs: Schema.Array(Schema.Unknown) }); + const RawItemContentSchema = Schema.Struct({ content: Schema.optional(Schema.NullOr(Schema.String)), contentMetadata: Schema.optional( @@ -647,6 +666,22 @@ export interface AzureDevOpsChangePage { readonly nextSkip: number | null; } +export interface AzureDevOpsDiffBlock { + readonly kind: "none" | "add" | "delete" | "edit"; + readonly oldStart: number; + readonly oldCount: number; + readonly newStart: number; + readonly newCount: number; + readonly oldLines: ReadonlyArray; + readonly newLines: ReadonlyArray; +} + +export interface AzureDevOpsFileDiff { + readonly change: AzureDevOpsChangeEntry; + /** Null when the file has no blocks to go by, so it is read instead. */ + readonly blocks: ReadonlyArray | null; +} + export interface AzureDevOpsItemContent { readonly contents: string; readonly isBinary: boolean; @@ -656,6 +691,10 @@ const decodeIterationPage = decodeJsonResult(RawIterationPageSchema); const decodeIterationEntry = Schema.decodeUnknownExit(RawIterationSchema); const decodeChangePage = decodeJsonResult(RawChangePageSchema); const decodeChangeEntry = Schema.decodeUnknownExit(RawChangeEntrySchema); +const decodeQuotedJson = decodeJsonResult(Schema.String); +const decodeFilesDiffPage = decodeJsonResult(RawFilesDiffPageSchema); +const decodeFileDiff = Schema.decodeUnknownExit(RawFileDiffSchema); +const decodeFileDiffBlock = Schema.decodeUnknownExit(RawFileDiffBlockSchema); const decodeItemContent = decodeJsonResult(RawItemContentSchema); /** Preserve whitespace to match patch paths and viewed marks. */ @@ -732,6 +771,61 @@ export function decodeIterationChangesJson( }); } +function toDiffBlocks(raw: ReadonlyArray): ReadonlyArray | null { + const blocks: AzureDevOpsDiffBlock[] = []; + for (const entry of raw) { + const decoded = decodeFileDiffBlock(entry); + if (Exit.isFailure(decoded)) return null; + const block = decoded.value; + blocks.push({ + kind: block.changeType, + oldStart: block.originalLineNumberStart, + oldCount: block.originalLinesCount, + newStart: block.modifiedLineNumberStart, + newCount: block.modifiedLinesCount, + oldLines: block.originalLines ?? [], + newLines: block.modifiedLines ?? [], + }); + } + return blocks; +} + +/** + * One page of files with their diff blocks. A row that cannot be read fails the page, which + * counts every row Azure sent. The page arrives as a JSON string (`to_string(@)`) because on + * Windows `az` drops every character its console code page lacks from plain JSON, page-wide. + */ +export function decodeFilesDiffJson( + raw: string, +): Result.Result, unknown> { + const quoted = decodeQuotedJson(raw); + if (!Result.isSuccess(quoted)) return Result.fail(quoted.failure); + const decoded = decodeFilesDiffPage(quoted.success); + if (!Result.isSuccess(decoded)) return Result.fail(decoded.failure); + const files: AzureDevOpsFileDiff[] = []; + for (const entry of decoded.success.fileDiffs) { + const decodedFile = decodeFileDiff(entry); + if (Exit.isFailure(decodedFile)) return Result.fail(decodedFile.cause); + const file = decodedFile.value; + const originalPath = toRepositoryPath(file.originalPath); + // A deleted file has no current path. + const path = toRepositoryPath(file.path) ?? originalPath; + if (path === null) return Result.fail(new Error("A file diff named no path.")); + const oldPath = originalPath ?? path; + files.push({ + change: { + path, + oldPath, + changeKind: toChangeKind(file.changeType, oldPath !== path), + objectId: null, + originalObjectId: null, + }, + blocks: toDiffBlocks(file.lineDiffBlocks ?? []), + }); + } + return Result.succeed(files); +} + /** Azure returns an empty body for absent files. Binary status comes from its metadata. */ export function decodeItemContentJson( raw: string,