Read a pull request's diff from T3 and split its patch into files

docs/pull-request-reads.md

@@ -1,6 +1,6 @@
 # Pull request reads
 
-`src/pull_request.rs` reads one pull request from T3's server: its detail, its checks, its conversation and the stack it is in. It is the first part of PR reviews in the TUI ([parity.md, group 12](parity.md#12-pull-requests)). The thread details call `detail` and `activity` at once when a link's details open, then `stack` once the detail says its host keeps stacks, and `r` there sends `invalidate` first, as [linked pull requests](pull-request-panel.md#what-a-links-details-read-from-its-host) says. Nothing calls `checks` yet.
+`src/pull_request.rs` reads one pull request from T3's server: its detail, its checks, its conversation, the stack it is in and its diff. It is the first part of PR reviews in the TUI ([parity.md, group 12](parity.md#12-pull-requests)). The thread details call `detail` and `activity` at once when a link's details open, then `stack` once the detail says its host keeps stacks, and `r` there sends `invalidate` first, as [linked pull requests](pull-request-panel.md#what-a-links-details-read-from-its-host) says. Nothing calls `checks` or `diff` yet.
 
 ```rust
 let reference = Reference::new(&thread_project_id, Some(&link.host), &link.repository, link.number)?;
@@ -9,9 +9,11 @@ let checks = pull_request::checks(&rpc, &reference).await?; // None where the ho
 let activity = pull_request::activity(&rpc, &reference).await?;
 let stack = pull_request::stack(&rpc, &reference).await?; // None where it is in no stack
 pull_request::invalidate(&rpc, &reference).await?; // T3's next read asks the host again
+let diff = pull_request::diff(&api, &reference, None).await?; // the first slice, over HTTP
+let files = pull_request::patch::Patch::read(&diff.patch).files;
 ```
 
-Each function is one RPC call with a 60-second timeout. T3 answers it through the host's CLI or API, or from a short cache of its own. Nothing here writes to the host or keeps what it read. Method names, payloads and shapes follow T3's `packages/contracts/src/pullRequest.ts` and `rpc.ts` at commit f570bd2.
+Each function but `diff` is one RPC call with a 60-second timeout. `diff` is one HTTP request with the same timeout. T3 answers it through the host's CLI or API, or from a short cache of its own. Nothing here writes to the host or keeps what it read. Method names, payloads and shapes follow T3's `packages/contracts/src/pullRequest.ts` and `rpc.ts` at commit f570bd2.
 
 ## What a read needs
 
@@ -22,8 +24,9 @@ Each function is one RPC call with a 60-second timeout. T3 answers it through th
 | `activity` | `pullRequests.activity` | `PullRequestRef` | `PullRequestActivity` |
 | `stack` | `pullRequests.stack` | `PullRequestRef` | `PullRequestStack` or null |
 | `invalidate` | `pullRequests.invalidate` | `{"reference": PullRequestRef}` | nothing |
+| `diff` | `POST /api/pull-requests/diff` | `PullRequestDiffInput` | `PullRequestDiffResult` |
 
-T3 checks all five against `orchestration:read`, which a saved login carries. `pullRequests.invalidate` takes read access too, because T3 counts refreshing as reading. The writes, such as comments, reviews and merges, need `source-control:write`, which t3term's logins don't have. They come later.
+T3 checks all six against `orchestration:read`, which a saved login carries. `pullRequests.invalidate` takes read access too, because T3 counts refreshing as reading. The writes, such as comments, reviews and merges, need `source-control:write`, which t3term's logins don't have. They come later.
 
 A `PullRequestRef` is `{"projectId", "repository", "number"}` with an optional `"host"`. T3 reads through the project's checkout and credentials. With a host, T3 routes a pull request from another repository on that host through the same project, so a thread's link can be read through the thread's own project. The desktop does that when no project matches the link's repository. `Reference::new` trims each string and refuses a blank one, one over 1,024 bytes, and a number below 1 or past 2^53 - 1, so t3term sends nothing T3's schema would refuse.
 
@@ -35,6 +38,14 @@ T3 only offers these methods when the environment's `capabilities.pullRequests`
 
 A `Stack` has the host's id, its own number, its page, the branch its bottom layer merges into, and its layers bottom to top, as GitHub lists them. Each layer has its number, head branch and state, and the title, draft flag and head commit where T3 sends them. A layer's title may be empty. T3 keeps the answer for 60 seconds (apps/server/src/pullRequest/PullRequestReadCache.ts:107-111), and `invalidate` makes it ask the host again.
 
+## A diff
+
+`diff` reads the changes over HTTP, as the desktop does. It POSTs a `PullRequestDiffInput`, which is a `PullRequestRef` and an optional `cursor`, to `/api/pull-requests/diff` with the session's token (packages/contracts/src/environmentHttp.ts:650-663). T3 sends the patch over HTTP because it is often the largest answer, and the route checks `orchestration:read` as the RPC methods do (apps/server/src/pullRequest/http.ts). It takes the `Api` a `Client` holds, where the other reads take the RPC client.
+
+A diff comes a slice at a time, and a slice holds whole files. `next_cursor` is where the next slice starts, or `None` once the diff is whole, and the caller passes it back as `cursor` to read on. `truncated` says the host left something in the slice out, such as a binary file or a change too large to inline. `omitted` gives the host's own counts for files whose hunks it left out. T3's GitHub provider asks GitHub for the whole diff, up to 8 MiB, in one slice. Past that, or past the 300 files GitHub will diff, it reads 100 files a slice from GitHub's files API, and writes a file GitHub sends no patch for as its header alone (apps/server/src/pullRequest/GitHubPullRequestCli.ts:2206-2259, gitHubPullRequestJson.ts:3219-3266). T3 keeps a slice for 60 seconds under the `updatedAt` its last detail read gave, so once a detail read finds the pull request changed, the next slice comes from the host (apps/server/src/pullRequest/PullRequestService.ts:164, 3175-3215).
+
+`patch::Patch::read` splits a slice's patch into files, hunks and lines. A file has its paths before and after, its status (added, deleted, renamed or modified), whether git says it is binary, and the lines it adds and removes. A line has its kind, its numbers before and after the change, and its text as a range of the patch, so nothing is copied. The parser reads git's quoted paths with their escapes, the tab git puts after a path with a space, and a plain unified diff without `diff --git`. A line it can't place, such as a hunk header that isn't one, marks the file `unreadable`. So does a hunk with fewer or more lines on a side than its header counts, including one the patch ends inside. The rest of the patch still reads.
+
 ## What an answer holds
 
 An answer is checked against T3's schema as it is copied into typed structs:
@@ -61,9 +72,14 @@ Text is cut on a character boundary before anything else reads it, and lists are
 | Remarks in one review thread | 1,000 |
 | Commits | 1,000 |
 | Layers of a stack | 1,000 |
+| A diff's answer | 16 MiB, twice the most T3 reads from GitHub at once |
+| Files a diff left out | 3,000 |
+| Files of one patch | 3,000 |
+| Hunks of one patch and their lines | 200,000, in all its files, a hunk's header counting as a line |
+| A path in a patch | 4,096 bytes, as T3's `FilePath` |
 | Reviewers, labels, commit authors, people behind one reaction | 100 |
 
-A body that lost text says so with `Body::cut`, and a detail, an activity, a list of checks, a review thread or a stack that lost entries says so with its `cut` flag. The bounds don't limit memory. The connection's reader decodes each message whole first, and tungstenite 0.28 allows one of up to 64 MiB.
+A body that lost text says so with `Body::cut`, and a detail, an activity, a list of checks, a review thread or a stack that lost entries says so with its `cut` flag. A diff that named more files left out says so with `omitted_cut`, and a patch with more files with `files_cut`. A file whose hunks or lines are past the bound keeps its counts and says so with `lines_cut`. The bounds don't limit memory for the RPC reads. The connection's reader decodes each message whole first, and tungstenite 0.28 allows one of up to 64 MiB. `diff` reads its answer a chunk at a time and stops at 16 MiB, before any of it is decoded.
 
 ## Failures
 
@@ -74,17 +90,21 @@ A body that lost text says so with `Body::cut`, and a detail, an activity, a lis
 | `NotFound` | `PullRequestOperationError` with the reason `not-found` | T3's `detail` |
 | `Malformed` | an answer that breaks T3's schema | "T3 sent a pull request answer t3term can't read:" and what broke |
 | `Rpc` | anything else, such as a host's failure, a refused scope or a lost connection | the RPC client's |
+| `Http` | a diff's answer that is none of the above: a refused token or scope, T3's own failure, another status, a redirect, an answer past 16 MiB or no answer | t3term's, with the reason T3 sent where it sent one |
+
+The diff route answers each failure with a status of its own: 503 for `PullRequestUnavailableError`, 502 for `PullRequestOperationError`, 401 for a token T3 refused, 403 for a missing scope and 500 for its own failure. `diff` reads the first two as the RPC reads do, so a host without credentials and a pull request the host can't find read the same either way. It never follows a redirect, which would send the reference and the token to a server t3term didn't choose.
 
 T3 doesn't send `PullRequestUnavailableError`'s message, only its reason and provider, so t3term carries the desktop's sentences. To read them, `RpcError::Failed` now keeps the failure's encoded error alongside its message.
 
 ## Tests
 
-The unit tests read a detail with every field filled in, then without each optional field and without each required one. They check values this version doesn't know, text cut on a character boundary at each bound, lists past their bound whose extra entries are never read, an activity's remarks, threads and commits, a stack's layers in order and without each required field, and the failures' messages for every host and reason. `tests/fake_server.rs` checks the exact request for each of the five methods with and without a host, a host with no checks, a pull request in no stack, an unavailable host, a pull request the host can't find and an answer that breaks T3's schema.
+The unit tests read a detail with every field filled in, then without each optional field and without each required one. They check values this version doesn't know, text cut on a character boundary at each bound, lists past their bound whose extra entries are never read, an activity's remarks, threads and commits, a stack's layers in order and without each required field, and the failures' messages for every host and reason. They read a diff's answer, a diff left out past its bound and each broken field, and run `diff` against a fake T3 on 127.0.0.1. That checks the exact request with and without a cursor, every failure the route encodes, an error that isn't JSON, a redirect, no answer, and an answer one byte past its bound with and without its length up front. The patch tests read hunks with every line numbered, added, deleted, renamed, binary and header-only files, quoted and spaced paths, Windows line ends, broken and short hunks, a plain unified diff and each bound. `tests/fake_server.rs` checks the exact request for each of the five methods with and without a host, a host with no checks, a pull request in no stack, an unavailable host, a pull request the host can't find and an answer that breaks T3's schema.
 
 ## Not done yet
 
 - The TUI calls `detail`, `activity`, `stack` and `invalidate`, and follows `pullRequests.subscribeRefreshes` through `client::spawn_pull_request_refresh_watch`. It shows the activity's remarks, verdicts, commits and reactions, not its review threads. It never reads `checks`.
 - There is no CLI command for a pull request.
-- The list of pull requests, the diff, viewed files, and reviewer and label candidates aren't read.
+- The list of pull requests, the diff of one commit, a file's whole contents, viewed files, review thread replies, and reviewer and label candidates aren't read.
+- `diff` doesn't ask for compression, as T3 runs on the same machine.
 - Nothing writes: no comment, review, reaction, resolution, label, reviewer or action. Those need `source-control:write`.
 - A project whose repository matches the link isn't looked for. The TUI passes the thread's project and the link's host, and T3 routes the ref by host.

src/http.rs

@@ -22,6 +22,18 @@ pub(crate) fn direct_client() -> reqwest::Client {
         .expect("an HTTP client")
 }
 
+/// Why `Api::post_bounded` has no answer to read.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub(crate) enum Unanswered {
+    /// The request failed before an answer came, or the answer broke off or took longer than
+    /// the time allowed. The text is reqwest's, without the URL.
+    Failed(String),
+    /// The server answered with a redirect, which t3term doesn't follow.
+    Redirected,
+    /// The body is longer than the bound.
+    TooLong,
+}
+
 #[derive(Clone)]
 pub struct Api {
     pub origin: String,
@@ -128,6 +140,48 @@ impl Api {
         self.request(reqwest::Method::POST, path, body).await
     }
 
+    /// POSTs `body` as JSON to `path` and reads the answer's status and body, all within
+    /// `timeout`. Unlike `request`, it leaves the status to the caller, and it never reads
+    /// more than `most` bytes of the body.
+    pub(crate) async fn post_bounded(
+        &self,
+        path: &str,
+        body: &Value,
+        timeout: Duration,
+        most: usize,
+    ) -> std::result::Result<(reqwest::StatusCode, Vec<u8>), Unanswered> {
+        let failed = |error: reqwest::Error| Unanswered::Failed(error.without_url().to_string());
+        let mut response = self
+            .http
+            .post(format!("{}{}", self.origin, path))
+            .bearer_auth(&self.token)
+            .header(PROTOCOL_HEADER, PROTOCOL_VERSION.to_string())
+            .timeout(timeout)
+            .json(body)
+            .send()
+            .await
+            .map_err(failed)?;
+        let status = response.status();
+        // As in `request`, a redirect would send the body to a server t3term didn't choose.
+        if status.is_redirection() {
+            return Err(Unanswered::Redirected);
+        }
+        if response
+            .content_length()
+            .is_some_and(|length| length > most as u64)
+        {
+            return Err(Unanswered::TooLong);
+        }
+        let mut read = Vec::new();
+        while let Some(chunk) = response.chunk().await.map_err(failed)? {
+            if read.len() + chunk.len() > most {
+                return Err(Unanswered::TooLong);
+            }
+            read.extend_from_slice(&chunk);
+        }
+        Ok((status, read))
+    }
+
     /// A single-use ticket that authenticates one WebSocket connection.
     pub async fn websocket_ticket(&self) -> Result<String> {
         let issued = self.post("/api/auth/websocket-ticket", None).await?;
@@ -141,26 +195,26 @@ impl Api {
 }
 
 /// A fake T3 for unit tests, on 127.0.0.1. It answers each request with the raw HTTP response
-/// `respond` gives for the request's path, and records the paths. An empty response closes the
-/// connection without an answer.
+/// `respond` gives for the request's path, and records the paths and bodies. An empty response
+/// closes the connection without an answer.
 #[cfg(test)]
 pub(crate) mod fake {
-    use std::io::{BufRead, BufReader, Write};
+    use std::io::{BufRead, BufReader, Read, Write};
     use std::net::TcpListener;
     use std::sync::{Arc, Mutex};
 
     pub(crate) struct Server {
         pub(crate) origin: String,
-        paths: Arc<Mutex<Vec<String>>>,
+        requests: Arc<Mutex<Vec<(String, String)>>>,
     }
 
     impl Server {
         /// Starts a server on a free port. Its thread answers until the test process exits.
         pub(crate) fn start(respond: impl Fn(&str) -> String + Send + 'static) -> Server {
             let listener = TcpListener::bind("127.0.0.1:0").unwrap();
             let origin = format!("http://{}", listener.local_addr().unwrap());
-            let paths = Arc::new(Mutex::new(Vec::new()));
-            let seen = paths.clone();
+            let requests = Arc::new(Mutex::new(Vec::new()));
+            let seen = requests.clone();
             std::thread::spawn(move || {
                 for stream in listener.incoming() {
                     let Ok(mut stream) = stream else { continue };
@@ -172,24 +226,41 @@ pub(crate) mod fake {
                     if reader.read_line(&mut request_line).is_err() {
                         continue;
                     }
-                    // Read up to the blank line that ends the headers. No request a test sends
-                    // has a body.
+                    // Read up to the blank line that ends the headers, then the body its length
+                    // gives.
                     let mut header = String::new();
+                    let mut length = 0;
                     while reader.read_line(&mut header).is_ok_and(|read| read > 2) {
+                        let (name, value) = header.split_once(':').unwrap_or_default();
+                        if name.eq_ignore_ascii_case("content-length") {
+                            length = value.trim().parse().unwrap_or(0);
+                        }
                         header.clear();
                     }
+                    let mut body = vec![0; length];
+                    if reader.read_exact(&mut body).is_err() {
+                        continue;
+                    }
                     let path = request_line.split_whitespace().nth(1).unwrap_or_default();
                     let reply = respond(path);
-                    seen.lock().unwrap().push(path.to_string());
+                    let body = String::from_utf8_lossy(&body).into_owned();
+                    seen.lock().unwrap().push((path.to_string(), body));
                     let _ = stream.write_all(reply.as_bytes());
                 }
             });
-            Server { origin, paths }
+            Server { origin, requests }
         }
 
         /// The path of each request so far, in order.
         pub(crate) fn paths(&self) -> Vec<String> {
-            self.paths.lock().unwrap().clone()
+            let requests = self.requests.lock().unwrap();
+            requests.iter().map(|(path, _)| path.clone()).collect()
+        }
+
+        /// The body of each request so far, in order. A request without one has "".
+        pub(crate) fn bodies(&self) -> Vec<String> {
+            let requests = self.requests.lock().unwrap();
+            requests.iter().map(|(_, body)| body.clone()).collect()
         }
     }