| | @@ -1,162 +0,0 @@ |
| 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-`. |