| | @@ -0,0 +1,162 @@ |
| 1 | +# Handover — `commits`: the commit page and the compare view |
| 2 | + |
| 3 | +Written by the `commits` agent of the section 1 wave, for the orchestrator to fold into |
| 4 | +`progress.md`, `current.md` and `ui.md`. Branch `feat/commit-page`. |
| 5 | + |
| 6 | +## What shipped |
| 7 | + |
| 8 | +Three routes, all read-only, all authorized through the existing `view_repo`: |
| 9 | + |
| 10 | +| URL | What it is | |
| 11 | +|---|---| |
| 12 | +| `/{handle}/repos/{name}/commits/{sha}` | one commit and its diff | |
| 13 | +| `/{handle}/repos/{name}/compare` | the form asking which two revisions | |
| 14 | +| `/{handle}/repos/{name}/compare/{base}...{head}` | the comparison, three-dot | |
| 15 | + |
| 16 | +New files: `src/application/diff.rs` (pure parsing), `src/application/commit.rs` (two use |
| 17 | +cases), `src/infrastructure/web/commit.rs` (three pages and the diff renderer). Four new |
| 18 | +`GitQuery` methods, one new domain type (`CommitDetail`), five plain-CSS rules in |
| 19 | +`styles.css`. |
| 20 | + |
| 21 | +Every sha in the log and the sha in the landing page's latest-commit bar now link to the |
| 22 | +commit page — the "nothing links to a commit by its sha yet" item in `current.md` is |
| 23 | +closed. |
| 24 | + |
| 25 | +## progress.md material — the decisions |
| 26 | + |
| 27 | +**The counts and the patch come from one `git` process.** `git diff-tree --numstat -p` |
| 28 | +writes the `--numstat` block *before* the patch. Asking for both in the same run means |
| 29 | +the per-file counts survive the byte cap a very large diff hits: a commit whose patch |
| 30 | +cannot be drawn still lists every changed file with real numbers. A second `--numstat` |
| 31 | +call would have been paid on every commit page to buy something only the rare oversized |
| 32 | +one needs. Measured alternative rejected: deriving counts by summing the patch's `+`/`-` |
| 33 | +lines is exact for text but produces nothing at all once the patch is truncated, which is |
| 34 | +the case the file list exists for. |
| 35 | + |
| 36 | +**`run_capped` reads through a pipe and kills the child.** `run` uses `Command::output`, |
| 37 | +which collects everything git writes. That is right for a tree listing and wrong for a |
| 38 | +patch — one commit can carry hundreds of megabytes of diff, and a page must not be able |
| 39 | +to pull that into memory. `run_capped` reads `max_bytes + 1` and `start_kill()`s. **The |
| 40 | +exit status is checked only when the cap was not hit**, because a killed process has a |
| 41 | +status that says so. |
| 42 | + |
| 43 | +**`merge-base` is the one command allowed a non-zero exit**, via `run_allowing`. The |
| 44 | +module's rule is that a non-zero exit is always a fault, because git's not-found codes |
| 45 | +collide with its error codes at other call sites. `git merge-base` is the exception git's |
| 46 | +own manual documents: 1 means "no common ancestor", 128 means broken. Two unrelated |
| 47 | +histories in one repository is a state a compare page must be able to state rather than |
| 48 | +500 on. **Adding a second caller to `run_allowing` deserves the same scrutiny** — the |
| 49 | +question is whether git's manual promises the code, not whether it happens to work. |
| 50 | + |
| 51 | +**Compare is three-dot**, diffing merge-base-to-head. A two-dot diff also undoes |
| 52 | +everything the base gained since the branch left it, which reads as a wall of deletions |
| 53 | +nobody made. It is also exactly what a pull request will need, so it is settled now rather |
| 54 | +than when pull requests arrive. |
| 55 | + |
| 56 | +**The patch parser tracks hunk line counts rather than matching prefixes anywhere.** A |
| 57 | +patch *of a patch* contains lines reading `diff --git …`, `+++ b/…` and `--- a/…` as |
| 58 | +ordinary content. A parser that matched those prefixes anywhere splits one file into |
| 59 | +several and attributes the rest of the commit to a file that does not exist. `@@ -a,b |
| 60 | ++c,d @@`'s two lengths are what tell the parser whether it is inside a hunk. |
| 61 | + |
| 62 | +**Paths come from `---`/`+++`/`rename from`, not from the `diff --git` header.** The |
| 63 | +header is `a/<old> b/<new>` with a space between two names that may themselves contain |
| 64 | +spaces, and nothing in the format says where the split is. The header is a first guess and |
| 65 | +every later line overrides it. C-style quoting (`"docs/caf\303\251.md"`) is unquoted, or |
| 66 | +a link would carry a name that is not the file's name. |
| 67 | + |
| 68 | +**The compare form GETs to `/compare?base=…&head=…` and that page redirects (307) to the |
| 69 | +path form.** A GET form submits query parameters, not path segments, and Steid does not |
| 70 | +require JavaScript. The shareable URL is therefore always the path one, which is what the |
| 71 | +branches page's contract needs. |
| 72 | + |
| 73 | +**`...` is unambiguous as a separator** because `RefName` refuses `..` outright, so a |
| 74 | +revision cannot contain two consecutive dots. Each half is percent-encoded on its own, so |
| 75 | +`feature/login` stays inside its half. |
| 76 | + |
| 77 | +**The file header is `sticky`, which is why the panel is not `overflow-hidden`.** A hidden |
| 78 | +overflow makes an ancestor a scroll container and sticky then sticks to a box that never |
| 79 | +scrolls — which looks exactly like sticky being broken. The corners are rounded on the |
| 80 | +children instead. |
| 81 | + |
| 82 | +## Process cost |
| 83 | + |
| 84 | +| Page | `git` processes | |
| 85 | +|---|---| |
| 86 | +| commit | **3** — resolve, read the commit, diff | |
| 87 | +| compare, path form | **5** — two resolves (concurrent), merge base, then log range and diff (concurrent) → three round trips | |
| 88 | +| compare, empty form | **2** — `list_refs` for the datalist, `default_branch` to prefill the base | |
| 89 | + |
| 90 | +## current.md material — holes opened and shortcuts taken |
| 91 | + |
| 92 | +- **The timeout state is compiled but has never been seen.** `unwrap_page` turns a |
| 93 | + `GitQueryError::TimedOut` into a rendered "This took too long to read" panel rather |
| 94 | + than a 500, which is what the brief asked for, but no repository here is large enough |
| 95 | + to spend twenty seconds in `diff-tree`. It is the one designed state with no screenshot. |
| 96 | +- **`Diff::files` is emptied wholesale when the patch is truncated.** Half a patch is not |
| 97 | + a smaller patch, so the page falls back to the numstat file list. The files that *did* |
| 98 | + fit are discarded rather than shown — deliberate, but it means a 10 MiB commit shows no |
| 99 | + lines at all rather than the first few files' worth. |
| 100 | +- **numstat itself could in principle be truncated.** It is written first and is ~30 bytes |
| 101 | + per file, so it takes ~300,000 changed files in one commit to reach the 10 MiB cap |
| 102 | + before the patch begins. Not defended against. |
| 103 | +- **A merge commit is diffed against its first parent only**, which is the right answer |
| 104 | + for "what did this commit change" but means a merge that resolved conflicts by hand |
| 105 | + shows only the first-parent view. No fixture here contains a merge, so **merge |
| 106 | + rendering was compiled, not seen** — the parent list renders two links by construction. |
| 107 | +- **`MAX_RAW_BYTES` (10 MiB) is now doing a second job.** It was chosen as "how much of a |
| 108 | + repository may one request hold in memory" for raw blobs; the diff reuses it. If it is |
| 109 | + ever retuned for blobs, the diff cap moves with it. |
| 110 | +- **`bg-surface` is nearly `bg-background` in light mode** (`oklch(1 0 0)` vs |
| 111 | + `oklch(0.99 …)`), so a file header reads only by its border there. Not introduced by |
| 112 | + this step — the empty-repository `<pre>` has the same property — but this page has one |
| 113 | + such header per file, which makes it the most visible instance. |
| 114 | +- **The `/log` page still shows at most 50 commits with no paging**, and a comparison |
| 115 | + shows at most 100. Both now say so; neither has a next page. |
| 116 | +- **The compare form's `<datalist>` costs a `list_refs`.** For a repository with hundreds |
| 117 | + of refs that is a large `<option>` list rendered inline on every visit to the form. |
| 118 | + |
| 119 | +## ui.md material |
| 120 | + |
| 121 | +The entry-point table's rows for **"A commit page — the sha in the latest-commit bar and |
| 122 | +in the log"** and **"Compare — reached from a branch row on the branches page"** are now |
| 123 | +filled in. Compare is *also* reachable directly at `/compare`, which the table did not |
| 124 | +anticipate: a branch row is the shortcut, not the only door. |
| 125 | + |
| 126 | +Notes worth adding under "The repository page": |
| 127 | + |
| 128 | +- **A commit page is a Commits-tab page**, and its header's Commits link points at the log |
| 129 | + *at that commit's own id*, not at the revision the URL used. A branch name there would |
| 130 | + send someone to a different commit tomorrow. |
| 131 | +- **A compare page is a Code-tab page.** It is about two revisions of the code, not about |
| 132 | + the history, and it is reached from the branches page. |
| 133 | +- **The success and destructive tokens appear as text in exactly three places**: the |
| 134 | + commit's `+a −b` summary, each file header's `+a −b`, and the truncated file list. The |
| 135 | + diff line tints are the same tokens at 12% (rows) and 20% (gutters) through |
| 136 | + `color-mix`, as plain CSS in `styles.css` — Tailwind has no utility for a token at an |
| 137 | + alpha, and a literal colour would follow neither a palette change nor the colour scheme. |
| 138 | +- **The primary colour is used only for links out of a dead end**: "View the whole file" |
| 139 | + under a truncated file, and "Compare them the other way round" when the head is behind. |
| 140 | + Nothing else on these pages is coloured. |
| 141 | +- **Every state is designed**: no files changed, binary, a rename, a file with no line |
| 142 | + changes, a file over 1,000 lines, a diff over the byte cap, identical revisions, |
| 143 | + unrelated histories, a head already contained in its base, an unknown ref (which |
| 144 | + re-renders the form with the reason rather than 404ing), and a timeout. |
| 145 | + |
| 146 | +## Verification |
| 147 | + |
| 148 | +`cargo test` — 507 pass. `cargo clippy --all-targets` — zero warnings. `cargo fmt` clean. |
| 149 | + |
| 150 | +Run on port 3101 against a `steid` repository holding this repository's own history, two |
| 151 | +annotated tags, and a `fixtures` branch built for this step: a commit with a rename, a |
| 152 | +binary change, an addition and a deletion; a commit adding a 5,000-line file; and a commit |
| 153 | +adding 15 MB of text. Checked in the browser: both colour modes on the commit page, an |
| 154 | +800px viewport with no horizontal page overflow (long lines scroll inside their own |
| 155 | +container), the sticky file header, all three compare states, the two truncation states, |
| 156 | +a branch name containing a slash through the compare URL, and 404 for an anonymous |
| 157 | +visitor on every one of these routes in a private repository. |
| 158 | + |
| 159 | +**Not verified:** the timeout state, a merge commit, and a repository whose refs are not |
| 160 | +UTF-8. |
| 161 | + |
| 162 | +Screenshots are in the wave's shots directory, prefixed `commits-`. |