Large pull requests, commits and comparisons open quickly and no longer risk failing: the Files changed tab of a 206-file pull request sent every file's diff at once, 14,366 table rows drawn on the server (6.7 MB of its 8.7 MB of HTML) and the same lines again in the data the page hydrates from (1.6 MB), with 300 KB of highlighting sent twice. Now the page arrives with the whole file list, tree and counts, and the lines of its first files, about 2,000; every later file holds its place at its height and loads as you scroll near it, eight or so to a request (diff-files.json, cached by the two commits compared, here and in the browser), and a refresh keeps what was loaded. A file with a diff over 1,000 lines (a lockfile, generated code) waits for Load diff; files with comments on their lines come with the page wherever they are; a link to a file (#file-<path>, or #diff-<path>) opens it, loads it at once however large, and keeps it at the top while the files above it fill in. Small changes are sent exactly as before. Highlighting done on the server stops after 16,000 characters rather than 40,000; the rest is coloured in the browser as it comes near, as it already was. The repos service listed the files past its 20,000-line read as binary with no counts, so 126 of this pull request's files said "Binary or large file"; they are now marked omitted, show no counts until read ("126 files not counted" in the toolbar), and load like the rest, read on their own by compare's new paths (at most 50); truncated now means only that more than 300 files changed. get_pull_request_changes and GET /repos/{owner}/{name}/pulls/{number}/changes take paths too, and the mobile app says an omitted file is too large to show there. The pull requests guide gains Files changed, and limitations says how large changes are shown. Checked: the site's typecheck, its 914 tests (new: the first-screens budget, large and commented files, omitted and binary ones, links to a file, and fetching in batches, once, with retries) and a production build; cargo test for contracts, repos (new: omitted files and reading by path), actions, security, work and api; the mobile typecheck; the docs build. Measured on flagon-io/g1t pull 1's real response, before → after: single-fetch data 1,592,101 → 236,252 bytes; the diff's server-rendered HTML 6,694,038 → 1,698,300 bytes (renderToString 62 → 15 ms), so the page's HTML about 8.7 MB → 2.0 MB; in a harness mounting the real routes in Chrome at 1280 px, first render 600 → 270 ms, settled 2,250 → 750 ms, long tasks 1,700 → 500 ms, 84,605 → 17,982 elements; scrolling to the bottom loads all 72 waiting files in 9 requests, and refreshes with the same data remove no elements, before and after files load. Screenshots at 1280 and 390 px looked at.
26 files+862−550/26 viewed
| 401 | 401 | } | |
| 402 | 402 | } | |
| 403 | 403 | ||
| 404 | + | /// A list given as an array, or comma-separated (from a query string); none when empty. | |
| 405 | + | fn path_list(input: &Value, key: &str) -> Option<Vec<String>> { | |
| 406 | + | let items: Vec<String> = match &input[key] { | |
| 407 | + | Value::Array(items) => items.iter().filter_map(|item| item.as_str().map(str::to_owned)).collect(), | |
| 408 | + | Value::String(text) => text.split(',').map(str::to_owned).collect(), | |
| 409 | + | _ => Vec::new(), | |
| 410 | + | }; | |
| 411 | + | let items: Vec<String> = items.into_iter().map(|item| item.trim().to_owned()).filter(|item| !item.is_empty()).collect(); | |
| 412 | + | (!items.is_empty()).then_some(items) | |
| 413 | + | } | |
| 414 | + | ||
| 404 | 415 | fn strings(input: &Value, key: &str) -> Option<Vec<String>> { | |
| 405 | 416 | input[key].as_array().map(|items| { | |
| 406 | 417 | items | |
| ⋯ | |||
| 1539 | 1550 | Op::ReopenPullRequest => "Reopen a closed pull request. It comes back as the draft it was if it was closed as one, and ready for review otherwise; a merged pull request cannot be reopened, nor one whose branch was deleted. Its author may reopen their own, and whoever asked g1t for one may reopen that one; anyone else needs the Triage role or higher. Publishes pull.reopened with its head commit.", | |
| 1540 | 1551 | Op::ConvertPullRequestToDraft => "Turn a pull request that is ready for review back into a draft. A draft cannot be merged until it is marked ready again; it leaves the merge queue, and a merge waiting for it to catch up is called off. Its author may, and whoever asked g1t for it; anyone else needs the Triage role or higher. Publishes pull.converted_to_draft.", | |
| 1541 | 1552 | Op::GetPullRequestChanges => { | |
| 1542 | − | "What a pull request changes: the files it touches and their line-by-line diff against the commit it started from. Use it to review a pull request or to compare several made for the same issue." | |
| 1553 | + | "What a pull request changes: the files it touches and their line-by-line diff against the commit it started from. Use it to review a pull request or to compare several made for the same issue. A large one lists every file (up to 300; truncated says when there were more) but has the lines of only the first, about 20,000 lines read: the files after that come with omitted true and no lines or counts. Ask again with paths (up to 50 at a time) to have those files' lines." | |
| 1543 | 1554 | } | |
| 1544 | 1555 | Op::MergePullRequest => { | |
| 1545 | 1556 | "Land a pull request on its base, the branch it merges into (the default branch unless it names another). Merging needs the Write role or higher, and only once it is marked ready and it meets every rule that holds for its base (see rules and required_checks on get_pull_request: approvals, checks, deployments, merge windows and the rest, from the repository's and its workspace's rulesets); the refusal names the first rule not met. With ignore_checks, someone who may merge can bypass required checks where the rule allows it; with bypass_rules, someone a ruleset lists as a bypass actor merges past its rules, and it is recorded. Merging into the default branch resolves the issue it was made for: the issue closes recording this pull request, and the other pull requests still in progress for that issue close as superseded; merging into another branch leaves the issue open. Where the repository has a merge queue, a pull request into the default branch joins the queue instead of landing at once. If its base has moved since the pull request was opened, it is brought up to date first and lands when that is done; a repository that requires pull requests into its default branch to be up to date refuses instead, so pull the base into its fork or branch, push, and merge again. Check status in the result to see whether it has landed." | |
| ⋯ | |||
| 2331 | 2342 | | Op::GetPullRequest | |
| 2332 | 2343 | | Op::ClosePullRequest | |
| 2333 | 2344 | | Op::ReopenPullRequest | |
| 2334 | − | | Op::ConvertPullRequestToDraft | |
| 2335 | − | | Op::GetPullRequestChanges => just_numbered(), | |
| 2345 | + | | Op::ConvertPullRequestToDraft => just_numbered(), | |
| 2346 | + | Op::GetPullRequestChanges => object( | |
| 2347 | + | numbered(json!({ | |
| 2348 | + | "paths": { | |
| 2349 | + | "type": "array", | |
| 2350 | + | "items": { "type": "string" }, | |
| 2351 | + | "description": "Only these files, with their lines (at most 50): how a file listed with omitted true is read. Comma-separated in a query string.", | |
| 2352 | + | }, | |
| 2353 | + | })), | |
| 2354 | + | &["repo", "number"], | |
| 2355 | + | ), | |
| 2336 | 2356 | Op::CreateIssue => object( | |
| 2337 | 2357 | json!({ | |
| 2338 | 2358 | "repo": repo_schema(), | |
| ⋯ | |||
| 4704 | 4724 | let found: Outcome<PullDetail> = call(work, "get_pull", &view()).await?; | |
| 4705 | 4725 | match found { | |
| 4706 | 4726 | Outcome::Ok(detail) => { | |
| 4707 | − | pass(repos, "compare", &detail.pull.comparison(viewer)).await | |
| 4727 | + | let mut asked = detail.pull.comparison(viewer); | |
| 4728 | + | asked.paths = path_list(input, "paths"); | |
| 4729 | + | pass(repos, "compare", &asked).await | |
| 4708 | 4730 | } | |
| 4709 | 4731 | Outcome::Fail(failure) => Ok(Outcome::Fail(failure)), | |
| 4710 | 4732 | } | |
| 1028 | 1028 | "GET", | |
| 1029 | 1029 | "/repos/:owner/:name/pulls/:number/changes", | |
| 1030 | 1030 | Op::GetPullRequestChanges, | |
| 1031 | − | &[], | |
| 1031 | + | &[("paths", "paths")], | |
| 1032 | 1032 | ), | |
| 1033 | 1033 | route( | |
| 1034 | 1034 | "POST", |
| 175 | 175 | act on. See [Dependency updates](/guides/dependency-updates/#options). | |
| 176 | 176 | - **Status.** Planned. | |
| 177 | 177 | ||
| 178 | + | ### Very large changes are shown in parts | |
| 179 | + | ||
| 180 | + | A pull request, commit or comparison lists at most 300 changed files. Of | |
| 181 | + | those, lines are read for the first ones until about 20,000 lines; the | |
| 182 | + | rest are listed without their counts and load one batch at a time as you | |
| 183 | + | scroll to them. A single file over 512 KB is not shown at all, and one | |
| 184 | + | with a diff over 1,000 lines waits for **Load diff**. | |
| 185 | + | ||
| 186 | + | - **Instead.** For a change of more than 300 files, compare the commits | |
| 187 | + | with git (`git diff --stat base...head`). From the API, ask for the files | |
| 188 | + | a large answer left out by `paths`. | |
| 189 | + | See [Files changed](/guides/pull-requests/#files-changed). | |
| 190 | + | - **Status.** Not planned to change. | |
| 191 | + | ||
| 178 | 192 | ### No conflict resolution in the browser | |
| 179 | 193 | ||
| 180 | 194 | You can't resolve a merge conflict on the pull request's page. |
| 1 | 1 | --- | |
| 2 | 2 | title: Pull requests and checks | |
| 3 | − | description: What the merge box shows before a pull request can merge, the checks your workflows report and which ones a merge needs, reviewers, sessions, drafts, closing and reopening, editing and deleting comments, and conflicts found before anyone tries to merge. | |
| 3 | + | description: What the merge box shows before a pull request can merge, the checks your workflows report and which ones a merge needs, reviewers, sessions, how a large change's files load, drafts, closing and reopening, editing and deleting comments, and conflicts found before anyone tries to merge. | |
| 4 | 4 | --- | |
| 5 | 5 | ||
| 6 | 6 | At the foot of an open pull request's conversation, the **merge box** says | |
| ⋯ | |||
| 187 | 187 | `read_session`. Blame uses it to say why a line is the way it is. See | |
| 188 | 188 | [sessions and why-blame](/guides/why-blame/). | |
| 189 | 189 | ||
| 190 | + | ## Files changed | |
| 191 | + | ||
| 192 | + | The **Files changed** tab lists every file the pull request changes, with | |
| 193 | + | a tree of them beside the diffs and how many lines each adds and removes. | |
| 194 | + | A commit's page and a comparison of two branches show their changes the | |
| 195 | + | same way. | |
| 196 | + | ||
| 197 | + | A large change opens quickly however many files it has. The page arrives | |
| 198 | + | with the first files' lines, about 2,000 of them, and the rest of the list | |
| 199 | + | with their counts; each later file's lines load as you scroll near it. | |
| 200 | + | Files with comments on their lines arrive with the page wherever they are, | |
| 201 | + | so their comments are there at once. | |
| 202 | + | ||
| 203 | + | A file whose diff is longer than 1,000 lines (a lockfile or generated | |
| 204 | + | code, usually) waits until you press **Load diff**. A link to a file | |
| 205 | + | (`#file-` and its path, such as `…/pull/12?tab=changes#file-src/app.ts`) | |
| 206 | + | opens it and loads its lines at once, however long. | |
| 207 | + | ||
| 208 | + | Some changes are too large to read in one go: | |
| 209 | + | ||
| 210 | + | - A comparison lists at most 300 files. When more changed, the page says | |
| 211 | + | so under the last one. | |
| 212 | + | - Lines are read for the first files until about 20,000 have been read. | |
| 213 | + | The files after that are listed without their counts and load the same | |
| 214 | + | way as you reach them; until then the toolbar says how many files are | |
| 215 | + | not counted. | |
| 216 | + | - A file over 512 KB, or a binary one, is listed without its contents. | |
| 217 | + | ||
| 218 | + | From the API, `get_pull_request_changes` works the same way: a file left | |
| 219 | + | out has `omitted: true`, and asking again with its path in `paths` (up to | |
| 220 | + | 50 at a time) returns its lines. | |
| 221 | + | ||
| 190 | 222 | ## Drafts, closing and reopening | |
| 191 | 223 | ||
| 192 | 224 | A draft is still being worked on: it can be reviewed, but it cannot merge | |
Binary or large file; its contents are not shown.
| 97 | 97 | <Text size="mono" style={{ flex: 1 }} numberOfLines={1}> | |
| 98 | 98 | {file.path} | |
| 99 | 99 | </Text> | |
| 100 | − | <Text size="mono" tone="success"> | |
| 101 | − | +{file.additions} | |
| 102 | − | </Text> | |
| 103 | − | <Text size="mono" tone="danger"> | |
| 104 | − | −{file.deletions} | |
| 100 | + | {!file.omitted && ( | |
| 101 | + | <> | |
| 102 | + | <Text size="mono" tone="success"> | |
| 103 | + | +{file.additions} | |
| 104 | + | </Text> | |
| 105 | + | <Text size="mono" tone="danger"> | |
| 106 | + | −{file.deletions} | |
| 107 | + | </Text> | |
| 108 | + | </> | |
| 109 | + | )} | |
| 110 | + | </View> | |
| 111 | + | {file.omitted ? ( | |
| 112 | + | <Text tone="muted" size="small" style={{ padding: space.md }}> | |
| 113 | + | Not shown here: this change is too large. Open it on the web to read this file. | |
| 105 | 114 | </Text> | |
| 106 | − | </View> | |
| 107 | − | {file.binary ? ( | |
| 115 | + | ) : file.binary ? ( | |
| 108 | 116 | <Text tone="muted" size="small" style={{ padding: space.md }}> | |
| 109 | 117 | A binary file. | |
| 110 | 118 | </Text> |
| 82 | 82 | additions: number; | |
| 83 | 83 | deletions: number; | |
| 84 | 84 | binary: boolean; | |
| 85 | + | /** Past what one answer reads: listed without its lines or counts. */ | |
| 86 | + | omitted?: boolean; | |
| 85 | 87 | hunks?: { lines: { kind: "context" | "add" | "delete"; old: number | null; new: number | null; text: string }[] }[]; | |
| 86 | 88 | }[]; | |
| 87 | 89 | }; |
| 14 | 14 | import { Fragment, useCallback, useEffect, useMemo, useRef, useState } from "react"; | |
| 15 | 15 | import { Form, Link } from "react-router"; | |
| 16 | 16 | ||
| 17 | − | import type { Comment, Comparison, DiffLine } from "@g1t/contracts"; | |
| 17 | + | import type { Comment, Comparison, DiffLine, FileDiff } from "@g1t/contracts"; | |
| 18 | 18 | ||
| 19 | + | import { loadFile, peekFile } from "../lib/diff-files"; | |
| 19 | 20 | import { | |
| 21 | + | type Deferred, | |
| 20 | 22 | type FileHtml, | |
| 23 | + | type HighlightedComparison, | |
| 21 | 24 | type HighlightedFile, | |
| 22 | 25 | type HighlightedLine, | |
| 26 | + | LARGE_FILE_LINES, | |
| 23 | 27 | diffContent, | |
| 24 | 28 | htmlOfFile, | |
| 29 | + | linkedFile, | |
| 25 | 30 | withHtml, | |
| 26 | 31 | } from "../lib/diff"; | |
| 27 | 32 | import { Markdown } from "./markdown"; | |
| ⋯ | |||
| 374 | 379 | ); | |
| 375 | 380 | } | |
| 376 | 381 | ||
| 382 | + | /** How tall a line of a diff is (`leading-5`): a file not loaded yet holds its place at its height. */ | |
| 383 | + | const LINE_PX = 20; | |
| 384 | + | ||
| 385 | + | /** Where a file sent without its lines shows them: a note and "Load diff", or room for them while they load. */ | |
| 386 | + | function NotLoaded({ | |
| 387 | + | deferred, | |
| 388 | + | failed, | |
| 389 | + | waiting, | |
| 390 | + | onLoad, | |
| 391 | + | }: { | |
| 392 | + | deferred: Deferred; | |
| 393 | + | failed: boolean; | |
| 394 | + | /** A large file nobody has asked for yet. */ | |
| 395 | + | waiting: boolean; | |
| 396 | + | onLoad: () => void; | |
| 397 | + | }) { | |
| 398 | + | if (failed || waiting) { | |
| 399 | + | return ( | |
| 400 | + | <div className="flex flex-wrap items-center gap-x-3 gap-y-2 px-4 py-4 text-sm text-muted"> | |
| 401 | + | <span className="grow"> | |
| 402 | + | {failed | |
| 403 | + | ? "This file's diff could not be loaded." | |
| 404 | + | : `A large diff (${(deferred.lines ?? 0).toLocaleString("en-US")} lines) is not shown until you ask for it.`} | |
| 405 | + | </span> | |
| 406 | + | <Button type="button" variant="outline" size="sm" onClick={onLoad}> | |
| 407 | + | {failed ? "Try again" : "Load diff"} | |
| 408 | + | </Button> | |
| 409 | + | </div> | |
| 410 | + | ); | |
| 411 | + | } | |
| 412 | + | return ( | |
| 413 | + | <div | |
| 414 | + | aria-busy="true" | |
| 415 | + | className="px-4 py-3 text-xs text-faint" | |
| 416 | + | style={{ height: Math.max(3, Math.min(deferred.lines ?? 3, LARGE_FILE_LINES)) * LINE_PX }} | |
| 417 | + | > | |
| 418 | + | Loading the diff… | |
| 419 | + | </div> | |
| 420 | + | ); | |
| 421 | + | } | |
| 422 | + | ||
| 423 | + | /** How many files waiting after one are fetched with it, and about how many lines that may come to. */ | |
| 424 | + | const AHEAD_FILES = 7; | |
| 425 | + | const AHEAD_LINES = 3_000; | |
| 426 | + | ||
| 427 | + | /** For each file sent without its lines, the ones after it to fetch in the same request. */ | |
| 428 | + | function filesAhead(files: HighlightedFile[]): Map<string, string[]> { | |
| 429 | + | const waiting = files.filter((file) => file.deferred && !file.deferred.large); | |
| 430 | + | const ahead = new Map<string, string[]>(); | |
| 431 | + | waiting.forEach((file, index) => { | |
| 432 | + | const next: string[] = []; | |
| 433 | + | let lines = file.deferred!.lines ?? 0; | |
| 434 | + | for (const later of waiting.slice(index + 1, index + 1 + AHEAD_FILES)) { | |
| 435 | + | // One the comparison left out is counted as a small file. | |
| 436 | + | lines += later.deferred!.lines ?? 100; | |
| 437 | + | if (lines > AHEAD_LINES) break; | |
| 438 | + | next.push(later.path); | |
| 439 | + | } | |
| 440 | + | ahead.set(file.path, next); | |
| 441 | + | }); | |
| 442 | + | return ahead; | |
| 443 | + | } | |
| 444 | + | ||
| 377 | 445 | function FileSection({ | |
| 378 | − | file, | |
| 446 | + | file: given, | |
| 447 | + | source, | |
| 448 | + | wanted, | |
| 449 | + | ahead, | |
| 379 | 450 | review, | |
| 380 | 451 | layout, | |
| 381 | 452 | collapsed, | |
| ⋯ | |||
| 385 | 456 | fileBase, | |
| 386 | 457 | }: { | |
| 387 | 458 | file: HighlightedFile; | |
| 459 | + | /** Where a file sent without its lines fetches them (lib/diff-files.ts). */ | |
| 460 | + | source: string | null; | |
| 461 | + | /** Linked to from the address: its lines load at once, however large. */ | |
| 462 | + | wanted: boolean; | |
| 463 | + | /** The files waiting after it, fetched with it. */ | |
| 464 | + | ahead?: string[]; | |
| 388 | 465 | review?: LineReview; | |
| 389 | 466 | layout: Layout; | |
| 390 | 467 | collapsed: boolean; | |
| ⋯ | |||
| 394 | 471 | /** Where the file can be read as of the change, e.g. `/acme/web/blob/<hash>`. */ | |
| 395 | 472 | fileBase?: string; | |
| 396 | 473 | }) { | |
| 397 | − | const Icon = fileIcon(file.status); | |
| 474 | + | const section = useRef<HTMLElement>(null); | |
| 475 | + | // Sent without its lines (lib/diff.ts firstScreens): fetched as it comes | |
| 476 | + | // near the screen, at once when it has comments or is linked to, and a | |
| 477 | + | // large one when asked for. What was fetched stays through a refresh. | |
| 478 | + | const deferred = given.deferred ?? null; | |
| 479 | + | const fetchKey = deferred && source ? `${source}\n${given.path}` : null; | |
| 480 | + | const [fetched, setFetched] = useState<{ key: string; file: FileDiff | null } | null>(() => { | |
| 481 | + | const hit = fetchKey ? peekFile(source!, given.path) : undefined; | |
| 482 | + | return hit === undefined ? null : { key: fetchKey!, file: hit }; | |
| 483 | + | }); | |
| 484 | + | const [failed, setFailed] = useState(false); | |
| 485 | + | const [asked, setAsked] = useState(false); | |
| 486 | + | const got = fetched && fetched.key === fetchKey ? fetched : null; | |
| 487 | + | const ready = !deferred || got != null; | |
| 488 | + | const file: HighlightedFile = got?.file ?? given; | |
| 398 | 489 | const comments = review?.comments.filter((comment) => comment.path === file.path) ?? []; | |
| 490 | + | const eager = wanted || asked || comments.length > 0; | |
| 491 | + | useEffect(() => { | |
| 492 | + | if (!fetchKey || got || collapsed || failed || !source) return; | |
| 493 | + | if (deferred?.large && !eager) return; | |
| 494 | + | const path = given.path; | |
| 495 | + | const load = () => | |
| 496 | + | loadFile(source, path, { also: ahead }).then( | |
| 497 | + | (found) => setFetched({ key: fetchKey, file: found }), | |
| 498 | + | () => setFailed(true), | |
| 499 | + | ); | |
| 500 | + | if (eager) { | |
| 501 | + | void load(); | |
| 502 | + | return; | |
| 503 | + | } | |
| 504 | + | const element = section.current; | |
| 505 | + | if (!element) return; | |
| 506 | + | const observer = new IntersectionObserver( | |
| 507 | + | ([entry]) => { | |
| 508 | + | if (!entry?.isIntersecting) return; | |
| 509 | + | observer.disconnect(); | |
| 510 | + | void load(); | |
| 511 | + | }, | |
| 512 | + | // Further ahead than the colours: a file's lines are a round trip away. | |
| 513 | + | { rootMargin: "2000px 0px" }, | |
| 514 | + | ); | |
| 515 | + | observer.observe(element); | |
| 516 | + | return () => observer.disconnect(); | |
| 517 | + | // The file's identity is its key; a refresh's new `deferred` object changes nothing. | |
| 518 | + | // eslint-disable-next-line react-hooks/exhaustive-deps | |
| 519 | + | }, [fetchKey, got != null, collapsed, failed, eager, deferred?.large]); | |
| 520 | + | // Linked to: once its lines are in, it is kept at the top while the files | |
| 521 | + | // above it load and change its place, until the reader scrolls or a | |
| 522 | + | // moment has passed. | |
| 523 | + | const linkedReady = wanted && (!deferred || got != null); | |
| 524 | + | useEffect(() => { | |
| 525 | + | const element = section.current; | |
| 526 | + | if (!linkedReady || !element) return; | |
| 527 | + | let timer: ReturnType<typeof setTimeout> | undefined; | |
| 528 | + | const align = () => { | |
| 529 | + | element.scrollIntoView({ block: "start" }); | |
| 530 | + | // Kept there until nothing has moved it for a moment. | |
| 531 | + | clearTimeout(timer); | |
| 532 | + | timer = setTimeout(stop, 1500); | |
| 533 | + | }; | |
| 534 | + | const observer = typeof ResizeObserver === "undefined" ? null : new ResizeObserver(align); | |
| 535 | + | observer?.observe(document.body); | |
| 536 | + | const stop = () => { | |
| 537 | + | observer?.disconnect(); | |
| 538 | + | for (const type of ["wheel", "touchstart", "keydown"]) window.removeEventListener(type, stop); | |
| 539 | + | }; | |
| 540 | + | for (const type of ["wheel", "touchstart", "keydown"]) window.addEventListener(type, stop, { passive: true }); | |
| 541 | + | align(); | |
| 542 | + | return () => { | |
| 543 | + | clearTimeout(timer); | |
| 544 | + | stop(); | |
| 545 | + | }; | |
| 546 | + | }, [linkedReady]); | |
| 547 | + | const Icon = fileIcon(file.status); | |
| 399 | 548 | const shown = new Set(file.hunks.flatMap((hunk) => hunk.lines.map((line) => line.new))); | |
| 400 | − | // Made against an earlier version of the change, on a line no longer in it. | |
| 401 | − | const outdated = comments.filter((comment) => comment.line == null || !shown.has(comment.line)); | |
| 549 | + | // Made against an earlier version of the change, on a line no longer in | |
| 550 | + | // it. Not known until a file sent without its lines has them. | |
| 551 | + | const outdated = ready ? comments.filter((comment) => comment.line == null || !shown.has(comment.line)) : []; | |
| 402 | 552 | // Shown plain at once, and highlighted in the browser when it comes near | |
| 403 | 553 | // the screen: the page never waits for colours. | |
| 404 | − | const section = useRef<HTMLElement>(null); | |
| 405 | 554 | const [highlighted, setHighlighted] = useState<{ content: string; html: FileHtml } | null>(null); | |
| 406 | 555 | // What the colours depend on. A refresh hands the page a new `file` with | |
| 407 | 556 | // the same lines: it keeps its colours rather than going plain and being | |
| ⋯ | |||
| 416 | 565 | const file = latest.current; | |
| 417 | 566 | // Highlighted by the server already: one of the change's first files. | |
| 418 | 567 | const done = file.hunks.some((hunk) => hunk.lines.some((line) => (line as HighlightedLine).html != null)); | |
| 419 | − | if (!element || collapsed || done || kept) return; | |
| 568 | + | if (!element || collapsed || done || kept || file.hunks.length === 0) return; | |
| 420 | 569 | let cancelled = false; | |
| 421 | 570 | const observer = new IntersectionObserver( | |
| 422 | 571 | ([entry]) => { | |
| ⋯ | |||
| 481 | 630 | {comments.length} | |
| 482 | 631 | </span> | |
| 483 | 632 | )} | |
| 484 | − | <Stat additions={file.additions} deletions={file.deletions} /> | |
| 633 | + | {/* Not counted until its lines are read: one the comparison left out. */} | |
| 634 | + | {!(deferred && deferred.lines == null && !ready) && <Stat additions={file.additions} deletions={file.deletions} />} | |
| 485 | 635 | {fileBase && file.status !== "deleted" && ( | |
| 486 | 636 | <span className="hidden items-center gap-0.5 text-xs sm:flex"> | |
| 487 | 637 | <Link to={`${fileBase}/${file.path}`} className="rounded px-1.5 py-0.5 text-muted hover:bg-raised hover:text-fg"> | |
| ⋯ | |||
| 518 | 668 | ))} | |
| 519 | 669 | </div> | |
| 520 | 670 | )} | |
| 521 | − | {file.binary ? ( | |
| 671 | + | {deferred && !ready ? ( | |
| 672 | + | <NotLoaded | |
| 673 | + | deferred={deferred} | |
| 674 | + | failed={failed} | |
| 675 | + | waiting={deferred.large && !eager} | |
| 676 | + | onLoad={() => { | |
| 677 | + | setFailed(false); | |
| 678 | + | setAsked(true); | |
| 679 | + | }} | |
| 680 | + | /> | |
| 681 | + | ) : deferred && got?.file === null ? ( | |
| 682 | + | <p className="px-4 py-6 text-sm text-muted">This file is no longer part of the change; reload the page to see it as it is now.</p> | |
| 683 | + | ) : file.binary ? ( | |
| 522 | 684 | <p className="px-4 py-6 text-sm text-muted">Binary or large file; its contents are not shown.</p> | |
| 523 | 685 | ) : file.hunks.length === 0 ? ( | |
| 524 | 686 | <p className="px-4 py-6 text-sm text-muted">No line changes{file.status === "added" ? ": an empty file" : ""}.</p> | |
| ⋯ | |||
| 633 | 795 | review, | |
| 634 | 796 | empty, | |
| 635 | 797 | fileBase, | |
| 798 | + | repo, | |
| 636 | 799 | }: { | |
| 637 | − | comparison: Comparison; | |
| 800 | + | comparison: Comparison | HighlightedComparison; | |
| 638 | 801 | review?: LineReview; | |
| 639 | 802 | /** What to say when nothing changed. */ | |
| 640 | 803 | empty?: React.ReactNode; | |
| 641 | 804 | /** Where each file can be read as of the change, to link to it. */ | |
| 642 | 805 | fileBase?: string; | |
| 806 | + | /** The repository's address, `/owner/name`: files sent without their lines are fetched from it. */ | |
| 807 | + | repo?: string; | |
| 643 | 808 | }) { | |
| 644 | − | const { files, truncated } = comparison; | |
| 809 | + | const { truncated } = comparison; | |
| 810 | + | const files: HighlightedFile[] = comparison.files; | |
| 811 | + | // The commits compared, so a file fetched later is the same change's. | |
| 812 | + | const source = repo | |
| 813 | + | ? `${repo}/diff-files.json?base=${comparison.base ?? ""}&head=${encodeURIComponent(comparison.head)}` | |
| 814 | + | : null; | |
| 815 | + | // A file linked to (`#file-<path>`) loads at once and opens, wherever it is. | |
| 816 | + | const [wanted, setWanted] = useState<string | null>(null); | |
| 817 | + | const ahead = useMemo(() => filesAhead(files), [files]); | |
| 645 | 818 | const [chosenLayout, setLayout] = useState<Layout>("unified"); | |
| 646 | 819 | // Side by side has no room on a phone. | |
| 647 | 820 | const narrow = useNarrow(); | |
| ⋯ | |||
| 721 | 894 | }, [files, filter]); | |
| 722 | 895 | const tree = useMemo(() => treeOf(shownFiles), [shownFiles]); | |
| 723 | 896 | ||
| 724 | − | const jump = useCallback((path: string) => { | |
| 897 | + | const jump = useCallback((path: string, behavior: ScrollBehavior = "smooth") => { | |
| 725 | 898 | setCollapsed((previous) => { | |
| 726 | 899 | if (!previous.has(path)) return previous; | |
| 727 | 900 | const next = new Set(previous); | |
| ⋯ | |||
| 730 | 903 | }); | |
| 731 | 904 | setActive(path); | |
| 732 | 905 | requestAnimationFrame(() => | |
| 733 | − | document.getElementById(`file-${path}`)?.scrollIntoView({ behavior: "smooth", block: "start" }), | |
| 906 | + | document.getElementById(`file-${path}`)?.scrollIntoView({ behavior, block: "start" }), | |
| 734 | 907 | ); | |
| 735 | 908 | }, []); | |
| 736 | 909 | ||
| 910 | + | // After the viewed files are folded away (above), so a linked one opens. | |
| 911 | + | useEffect(() => { | |
| 912 | + | const follow = () => { | |
| 913 | + | const path = linkedFile(window.location.hash, files); | |
| 914 | + | if (!path) return; | |
| 915 | + | setWanted(path); | |
| 916 | + | // At once: a long smooth scroll would race the files loading on the way. | |
| 917 | + | jump(path, "instant"); | |
| 918 | + | }; | |
| 919 | + | follow(); | |
| 920 | + | window.addEventListener("hashchange", follow); | |
| 921 | + | return () => window.removeEventListener("hashchange", follow); | |
| 922 | + | // The address is read when the page arrives and when it changes, not on every refresh. | |
| 923 | + | // eslint-disable-next-line react-hooks/exhaustive-deps | |
| 924 | + | }, [jump]); | |
| 925 | + | ||
| 737 | 926 | // The file at the top of the screen is the current one. | |
| 738 | 927 | useEffect(() => { | |
| 739 | 928 | const sections = [...document.querySelectorAll<HTMLElement>("[data-diff-file]")]; | |
| ⋯ | |||
| 785 | 974 | const additions = files.reduce((sum, file) => sum + file.additions, 0); | |
| 786 | 975 | const deletions = files.reduce((sum, file) => sum + file.deletions, 0); | |
| 787 | 976 | const allCollapsed = files.every((file) => collapsed.has(file.path)); | |
| 977 | + | // Files the comparison left out have no counts until they are read. | |
| 978 | + | const uncounted = files.filter((file) => file.omitted).length; | |
| 788 | 979 | return ( | |
| 789 | 980 | <div ref={frame}> | |
| 790 | 981 | <div ref={toolbar} className="sticky top-(--topbar-h) z-30 -mx-1 mb-3 flex flex-wrap items-center gap-x-4 gap-y-2 bg-bg/90 px-1 py-2 backdrop-blur"> | |
| ⋯ | |||
| 792 | 983 | <span className="font-medium text-fg">{files.length}</span> {files.length === 1 ? "file" : "files"} | |
| 793 | 984 | </span> | |
| 794 | 985 | <Stat additions={additions} deletions={deletions} /> | |
| 986 | + | {uncounted > 0 && ( | |
| 987 | + | <span className="text-xs text-faint"> | |
| 988 | + | {uncounted} {uncounted === 1 ? "file" : "files"} not counted | |
| 989 | + | </span> | |
| 990 | + | )} | |
| 795 | 991 | <span className="flex items-center gap-2 text-xs text-muted"> | |
| 796 | 992 | <span className="h-1.5 w-20 overflow-hidden rounded-full bg-line"> | |
| 797 | 993 | <span | |
| ⋯ | |||
| 938 | 1134 | <FileSection | |
| 939 | 1135 | key={file.path} | |
| 940 | 1136 | file={file} | |
| 1137 | + | source={source} | |
| 1138 | + | wanted={wanted === file.path} | |
| 1139 | + | ahead={ahead.get(file.path)} | |
| 941 | 1140 | review={review} | |
| 942 | 1141 | layout={layout} | |
| 943 | 1142 | collapsed={collapsed.has(file.path)} | |
| ⋯ | |||
| 948 | 1147 | /> | |
| 949 | 1148 | ))} | |
| 950 | 1149 | {shownFiles.length === 0 && <p className="py-8 text-center text-sm text-muted">No file matches.</p>} | |
| 951 | − | {truncated && <p className="text-center text-sm text-muted">This change is too large to show in full.</p>} | |
| 1150 | + | {truncated && ( | |
| 1151 | + | <p className="text-center text-sm text-muted">More files changed than one page lists: these are the first {files.length}.</p> | |
| 1152 | + | )} | |
| 952 | 1153 | </div> | |
| 953 | 1154 | </div> | |
| 954 | 1155 | </div> | |
| 1 | + | import assert from "node:assert/strict"; | |
| 2 | + | import { afterEach, test } from "node:test"; | |
| 3 | + | ||
| 4 | + | import type { FileDiff } from "@g1t/contracts"; | |
| 5 | + | ||
| 6 | + | import { forgetFiles, loadFile, peekFile } from "./diff-files.ts"; | |
| 7 | + | ||
| 8 | + | const SOURCE = "/acme/web/diff-files.json?base=&head=" + "b".repeat(40); | |
| 9 | + | ||
| 10 | + | const file = (path: string): FileDiff => ({ path, status: "modified", additions: 1, deletions: 0, binary: false, hunks: [] }); | |
| 11 | + | ||
| 12 | + | /** A stand-in for fetch that answers with the paths asked for, and notes each address. */ | |
| 13 | + | function server(fail = false) { | |
| 14 | + | const asked: string[] = []; | |
| 15 | + | const fetcher = (async (url: string) => { | |
| 16 | + | asked.push(url); | |
| 17 | + | if (fail) return new Response("no", { status: 500 }); | |
| 18 | + | const paths = new URL(url, "https://g1t.test").searchParams.getAll("path").filter((path) => path !== "gone.ts"); | |
| 19 | + | return Response.json({ files: paths.map(file) }); | |
| 20 | + | }) as typeof fetch; | |
| 21 | + | return { asked, fetcher }; | |
| 22 | + | } | |
| 23 | + | ||
| 24 | + | afterEach(forgetFiles); | |
| 25 | + | ||
| 26 | + | test("files asked for together are fetched in one request, and once", async () => { | |
| 27 | + | const { asked, fetcher } = server(); | |
| 28 | + | const [a, b, gone] = await Promise.all([ | |
| 29 | + | loadFile(SOURCE, "src/a.ts", { fetcher }), | |
| 30 | + | loadFile(SOURCE, "src/b c.ts", { fetcher }), | |
| 31 | + | loadFile(SOURCE, "gone.ts", { fetcher }), | |
| 32 | + | ]); | |
| 33 | + | assert.equal(asked.length, 1); | |
| 34 | + | assert.ok(asked[0]!.startsWith(SOURCE + "&path=src%2Fa.ts&path=src%2Fb%20c.ts")); | |
| 35 | + | assert.equal(a?.path, "src/a.ts"); | |
| 36 | + | assert.equal(b?.path, "src/b c.ts"); | |
| 37 | + | // No longer in the change: null, not an error. | |
| 38 | + | assert.equal(gone, null); | |
| 39 | + | // Asked again, or drawn again after a refresh: from what was fetched. | |
| 40 | + | assert.equal(await loadFile(SOURCE, "src/a.ts", { fetcher }), a); | |
| 41 | + | assert.equal(peekFile(SOURCE, "src/a.ts"), a); | |
| 42 | + | assert.equal(peekFile(SOURCE, "never.ts"), undefined); | |
| 43 | + | assert.equal(asked.length, 1); | |
| 44 | + | }); | |
| 45 | + | ||
| 46 | + | test("many files at once go 20 to a request", async () => { | |
| 47 | + | const { asked, fetcher } = server(); | |
| 48 | + | await Promise.all(Array.from({ length: 45 }, (_, n) => loadFile(SOURCE, `f${n}`, { fetcher }))); | |
| 49 | + | assert.equal(asked.length, 3); | |
| 50 | + | }); | |
| 51 | + | ||
| 52 | + | test("the files after one come with it, and are not asked for again", async () => { | |
| 53 | + | const { asked, fetcher } = server(); | |
| 54 | + | await loadFile(SOURCE, "a", { also: ["b", "c"], fetcher }); | |
| 55 | + | assert.equal(asked.length, 1); | |
| 56 | + | assert.ok(asked[0]!.endsWith("&path=b&path=c&path=a")); | |
| 57 | + | assert.equal(peekFile(SOURCE, "c")?.path, "c"); | |
| 58 | + | // Reached later, they are there; one already fetched is not asked for again. | |
| 59 | + | assert.equal((await loadFile(SOURCE, "c", { also: ["a", "d"], fetcher }))?.path, "c"); | |
| 60 | + | await loadFile(SOURCE, "d", { fetcher }); | |
| 61 | + | assert.equal(asked.length, 2); | |
| 62 | + | assert.ok(asked[1]!.endsWith("?base=&head=" + "b".repeat(40) + "&path=d")); | |
| 63 | + | }); | |
| 64 | + | ||
| 65 | + | test("a failed fetch is an error, and is tried again when asked again", async () => { | |
| 66 | + | const failing = server(true); | |
| 67 | + | await assert.rejects(loadFile(SOURCE, "src/a.ts", { fetcher: failing.fetcher })); | |
| 68 | + | assert.equal(peekFile(SOURCE, "src/a.ts"), undefined); | |
| 69 | + | const working = server(); | |
| 70 | + | assert.equal((await loadFile(SOURCE, "src/a.ts", { fetcher: working.fetcher }))?.path, "src/a.ts"); | |
| 71 | + | assert.equal(working.asked.length, 1); | |
| 72 | + | }); |
| 1 | + | import type { FileDiff } from "@g1t/contracts"; | |
| 2 | + | ||
| 3 | + | /** | |
| 4 | + | * Fetching the files a large change was sent without (lib/diff.ts | |
| 5 | + | * firstScreens), from routes/repo/diff-files.ts. Files asked for in the | |
| 6 | + | * same moment, as several scroll near together, go in one request, and | |
| 7 | + | * each is fetched once per page: a refresh or a file folded and unfolded | |
| 8 | + | * finds it here. | |
| 9 | + | */ | |
| 10 | + | ||
| 11 | + | /** As many as the route takes in one request. */ | |
| 12 | + | const PER_REQUEST = 20; | |
| 13 | + | /** How long asks are gathered before they are sent. */ | |
| 14 | + | const GATHER_MS = 30; | |
| 15 | + | ||
| 16 | + | type Ask = { path: string; resolve: (file: FileDiff | null) => void; reject: (error: unknown) => void }; | |
| 17 | + | ||
| 18 | + | const loaded = new Map<string, FileDiff | null>(); | |
| 19 | + | const loading = new Map<string, Promise<FileDiff | null>>(); | |
| 20 | + | const queued = new Map<string, Ask[]>(); | |
| 21 | + | let timer: ReturnType<typeof setTimeout> | null = null; | |
| 22 | + | ||
| 23 | + | const keyOf = (source: string, path: string) => `${source}\n${path}`; | |
| 24 | + | ||
| 25 | + | /** A file fetched already, so it is drawn at once rather than after a frame of its placeholder. */ | |
| 26 | + | export function peekFile(source: string, path: string): FileDiff | null | undefined { | |
| 27 | + | return loaded.get(keyOf(source, path)); | |
| 28 | + | } | |
| 29 | + | ||
| 30 | + | /** | |
| 31 | + | * The file at `path` of the change `source` names (the route's address with | |
| 32 | + | * its `base` and `head`), with its lines; null when the change no longer has it. | |
| 33 | + | * The files in `also` (the ones after it) come in the same request, so a | |
| 34 | + | * reader scrolling down finds them fetched rather than asking one by one. | |
| 35 | + | */ | |
| 36 | + | export function loadFile( | |
| 37 | + | source: string, | |
| 38 | + | path: string, | |
| 39 | + | { also = [], fetcher = fetch }: { also?: string[]; fetcher?: typeof fetch } = {}, | |
| 40 | + | ): Promise<FileDiff | null> { | |
| 41 | + | const key = keyOf(source, path); | |
| 42 | + | // Fetched already, or on its way (with the file before it): nothing to send. | |
| 43 | + | if (loaded.has(key)) return Promise.resolve(loaded.get(key)!); | |
| 44 | + | if (loading.has(key)) return loading.get(key)!; | |
| 45 | + | for (const next of also) { | |
| 46 | + | if (!loaded.has(keyOf(source, next))) void ask(source, next, fetcher).catch(() => {}); | |
| 47 | + | } | |
| 48 | + | return ask(source, path, fetcher); | |
| 49 | + | } | |
| 50 | + | ||
| 51 | + | function ask(source: string, path: string, fetcher: typeof fetch): Promise<FileDiff | null> { | |
| 52 | + | const key = keyOf(source, path); | |
| 53 | + | if (loaded.has(key)) return Promise.resolve(loaded.get(key)!); | |
| 54 | + | const pending = loading.get(key); | |
| 55 | + | if (pending) return pending; | |
| 56 | + | const promise = new Promise<FileDiff | null>((resolve, reject) => { | |
| 57 | + | const asks = queued.get(source) ?? []; | |
| 58 | + | asks.push({ path, resolve, reject }); | |
| 59 | + | queued.set(source, asks); | |
| 60 | + | timer ??= setTimeout(() => flush(fetcher), GATHER_MS); | |
| 61 | + | }).then( | |
| 62 | + | (file) => { | |
| 63 | + | loaded.set(key, file); | |
| 64 | + | loading.delete(key); | |
| 65 | + | return file; | |
| 66 | + | }, | |
| 67 | + | (error: unknown) => { | |
| 68 | + | // Asked again, it is fetched again. | |
| 69 | + | loading.delete(key); | |
| 70 | + | throw error; | |
| 71 | + | }, | |
| 72 | + | ); | |
| 73 | + | loading.set(key, promise); | |
| 74 | + | return promise; | |
| 75 | + | } | |
| 76 | + | ||
| 77 | + | function flush(fetcher: typeof fetch) { | |
| 78 | + | timer = null; | |
| 79 | + | const all = [...queued]; | |
| 80 | + | queued.clear(); | |
| 81 | + | for (const [source, asks] of all) { | |
| 82 | + | for (let start = 0; start < asks.length; start += PER_REQUEST) { | |
| 83 | + | const batch = asks.slice(start, start + PER_REQUEST); | |
| 84 | + | const url = `${source}${source.includes("?") ? "&" : "?"}${batch.map((ask) => `path=${encodeURIComponent(ask.path)}`).join("&")}`; | |
| 85 | + | fetcher(url, { headers: { accept: "application/json" } }) | |
| 86 | + | .then(async (response) => { | |
| 87 | + | if (!response.ok) throw new Error(`The diff could not be loaded (${response.status}).`); | |
| 88 | + | const { files } = (await response.json()) as { files: FileDiff[] }; | |
| 89 | + | const byPath = new Map(files.map((file) => [file.path, file])); | |
| 90 | + | for (const ask of batch) ask.resolve(byPath.get(ask.path) ?? null); | |
| 91 | + | }) | |
| 92 | + | .catch((error: unknown) => { | |
| 93 | + | for (const ask of batch) ask.reject(error); | |
| 94 | + | }); | |
| 95 | + | } | |
| 96 | + | } | |
| 97 | + | } | |
| 98 | + | ||
| 99 | + | /** For tests: forget everything fetched. */ | |
| 100 | + | export function forgetFiles() { | |
| 101 | + | loaded.clear(); | |
| 102 | + | loading.clear(); | |
| 103 | + | queued.clear(); | |
| 104 | + | if (timer) clearTimeout(timer); | |
| 105 | + | timer = null; | |
| 106 | + | } |
| 3 | 3 | ||
| 4 | 4 | import type { FileDiff } from "@g1t/contracts"; | |
| 5 | 5 | ||
| 6 | − | import { diffContent, htmlOfFile, withHtml } from "./diff.ts"; | |
| 6 | + | import { diffContent, firstScreens, htmlOfFile, linkedFile, withHtml } from "./diff.ts"; | |
| 7 | 7 | import { highlightFile } from "./shiki.ts"; | |
| 8 | 8 | ||
| 9 | 9 | const FILE: FileDiff = { | |
| ⋯ | |||
| 69 | 69 | }; | |
| 70 | 70 | assert.notEqual(diffContent(one), diffContent(two)); | |
| 71 | 71 | }); | |
| 72 | + | ||
| 73 | + | /** A file of `lines` added lines. */ | |
| 74 | + | function sized(path: string, lines: number): FileDiff { | |
| 75 | + | return { | |
| 76 | + | path, | |
| 77 | + | status: "added", | |
| 78 | + | additions: lines, | |
| 79 | + | deletions: 0, | |
| 80 | + | binary: false, | |
| 81 | + | hunks: [{ lines: Array.from({ length: lines }, (_, n) => ({ kind: "add" as const, old: null, new: n + 1, text: `line ${n}` })) }], | |
| 82 | + | }; | |
| 83 | + | } | |
| 84 | + | ||
| 85 | + | const comparisonOf = (files: FileDiff[]) => ({ base: "a".repeat(40), head: "b".repeat(40), files, truncated: false }); | |
| 86 | + | ||
| 87 | + | test("a small change is sent whole", () => { | |
| 88 | + | const files = [sized("a", 10), sized("b", 300), FILE]; | |
| 89 | + | const sent = firstScreens(comparisonOf(files)); | |
| 90 | + | assert.deepEqual(sent.files, files); | |
| 91 | + | assert.ok(sent.files.every((file) => !file.deferred)); | |
| 92 | + | }); | |
| 93 | + | ||
| 94 | + | test("past the first screens' lines, files are sent with their counts but not their lines", () => { | |
| 95 | + | const files = Array.from({ length: 10 }, (_, n) => sized(`f${n}`, 400)); | |
| 96 | + | const sent = firstScreens(comparisonOf(files), { budget: 2_000 }); | |
| 97 | + | // 5 × 400 lines fit in 2,000; the rest wait, in order. | |
| 98 | + | assert.deepEqual( | |
| 99 | + | sent.files.map((file) => file.hunks.length > 0), | |
| 100 | + | [true, true, true, true, true, false, false, false, false, false], | |
| 101 | + | ); | |
| 102 | + | const later = sent.files[7]!; | |
| 103 | + | assert.deepEqual(later.deferred, { lines: 400, large: false }); | |
| 104 | + | assert.equal(later.additions, 400); | |
| 105 | + | assert.equal(later.path, "f7"); | |
| 106 | + | // Once one does not fit, a smaller one after it waits too: what is drawn is the top of the change. | |
| 107 | + | const mixed = firstScreens(comparisonOf([sized("a", 800), sized("b", 900), sized("c", 10)]), { budget: 1_000 }); | |
| 108 | + | assert.deepEqual(mixed.files.map((file) => file.deferred?.lines ?? null), [null, 900, 10]); | |
| 109 | + | }); | |
| 110 | + | ||
| 111 | + | test("a very large file waits to be asked for, and does not hold back the files after it", () => { | |
| 112 | + | const sent = firstScreens(comparisonOf([sized("package-lock.json", 5_000), sized("src/a.ts", 20)])); | |
| 113 | + | assert.deepEqual(sent.files[0]!.deferred, { lines: 5_000, large: true }); | |
| 114 | + | assert.equal(sent.files[0]!.hunks.length, 0); | |
| 115 | + | assert.equal(sent.files[1]!.deferred, undefined); | |
| 116 | + | assert.equal(sent.files[1]!.hunks.length, 1); | |
| 117 | + | }); | |
| 118 | + | ||
| 119 | + | test("files with comments keep their lines wherever they are, however large", () => { | |
| 120 | + | const files = [sized("a", 900), sized("b", 400), sized("lock", 3_000), sized("c", 400)]; | |
| 121 | + | const sent = firstScreens(comparisonOf(files), { budget: 1_000, eager: ["c", "lock"] }); | |
| 122 | + | assert.deepEqual( | |
| 123 | + | sent.files.map((file) => file.deferred ?? null), | |
| 124 | + | [null, { lines: 400, large: false }, null, null], | |
| 125 | + | ); | |
| 126 | + | }); | |
| 127 | + | ||
| 128 | + | test("files the comparison left out wait with no counts; binary files are as they were", () => { | |
| 129 | + | const omitted: FileDiff = { ...sized("z", 0), hunks: [], additions: 0, omitted: true }; | |
| 130 | + | const binary: FileDiff = { ...sized("logo.png", 0), hunks: [], binary: true }; | |
| 131 | + | const sent = firstScreens(comparisonOf([omitted, binary])); | |
| 132 | + | assert.deepEqual(sent.files[0]!.deferred, { lines: null, large: false }); | |
| 133 | + | assert.equal(sent.files[1]!.deferred, undefined); | |
| 134 | + | assert.equal(sent.files[1]!.binary, true); | |
| 135 | + | }); | |
| 136 | + | ||
| 137 | + | test("a link to a file names it by path, encoded or not, and only a file of the change", () => { | |
| 138 | + | const files = [{ path: "src/a b.ts" }, { path: "README.md" }]; | |
| 139 | + | assert.equal(linkedFile("#file-README.md", files), "README.md"); | |
| 140 | + | assert.equal(linkedFile("#diff-src/a%20b.ts", files), "src/a b.ts"); | |
| 141 | + | assert.equal(linkedFile("#file-src/a b.ts", files), "src/a b.ts"); | |
| 142 | + | assert.equal(linkedFile("#file-missing.ts", files), null); | |
| 143 | + | assert.equal(linkedFile("#readme", files), null); | |
| 144 | + | assert.equal(linkedFile("", files), null); | |
| 145 | + | assert.equal(linkedFile("#file-%E0%A4%A", files), null); | |
| 146 | + | }); | |
| 2 | 2 | ||
| 3 | 3 | /** A diff line, with its text as highlighted HTML when the language is known. */ | |
| 4 | 4 | export type HighlightedLine = DiffLine & { html?: string }; | |
| 5 | − | export type HighlightedFile = Omit<FileDiff, "hunks"> & { hunks: { lines: HighlightedLine[] }[] }; | |
| 5 | + | export type HighlightedFile = Omit<FileDiff, "hunks"> & { hunks: { lines: HighlightedLine[] }[]; deferred?: Deferred }; | |
| 6 | 6 | export type HighlightedComparison = Omit<Comparison, "files"> & { files: HighlightedFile[] }; | |
| 7 | 7 | ||
| 8 | + | /** | |
| 9 | + | * A file sent without its lines, which the page fetches when it gets near | |
| 10 | + | * (components/diff-view.tsx): `lines` is how many it has, null when the | |
| 11 | + | * comparison did not read them (`omitted`); a `large` one waits to be asked | |
| 12 | + | * for. | |
| 13 | + | */ | |
| 14 | + | export type Deferred = { lines: number | null; large: boolean }; | |
| 15 | + | ||
| 16 | + | /** How many diff lines a page is sent with: about its first screens. */ | |
| 17 | + | export const FIRST_LINES = 2_000; | |
| 18 | + | /** Past this a file's lines wait for "Load diff": generated files, lockfiles. */ | |
| 19 | + | export const LARGE_FILE_LINES = 1_000; | |
| 20 | + | ||
| 21 | + | /** How many lines a file's diff has. */ | |
| 22 | + | export function linesOf(file: FileDiff): number { | |
| 23 | + | return file.hunks.reduce((sum, hunk) => sum + hunk.lines.length, 0); | |
| 24 | + | } | |
| 25 | + | ||
| 26 | + | /** | |
| 27 | + | * A comparison as a page is sent it: the first files with their lines, | |
| 28 | + | * until about `budget` lines, and every file after that, and any very large | |
| 29 | + | * one, with its path and counts only (`deferred`), so a pull request of | |
| 30 | + | * hundreds of files is not drawn, sent and hydrated all at once. Files | |
| 31 | + | * named in `eager` (ones with comments on their lines) keep their lines | |
| 32 | + | * wherever they are. The list itself is whole: the tree and counts show at once. | |
| 33 | + | */ | |
| 34 | + | export function firstScreens( | |
| 35 | + | comparison: Comparison | HighlightedComparison, | |
| 36 | + | { eager = [], budget = FIRST_LINES, large = LARGE_FILE_LINES }: { eager?: Iterable<string>; budget?: number; large?: number } = {}, | |
| 37 | + | ): HighlightedComparison { | |
| 38 | + | const wanted = new Set(eager); | |
| 39 | + | let spent = 0; | |
| 40 | + | let full = false; | |
| 41 | + | const files = comparison.files.map((file): HighlightedFile => { | |
| 42 | + | if (file.omitted) return { ...file, hunks: [], deferred: { lines: null, large: false } }; | |
| 43 | + | if (file.binary || file.hunks.length === 0) return file; | |
| 44 | + | const lines = linesOf(file); | |
| 45 | + | if (wanted.has(file.path)) { | |
| 46 | + | spent += lines; | |
| 47 | + | return file; | |
| 48 | + | } | |
| 49 | + | if (lines > large) return { ...file, hunks: [], deferred: { lines, large: true } }; | |
| 50 | + | // In order, so what is drawn is the top of the change; once one does | |
| 51 | + | // not fit, the rest wait too. | |
| 52 | + | if (!full && spent + lines <= budget) { | |
| 53 | + | spent += lines; | |
| 54 | + | return file; | |
| 55 | + | } | |
| 56 | + | full = true; | |
| 57 | + | return { ...file, hunks: [], deferred: { lines, large: false } }; | |
| 58 | + | }); | |
| 59 | + | return { ...comparison, files }; | |
| 60 | + | } | |
| 61 | + | ||
| 62 | + | /** The file a page's address points to, as `#file-<path>` (or `#diff-<path>`), if it is one of `files`. */ | |
| 63 | + | export function linkedFile(hash: string, files: { path: string }[]): string | null { | |
| 64 | + | const match = /^#(?:file|diff)-(.+)$/.exec(hash); | |
| 65 | + | if (!match) return null; | |
| 66 | + | let path = match[1]!; | |
| 67 | + | try { | |
| 68 | + | path = decodeURIComponent(path); | |
| 69 | + | } catch { | |
| 70 | + | // Not encoded: as it is. | |
| 71 | + | } | |
| 72 | + | return files.some((file) => file.path === path) ? path : null; | |
| 73 | + | } | |
| 74 | + | ||
| 8 | 75 | /** Each hunk's lines' HTML, null for a line without any: what a highlighted file adds to its diff. */ | |
| 9 | 76 | export type FileHtml = (string | null)[][]; | |
| 10 | 77 |
| 45 | 45 | /** | |
| 46 | 46 | * About 8 MB of highlighted lines per isolate (two bytes a character); one | |
| 47 | 47 | * file up to a quarter of that, which a 1,800-line file fits. A pull | |
| 48 | − | * request's first screens are at most 40,000 characters of text, about | |
| 49 | − | * ten times that highlighted. | |
| 48 | + | * request's first screens are at most 16,000 characters of text, about | |
| 49 | + | * four times that highlighted. | |
| 50 | 50 | */ | |
| 51 | 51 | const linesCache = contentCache<string[]>({ | |
| 52 | 52 | name: "highlight-lines", | |
| ⋯ | |||
| 89 | 89 | * How much of a change is highlighted before the page is sent: its first | |
| 90 | 90 | * screens. The rest is highlighted in the browser as it comes near | |
| 91 | 91 | * (components/diff-view.tsx), so a large change never holds the page up. | |
| 92 | + | * Highlighted HTML is about four times its text, and is sent twice (drawn, | |
| 93 | + | * and in the data the page hydrates from), so this is kept to a few | |
| 94 | + | * screens: about 400 lines. | |
| 92 | 95 | */ | |
| 93 | − | const FIRST_SCREENS_CHARS = 40_000; | |
| 96 | + | const FIRST_SCREENS_CHARS = 16_000; | |
| 94 | 97 | ||
| 95 | 98 | /** A file's diff highlighted, from the cache when the same lines were highlighted before. */ | |
| 96 | 99 | async function highlightFileCached(file: Comparison["files"][number]): Promise<HighlightedFile | null> { | |
| ⋯ | |||
| 110 | 113 | * the budget runs out, so what is on screen when the page arrives already | |
| 111 | 114 | * has its colours. | |
| 112 | 115 | */ | |
| 113 | − | export async function highlightFirstFiles(comparison: Comparison): Promise<HighlightedComparison> { | |
| 116 | + | export async function highlightFirstFiles(comparison: Comparison | HighlightedComparison): Promise<HighlightedComparison> { | |
| 114 | 117 | let budget = FIRST_SCREENS_CHARS; | |
| 115 | 118 | const files: HighlightedFile[] = []; | |
| 116 | 119 | for (const file of comparison.files) { | |
| 120 | + | // Sent without its lines (lib/diff.ts firstScreens): nothing to colour. | |
| 121 | + | if (file.hunks.length === 0) { | |
| 122 | + | files.push(file); | |
| 123 | + | continue; | |
| 124 | + | } | |
| 117 | 125 | const size = file.hunks.reduce((sum, hunk) => sum + hunk.lines.reduce((n, line) => n + line.text.length + 1, 0), 0); | |
| 118 | 126 | if (budget <= 0 || size > budget || file.binary) { | |
| 119 | 127 | // Past the first screens: everything after it waits for the browser. | |
| 221 | 221 | ]), | |
| 222 | 222 | // Why a line is the way it is, fetched by the blame view. | |
| 223 | 223 | route(":owner/:repo/why/:hash", "routes/repo/why.ts"), | |
| 224 | + | // Files of a large change with their lines, fetched as the diff view reaches them. | |
| 225 | + | route(":owner/:repo/diff-files.json", "routes/repo/diff-files.ts"), | |
| 224 | 226 | // Its branch names, for changing the branch a pull request merges into. | |
| 225 | 227 | route(":owner/:repo/branches.json", "routes/repo/branch-names.ts"), | |
| 226 | 228 | // An invitation to a repository, answered by someone who cannot see it yet. |
| 5 | 5 | import type { Comparison, Pull } from "@g1t/contracts"; | |
| 6 | 6 | ||
| 7 | 7 | import { parseCommitMessage } from "../../lib/commit-message"; | |
| 8 | + | import { firstScreens } from "../../lib/diff"; | |
| 8 | 9 | import type { Route } from "./+types/commit"; | |
| 9 | 10 | import { page } from "../../lib/meta"; | |
| 10 | 11 | import { CommitChecksBadge } from "../../components/commit-checks"; | |
| ⋯ | |||
| 75 | 76 | return data( | |
| 76 | 77 | { | |
| 77 | 78 | ...loaded, | |
| 79 | + | // Its first screens with their lines; the rest load as they are reached. | |
| 80 | + | comparison: firstScreens(loaded.comparison), | |
| 78 | 81 | commit, | |
| 79 | 82 | // Streamed: the page shows the commit while this is worked out. | |
| 80 | 83 | pull: pullForCommit(path, viewer, loaded.commit.hash), | |
| ⋯ | |||
| 211 | 214 | </section> | |
| 212 | 215 | </Card> | |
| 213 | 216 | <div className="mt-6"> | |
| 214 | − | <DiffView comparison={comparison} empty="This commit changes no files." fileBase={`${base}/blob/${commit.hash}`} /> | |
| 217 | + | <DiffView comparison={comparison} repo={base} empty="This commit changes no files." fileBase={`${base}/blob/${commit.hash}`} /> | |
| 215 | 218 | </div> | |
| 216 | 219 | </div> | |
| 217 | 220 | ); | |
| 1 | 1 | import { ArrowLeft, GitCommitHorizontal, GitPullRequest } from "lucide-react"; | |
| 2 | 2 | import { Form, Link } from "react-router"; | |
| 3 | 3 | ||
| 4 | − | import type { Comparison } from "@g1t/contracts"; | |
| 5 | − | ||
| 6 | 4 | import type { Route } from "./+types/compare"; | |
| 7 | 5 | import { DiffView } from "../../components/diff-view"; | |
| 8 | 6 | import { CommitAvatars, CommitNames } from "../../components/commit-person"; | |
| ⋯ | |||
| 11 | 9 | import { Card } from "../../components/ui/card"; | |
| 12 | 10 | import { Hint } from "../../components/ui/hint"; | |
| 13 | 11 | import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from "../../components/ui/select"; | |
| 12 | + | import { type HighlightedComparison, firstScreens } from "../../lib/diff"; | |
| 14 | 13 | import { page } from "../../lib/meta"; | |
| 15 | 14 | import { showCommits } from "../../lib/commit-people.server"; | |
| 16 | 15 | import { repos } from "../../lib/services.server"; | |
| ⋯ | |||
| 69 | 68 | // that point is past what was read, the two tips are compared. | |
| 70 | 69 | const mergeBase = shared === -1 ? base : headLog.value[shared]!.hash; | |
| 71 | 70 | const compared = await repos.compare(found.id, viewer, mergeBase, head); | |
| 72 | − | const comparison: Comparison | null = compared.ok ? compared.value : null; | |
| 71 | + | // Its first screens with their lines; the rest load as they are reached. | |
| 72 | + | const comparison: HighlightedComparison | null = compared.ok ? firstScreens(compared.value) : null; | |
| 73 | 73 | return { base, head, branches, comparison, commits, same: false }; | |
| 74 | 74 | } | |
| 75 | 75 | ||
| ⋯ | |||
| 167 | 167 | </Card> | |
| 168 | 168 | </section> | |
| 169 | 169 | {comparison ? ( | |
| 170 | − | <DiffView comparison={comparison} fileBase={`${repoBase}/blob/${encodeURIComponent(head)}`} empty="The two sides have the same files." /> | |
| 170 | + | <DiffView comparison={comparison} repo={repoBase} fileBase={`${repoBase}/blob/${encodeURIComponent(head)}`} empty="The two sides have the same files." /> | |
| 171 | 171 | ) : ( | |
| 172 | 172 | <p className="text-sm text-muted">The changes could not be read just now. The commits above are what {head} adds.</p> | |
| 173 | 173 | )} | |
| 1 | + | /** | |
| 2 | + | * Some files of a change with their lines, fetched by the diff view as it | |
| 3 | + | * reaches files a large pull request, commit or comparison was sent | |
| 4 | + | * without (lib/diff.ts firstScreens, components/diff-view.tsx). | |
| 5 | + | * | |
| 6 | + | * `base` and `head` are the commits the page compared, so the answer never | |
| 7 | + | * changes: it is kept in the data centre's cache once the viewer is known | |
| 8 | + | * to read the repository, and the browser keeps it too. | |
| 9 | + | */ | |
| 10 | + | import { data } from "react-router"; | |
| 11 | + | ||
| 12 | + | import type { FileDiff } from "@g1t/contracts"; | |
| 13 | + | ||
| 14 | + | import type { Route } from "./+types/diff-files"; | |
| 15 | + | import { immutable } from "../../lib/immutable.server"; | |
| 16 | + | import { repos } from "../../lib/services.server"; | |
| 17 | + | import { getViewer, unwrap } from "../../lib/session.server"; | |
| 18 | + | ||
| 19 | + | const COMMIT = /^[0-9a-f]{40}$/; | |
| 20 | + | /** The most files one request reads; the repos service reads no more than 50. */ | |
| 21 | + | export const MAX_PATHS = 20; | |
| 22 | + | ||
| 23 | + | export async function loader({ params, context, request }: Route.LoaderArgs) { | |
| 24 | + | const url = new URL(request.url); | |
| 25 | + | const head = url.searchParams.get("head") ?? ""; | |
| 26 | + | const base = url.searchParams.get("base") ?? ""; | |
| 27 | + | const paths = [...new Set(url.searchParams.getAll("path"))].sort(); | |
| 28 | + | if (!COMMIT.test(head) || (base !== "" && !COMMIT.test(base)) || paths.length === 0 || paths.length > MAX_PATHS) { | |
| 29 | + | throw data({ error: "Give head (and base) as full commit hashes and 1 to 20 paths." }, { status: 400 }); | |
| 30 | + | } | |
| 31 | + | const viewer = getViewer(context); | |
| 32 | + | const repo = unwrap(await repos.get({ namespace: params.owner, name: params.repo }, viewer)); | |
| 33 | + | // With no base the head is compared with its first parent, as the page | |
| 34 | + | // that sent the file did: a commit's own page, or a change with no | |
| 35 | + | // shared history. | |
| 36 | + | const files = await immutable(`diff-files:${repo.id}:${base}:${head}:${paths.join("\n")}`, async () => { | |
| 37 | + | const compared = await repos.compare(repo.id, viewer, base || null, head, null, paths); | |
| 38 | + | return compared.ok ? compared.value.files : null; | |
| 39 | + | }); | |
| 40 | + | if (!files) throw data({ error: "Those files could not be compared." }, { status: 404 }); | |
| 41 | + | return Response.json({ files } satisfies { files: FileDiff[] }, { | |
| 42 | + | headers: { "cache-control": "private, max-age=31536000, immutable" }, | |
| 43 | + | }); | |
| 44 | + | } |
| 41 | 41 | } from "@g1t/contracts"; | |
| 42 | 42 | ||
| 43 | 43 | import type { Route } from "./+types/pull"; | |
| 44 | + | import { firstScreens } from "../../lib/diff"; | |
| 44 | 45 | import { highlightFirstFiles } from "../../lib/highlight.server"; | |
| 45 | 46 | import { excerpt, page } from "../../lib/meta"; | |
| 46 | 47 | import { openedBy } from "../../lib/opened-by"; | |
| ⋯ | |||
| 194 | 195 | (found.value.statuses ?? []).length === 0 && | |
| 195 | 196 | (found.value.requiredChecks ?? []).length === 0; | |
| 196 | 197 | const [comparison, noChecks] = await Promise.all([ | |
| 197 | − | // The first screens of it highlighted here, so their colours do not pop in. | |
| 198 | + | // Its first screens sent with their lines, and files with comments on | |
| 199 | + | // their lines; the rest load as the reader reaches them. What is sent is | |
| 200 | + | // highlighted here, so its colours do not pop in. | |
| 198 | 201 | tab === "changes" | |
| 199 | 202 | ? repos | |
| 200 | 203 | .compare(range.repoId, viewer, range.base, range.head, range.baseBranch) | |
| 201 | − | .then(async (found) => (found.ok ? { ok: true as const, value: await highlightFirstFiles(found.value) } : found)) | |
| 204 | + | .then(async (compared) => | |
| 205 | + | compared.ok | |
| 206 | + | ? { | |
| 207 | + | ok: true as const, | |
| 208 | + | value: await highlightFirstFiles( | |
| 209 | + | firstScreens(compared.value, { eager: found.value.comments.flatMap((comment) => (comment.path ? [comment.path] : [])) }), | |
| 210 | + | ), | |
| 211 | + | } | |
| 212 | + | : compared, | |
| 213 | + | ) | |
| 202 | 214 | : null, | |
| 203 | 215 | unchecked | |
| 204 | 216 | ? actions | |
| ⋯ | |||
| 938 | 950 | {comparison ? ( | |
| 939 | 951 | <DiffView | |
| 940 | 952 | comparison={comparison} | |
| 953 | + | repo={base} | |
| 941 | 954 | review={{ | |
| 942 | 955 | comments: comments.filter((comment) => comment.path), | |
| 943 | 956 | canComment: Boolean(viewer), | |
| 477 | 477 | /// True when the file is binary or too large, so no lines are shown. | |
| 478 | 478 | pub binary: bool, | |
| 479 | 479 | pub hunks: Vec<Hunk>, | |
| 480 | + | /// True when the comparison reached its limit on lines before this | |
| 481 | + | /// file, so it is listed without its lines (and with no counts): ask | |
| 482 | + | /// `compare` again with its path in `paths` to have them. | |
| 483 | + | #[serde(default, skip_serializing_if = "std::ops::Not::not")] | |
| 484 | + | pub omitted: bool, | |
| 480 | 485 | } | |
| 481 | 486 | ||
| 482 | 487 | /// What changed between two commits. | |
| ⋯ | |||
| 486 | 491 | pub base: Option<String>, | |
| 487 | 492 | pub head: String, | |
| 488 | 493 | pub files: Vec<FileDiff>, | |
| 489 | − | /// True when the change was too large to return in full. | |
| 494 | + | /// True when more files changed than are listed. A listed file whose | |
| 495 | + | /// lines were left out says so itself (`FileDiff::omitted`). | |
| 490 | 496 | pub truncated: bool, | |
| 491 | 497 | } | |
| 492 | 498 | ||
| ⋯ | |||
| 511 | 517 | /// another branch is compared this way. | |
| 512 | 518 | #[serde(default)] | |
| 513 | 519 | pub base_branch: Option<String>, | |
| 520 | + | /// Only these files, with their lines: how a file left out of a large | |
| 521 | + | /// comparison is read on its own. At most 50. | |
| 522 | + | #[serde(default)] | |
| 523 | + | pub paths: Option<Vec<String>>, | |
| 514 | 524 | } | |
| 515 | 525 | ||
| 516 | 526 | /// Lines `start` to `end` of a file, inclusive and counted from 1, last | |
| 561 | 561 | base: self.merge_base.clone(), | |
| 562 | 562 | head, | |
| 563 | 563 | base_branch: self.base.clone().filter(|base| !base.is_empty()), | |
| 564 | + | paths: None, | |
| 564 | 565 | } | |
| 565 | 566 | } | |
| 566 | 567 | } |
| 408 | 408 | rawFile: (repoId, ref, path, maxBytes) => call("raw_file", { repoId, ref, path, maxBytes }), | |
| 409 | 409 | commitFile: (repo, actor, file) => call("commit_file", { repo, actor, ...file }), | |
| 410 | 410 | land: (sourceId, actor, branch) => call("land", { sourceId, actor, branch }), | |
| 411 | − | compare: (repoId, viewer, base, head, baseBranch) => call("compare", { repoId, viewer, base, head, baseBranch }), | |
| 411 | + | compare: (repoId, viewer, base, head, baseBranch, paths) => call("compare", { repoId, viewer, base, head, baseBranch, paths: paths ?? null }), | |
| 412 | 412 | claimBackups: (limit, maxRunning) => call("claim_backups", { limit, maxRunning }), | |
| 413 | 413 | // The sandbox's own calls are snake_case (they come through the API). | |
| 414 | 414 | failBackup: (jobId, token, error) => call("backup_fail", { job_id: jobId, token, error }), |
| 349 | 349 | */ | |
| 350 | 350 | /** | |
| 351 | 351 | * With no `base`, a branch is compared from where it left `baseBranch` | |
| 352 | − | * (the default branch when absent). | |
| 352 | + | * (the default branch when absent). With `paths`, only those files (at | |
| 353 | + | * most 50), with their lines: how a file a large comparison lists as | |
| 354 | + | * `omitted` is read. | |
| 353 | 355 | */ | |
| 354 | 356 | compare( | |
| 355 | 357 | repoId: string, | |
| ⋯ | |||
| 357 | 359 | base?: string | null, | |
| 358 | 360 | head?: string | null, | |
| 359 | 361 | baseBranch?: string | null, | |
| 362 | + | paths?: string[] | null, | |
| 360 | 363 | ): Promise<Result<Comparison>>; | |
| 361 | 364 | ||
| 362 | 365 | /** | |
| ⋯ | |||
| 480 | 483 | /** True when the file is binary or too large, so no lines are shown. */ | |
| 481 | 484 | binary: boolean; | |
| 482 | 485 | hunks: Hunk[]; | |
| 486 | + | /** | |
| 487 | + | * True when the comparison reached its limit on lines before this file, | |
| 488 | + | * so it is listed without its lines or counts: `compare` with its path | |
| 489 | + | * in `paths` has them. | |
| 490 | + | */ | |
| 491 | + | omitted?: boolean; | |
| 483 | 492 | }; | |
| 484 | 493 | ||
| 485 | 494 | /** What changed between two commits. */ | |
| ⋯ | |||
| 487 | 496 | base: string | null; | |
| 488 | 497 | head: string; | |
| 489 | 498 | files: FileDiff[]; | |
| 490 | − | /** True when the change was too large to return in full. */ | |
| 499 | + | /** True when more files changed than are listed. */ | |
| 491 | 500 | truncated: boolean; | |
| 492 | 501 | }; | |
| 236 | 236 | base, | |
| 237 | 237 | head: Some(head), | |
| 238 | 238 | base_branch: None, | |
| 239 | + | paths: None, | |
| 239 | 240 | }, | |
| 240 | 241 | ) | |
| 241 | 242 | .await?; |
| 200 | 200 | } | |
| 201 | 201 | ||
| 202 | 202 | /// The files that differ between two trees, with their line changes. | |
| 203 | − | /// Returns the files and whether the result was cut short. | |
| 203 | + | /// Returns the files and whether the list of files was cut short. | |
| 204 | + | /// | |
| 205 | + | /// Files are read in order until about `MAX_LINES` lines have been read; | |
| 206 | + | /// the ones after that are listed without their lines, marked `omitted`, | |
| 207 | + | /// and are read by asking for them by path (`only`), which reads just them. | |
| 204 | 208 | pub async fn compare_trees<R: GitRepo>( | |
| 205 | 209 | repo: &R, | |
| 206 | 210 | old_tree: Option<&str>, | |
| 207 | 211 | new_tree: &str, | |
| 212 | + | only: Option<&[String]>, | |
| 208 | 213 | ) -> Result<(Vec<FileDiff>, bool)> { | |
| 209 | − | let (changes, mut truncated) = changed_files(repo, old_tree, new_tree).await?; | |
| 214 | + | let (mut changes, truncated) = changed_files(repo, old_tree, new_tree).await?; | |
| 215 | + | if let Some(only) = only { | |
| 216 | + | changes.retain(|change| only.contains(&change.path)); | |
| 217 | + | } | |
| 210 | 218 | let mut files = Vec::with_capacity(changes.len()); | |
| 211 | 219 | let mut lines = 0; | |
| 212 | 220 | // Contents are read a batch at a time, all of a batch at once. | |
| ⋯ | |||
| 239 | 247 | (_, None) => FileStatus::Deleted, | |
| 240 | 248 | _ => FileStatus::Modified, | |
| 241 | 249 | }; | |
| 242 | − | let texts = match texts_read.next() { | |
| 243 | − | Some(texts) if lines < MAX_LINES => texts, | |
| 244 | − | _ => { | |
| 245 | − | truncated = true; | |
| 246 | − | None | |
| 247 | − | } | |
| 250 | + | // Past the limit (not read at all), or read: its two sides, or | |
| 251 | + | // nothing when it is binary or too large. | |
| 252 | + | let (texts, omitted) = match texts_read.next() { | |
| 253 | + | Some(texts) if lines < MAX_LINES => (texts, false), | |
| 254 | + | _ => (None, true), | |
| 248 | 255 | }; | |
| 249 | 256 | let (hunks, additions, deletions, binary) = match texts { | |
| 250 | 257 | Some((old, new)) => { | |
| 251 | 258 | let (hunks, additions, deletions) = line_diff(&old, &new); | |
| 252 | 259 | (hunks, additions, deletions, false) | |
| 253 | 260 | } | |
| 254 | − | None => (Vec::new(), 0, 0, true), | |
| 261 | + | None => (Vec::new(), 0, 0, !omitted), | |
| 255 | 262 | }; | |
| 256 | 263 | lines += hunks.iter().map(|hunk| hunk.lines.len()).sum::<usize>(); | |
| 257 | 264 | files.push(FileDiff { | |
| ⋯ | |||
| 261 | 268 | deletions, | |
| 262 | 269 | binary, | |
| 263 | 270 | hunks, | |
| 271 | + | omitted, | |
| 264 | 272 | }); | |
| 265 | 273 | } | |
| 266 | 274 | Ok((files, truncated)) | |
| 267 | 275 | } | |
| 276 | + | ||
| 277 | + | #[cfg(test)] | |
| 278 | + | mod tests { | |
| 279 | + | use std::collections::HashMap; | |
| 280 | + | use std::future::Future; | |
| 281 | + | use std::pin::pin; | |
| 282 | + | use std::task::{Context, Poll, Waker}; | |
| 283 | + | ||
| 284 | + | use g1t_contracts::repos::{Branch, Commit, GitAccess}; | |
| 285 | + | ||
| 286 | + | use super::*; | |
| 287 | + | use crate::store::Scope; | |
| 288 | + | ||
| 289 | + | fn run<F: Future>(future: F) -> F::Output { | |
| 290 | + | match pin!(future).as_mut().poll(&mut Context::from_waker(Waker::noop())) { | |
| 291 | + | Poll::Ready(output) => output, | |
| 292 | + | Poll::Pending => panic!("the fake store never waits"), | |
| 293 | + | } | |
| 294 | + | } | |
| 295 | + | ||
| 296 | + | #[derive(Default)] | |
| 297 | + | struct Fake { | |
| 298 | + | trees: HashMap<String, Vec<TreeEntry>>, | |
| 299 | + | blobs: HashMap<String, Vec<u8>>, | |
| 300 | + | } | |
| 301 | + | ||
| 302 | + | impl GitRepo for Fake { | |
| 303 | + | async fn access(&self, _scope: Scope) -> Result<GitAccess> { | |
| 304 | + | unimplemented!() | |
| 305 | + | } | |
| 306 | + | async fn branches(&self) -> Result<Vec<Branch>> { | |
| 307 | + | Ok(Vec::new()) | |
| 308 | + | } | |
| 309 | + | async fn log(&self, _git_ref: &str, _limit: u32) -> Result<Vec<Commit>> { | |
| 310 | + | Ok(Vec::new()) | |
| 311 | + | } | |
| 312 | + | async fn parents(&self, _commit_hash: &str) -> Result<Option<Vec<String>>> { | |
| 313 | + | Ok(None) | |
| 314 | + | } | |
| 315 | + | async fn read_tree(&self, tree_hash: &str) -> Result<Option<Vec<TreeEntry>>> { | |
| 316 | + | Ok(self.trees.get(tree_hash).cloned()) | |
| 317 | + | } | |
| 318 | + | async fn read_blob(&self, blob_hash: &str) -> Result<Option<Vec<u8>>> { | |
| 319 | + | Ok(self.blobs.get(blob_hash).cloned()) | |
| 320 | + | } | |
| 321 | + | async fn read_file(&self, _git_ref: &str, _path: &str) -> Result<Option<Vec<u8>>> { | |
| 322 | + | Ok(None) | |
| 323 | + | } | |
| 324 | + | async fn fork(&self, _target_key: &str) -> Result<()> { | |
| 325 | + | Ok(()) | |
| 326 | + | } | |
| 327 | + | } | |
| 328 | + | ||
| 329 | + | /// A tree that adds `count` files of `lines` lines each, f000 onward, | |
| 330 | + | /// then a binary one, `zz.bin`. | |
| 331 | + | fn added(count: usize, lines: usize) -> Fake { | |
| 332 | + | let mut fake = Fake::default(); | |
| 333 | + | let mut entries = Vec::new(); | |
| 334 | + | for n in 0..count { | |
| 335 | + | let hash = format!("b{n}"); | |
| 336 | + | fake.blobs.insert(hash.clone(), "line\n".repeat(lines).into_bytes()); | |
| 337 | + | entries.push(TreeEntry { name: format!("f{n:03}"), hash, kind: EntryKind::Blob }); | |
| 338 | + | } | |
| 339 | + | fake.blobs.insert("bin".into(), vec![0, 1, 2]); | |
| 340 | + | entries.push(TreeEntry { name: "zz.bin".into(), hash: "bin".into(), kind: EntryKind::Blob }); | |
| 341 | + | fake.trees.insert("root".into(), entries); | |
| 342 | + | fake | |
| 343 | + | } | |
| 344 | + | ||
| 345 | + | #[test] | |
| 346 | + | fn a_small_change_has_every_files_lines() { | |
| 347 | + | let fake = added(3, 10); | |
| 348 | + | let (files, truncated) = run(compare_trees(&fake, None, "root", None)).unwrap(); | |
| 349 | + | assert!(!truncated); | |
| 350 | + | assert_eq!(files.len(), 4); | |
| 351 | + | assert!(files[..3].iter().all(|f| !f.omitted && !f.binary && f.additions == 10)); | |
| 352 | + | assert!(files[3].binary && !files[3].omitted); | |
| 353 | + | } | |
| 354 | + | ||
| 355 | + | #[test] | |
| 356 | + | fn files_past_the_line_limit_are_listed_as_omitted_not_binary() { | |
| 357 | + | // 40 files of 1,000 lines: about 20 of them fit. | |
| 358 | + | let fake = added(40, 1_000); | |
| 359 | + | let (files, truncated) = run(compare_trees(&fake, None, "root", None)).unwrap(); | |
| 360 | + | assert!(!truncated, "every file is listed"); | |
| 361 | + | assert_eq!(files.len(), 41); | |
| 362 | + | let read = files.iter().filter(|f| !f.omitted).count(); | |
| 363 | + | assert!((MAX_LINES / 1_000..=MAX_LINES / 1_000 + READS_AT_ONCE).contains(&read), "{read} read"); | |
| 364 | + | let last = files.last().unwrap(); | |
| 365 | + | assert!(last.omitted && !last.binary && last.hunks.is_empty() && last.additions == 0); | |
| 366 | + | } | |
| 367 | + | ||
| 368 | + | #[test] | |
| 369 | + | fn an_omitted_file_is_read_when_asked_for_by_path() { | |
| 370 | + | let fake = added(40, 1_000); | |
| 371 | + | let only = ["f039".to_owned(), "missing".to_owned()]; | |
| 372 | + | let (files, _) = run(compare_trees(&fake, None, "root", Some(&only))).unwrap(); | |
| 373 | + | assert_eq!(files.len(), 1); | |
| 374 | + | assert_eq!(files[0].path, "f039"); | |
| 375 | + | assert!(!files[0].omitted); | |
| 376 | + | assert_eq!(files[0].additions, 1_000); | |
| 377 | + | } | |
| 378 | + | } | |
| 79 | 79 | const MAX_TEXT_BYTES: usize = 512 * 1024; | |
| 80 | 80 | /// How far back a pull request may have forked and still be landed. | |
| 81 | 81 | const MAX_ANCESTRY: u32 = 1000; | |
| 82 | + | /// The most files one `compare` asked for by path reads. | |
| 83 | + | const MAX_COMPARED_PATHS: usize = 50; | |
| 82 | 84 | /// The most tags a repository's Tags page reads and lists. | |
| 83 | 85 | const MAX_TAGS_READ: usize = 100; | |
| 84 | 86 | /// Branch heads measured in one `branch_drift` call. | |
| ⋯ | |||
| 1719 | 1721 | .map(|commit| commit.tree_hash), | |
| 1720 | 1722 | None => None, | |
| 1721 | 1723 | }; | |
| 1724 | + | // Only a few files of it, when asked: how the files a large | |
| 1725 | + | // comparison lists without their lines are read, as a page reaches them. | |
| 1726 | + | let only = a.paths.as_deref().map(|paths| &paths[..paths.len().min(MAX_COMPARED_PATHS)]); | |
| 1722 | 1727 | let (files, truncated) = | |
| 1723 | − | diff::compare_trees(&git, base_tree.as_deref(), &head.tree_hash).await?; | |
| 1728 | + | diff::compare_trees(&git, base_tree.as_deref(), &head.tree_hash, only).await?; | |
| 1724 | 1729 | Ok(Outcome::Ok(Comparison { | |
| 1725 | 1730 | base, | |
| 1726 | 1731 | head: head.hash.clone(), | |
| 408 | 408 | let compared: Outcome<Comparison> = g1t_kit::call( | |
| 409 | 409 | &self.repos, | |
| 410 | 410 | "compare", | |
| 411 | − | &CompareArgs { repo_id, viewer: Some(User::system(&repo.namespace)), base: None, head, base_branch: pull.base.clone() }, | |
| 411 | + | &CompareArgs { repo_id, viewer: Some(User::system(&repo.namespace)), base: None, head, base_branch: pull.base.clone(), paths: None }, | |
| 412 | 412 | ) | |
| 413 | 413 | .await?; | |
| 414 | 414 | Ok(compared.into_result().ok()) | |
| ⋯ | |||
| 566 | 566 | additions: 1, | |
| 567 | 567 | deletions: 0, | |
| 568 | 568 | binary: false, | |
| 569 | + | omitted: false, | |
| 569 | 570 | hunks: vec![Hunk { lines: vec![line(LineKind::Context, Some(11)), line(LineKind::Add, Some(12)), line(LineKind::Delete, None)] }], | |
| 570 | 571 | }], | |
| 571 | 572 | } | |