a73x

ee237a75

Show a patch's whole diff, not just its head commit

a73x   2026-08-30 11:04

Commit message
Show a patch's whole diff, not just its head commit

The patch page linked each revision's commit to `/diff/{oid}`, which is a
single-commit view: it diffs a commit against its first parent. On a patch
of seven commits that renders one commit and one file while `patch diff
--stat` reports thirty-three, so the page disagreed with the record it was
displaying and there was no web view of a patch's actual change at all.

Adds `/{repo}/patches/{id}/diff`, reached from a "Files changed" link, and
leaves the per-commit links alone — drilling into one commit is still worth
doing, it just is not the patch.

The base is resolved by `patch::patch_diff`, extracted from the CLI's
`generate_diff` rather than reimplemented. A second implementation would be
a second answer to which trees a patch compares, and the two would part
company the moment a patch lands: the recomputed merge-base is then the head
itself, so the diff renders empty. The recorded base is preferred for that
reason, with merge-base as the fallback it always was.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
src/patch.rs
Old New
@@ -1255,12 +1255,19 @@ fn recorded_base_tree<'a>(repo: &'a Repository, base: Option<Oid>) -> Option<git
1255 repo.find_commit(oid).ok()?.tree().ok() 1255 repo.find_commit(oid).ok()?.tree().ok()
1256 } 1256 }
1257 1257
1258 /// Generate a diff string from a patch's base and head using three-dot (merge-base) diff. 1258 /// The diff a patch *is*: its base to its head, three-dot (merge-base) when the
1259 pub fn generate_diff( 1259 /// revision recorded no base of its own.
1260 repo: &Repository, 1260 ///
1261 /// Public because the web server renders the same range. A second
1262 /// implementation there would be a second answer to "which trees does this
1263 /// patch compare", and the two would disagree the moment a patch lands — at
1264 /// which point the recomputed merge-base *is* the head and the diff comes out
1265 /// empty. There is one base resolution, and this is it.
1266 pub fn patch_diff<'r>(
1267 repo: &'r Repository,
1261 patch: &state::PatchState, 1268 patch: &state::PatchState,
1262 opts: &DiffOpts, 1269 opts: &DiffOpts,
1263 ) -> Result<String, Error> { 1270 ) -> Result<git2::Diff<'r>, Error> {
1264 let head_oid = patch.resolve_head(repo)?; 1271 let head_oid = patch.resolve_head(repo)?;
1265 let head_commit = repo 1272 let head_commit = repo
1266 .find_commit(head_oid) 1273 .find_commit(head_oid)
@@ -1271,12 +1278,20 @@ pub fn generate_diff(
1271 None => resolve_base_tree(repo, &patch.base_ref, head_oid)?, 1278 None => resolve_base_tree(repo, &patch.base_ref, head_oid)?,
1272 }; 1279 };
1273 1280
1274 let git_diff = repo.diff_tree_to_tree( 1281 Ok(repo.diff_tree_to_tree(
1275 base_tree.as_ref(), 1282 base_tree.as_ref(),
1276 Some(&head_tree), 1283 Some(&head_tree),
1277 Some(&mut diff_options(opts)), 1284 Some(&mut diff_options(opts)),
1278 )?; 1285 )?)
1279 format_diff(&git_diff, opts) 1286 }
1287
1288 /// Generate a diff string from a patch's base and head using three-dot (merge-base) diff.
1289 pub fn generate_diff(
1290 repo: &Repository,
1291 patch: &state::PatchState,
1292 opts: &DiffOpts,
1293 ) -> Result<String, Error> {
1294 format_diff(&patch_diff(repo, patch, opts)?, opts)
1280 } 1295 }
1281 1296
1282 /// Generate a diff for a specific revision against the base branch (historical full diff). 1297 /// Generate a diff for a specific revision against the base branch (historical full diff).
src/server/http/mod.rs
Old New
@@ -41,6 +41,10 @@ pub fn router(state: AppState) -> Router {
41 "/{repo_name}/patches/{id}", 41 "/{repo_name}/patches/{id}",
42 axum::routing::get(repo::patch_detail), 42 axum::routing::get(repo::patch_detail),
43 ) 43 )
44 .route(
45 "/{repo_name}/patches/{id}/diff",
46 axum::routing::get(repo::patch_diff),
47 )
44 .route("/{repo_name}/issues", axum::routing::get(repo::issues)) 48 .route("/{repo_name}/issues", axum::routing::get(repo::issues))
45 .route( 49 .route(
46 "/{repo_name}/issues/{id}", 50 "/{repo_name}/issues/{id}",
src/server/http/repo/diff.rs
Old New
@@ -258,6 +258,16 @@ fn compute_diff_files(repo: &git2::Repository, oid: git2::Oid) -> Vec<DiffFile>
258 Err(_) => return Vec::new(), 258 Err(_) => return Vec::new(),
259 }; 259 };
260 260
261 collect_diff_files(&diff)
262 }
263
264 /// Walk any libgit2 diff into the side-by-side rows the templates render.
265 ///
266 /// Which trees were compared is the caller's question — a commit against its
267 /// parent here, a patch against its base in [`patch_diff`] — and the two must
268 /// not drift apart in how they *render*, which is why only the tree choice
269 /// differs between them.
270 pub fn collect_diff_files(diff: &git2::Diff<'_>) -> Vec<DiffFile> {
261 let collector = RefCell::new(DiffCollector::default()); 271 let collector = RefCell::new(DiffCollector::default());
262 272
263 let mut file_cb = |delta: git2::DiffDelta<'_>, _progress: f32| { 273 let mut file_cb = |delta: git2::DiffDelta<'_>, _progress: f32| {
src/server/http/repo/mod.rs
Old New
@@ -1,5 +1,6 @@
1 mod commits; 1 mod commits;
2 mod diff; 2 mod diff;
3 mod patch_diff;
3 mod issues; 4 mod issues;
4 mod overview; 5 mod overview;
5 mod patches; 6 mod patches;
@@ -9,6 +10,7 @@ mod tree;
9 10
10 pub use commits::{commits, commits_ref}; 11 pub use commits::{commits, commits_ref};
11 pub use diff::diff; 12 pub use diff::diff;
13 pub use patch_diff::patch_diff;
12 pub use issues::{issue_detail, issues}; 14 pub use issues::{issue_detail, issues};
13 pub use overview::overview; 15 pub use overview::overview;
14 pub use patches::{patch_detail, patches}; 16 pub use patches::{patch_detail, patches};
src/server/http/repo/patch_diff.rs
Old New
@@ -0,0 +1,79 @@
1 //! The whole change a patch carries, base to head.
2 //!
3 //! Distinct from `/diff/{oid}`, which is one commit against its parent. A
4 //! patch of seven commits has seven such views and none of them is the patch,
5 //! which is the confusion this page exists to end.
6
7 use std::sync::Arc;
8
9 use axum::extract::{Path as AxumPath, State};
10 use axum::response::{IntoResponse, Response};
11
12 use super::diff::{collect_diff_files, DiffFile};
13 use super::{collab_counts, internal_error, not_found, open_repo, AppState};
14
15 #[derive(askama::Template, askama_web::WebTemplate)]
16 #[template(path = "patch_diff.html")]
17 pub struct PatchDiffTemplate {
18 pub site_title: String,
19 pub repo_name: String,
20 pub active_section: String,
21 pub open_patches: usize,
22 pub open_issues: usize,
23 pub patch_id: String,
24 pub short_id: String,
25 pub title: String,
26 pub branch: String,
27 pub base_ref: String,
28 pub file_count: usize,
29 pub diff_files: Vec<DiffFile>,
30 }
31
32 pub async fn patch_diff(
33 AxumPath((repo_name, patch_id)): AxumPath<(String, String)>,
34 State(state): State<Arc<AppState>>,
35 ) -> Response {
36 let (_entry, repo) = match open_repo(&state, &repo_name) {
37 Ok(r) => r,
38 Err(resp) => return resp,
39 };
40
41 let (open_patches, open_issues) = collab_counts(&repo);
42
43 let (ref_name, full_id) = match git_collab::state::resolve_patch_ref(&repo, &patch_id) {
44 Ok(r) => r,
45 Err(_) => return not_found(&state, format!("Patch '{}' not found.", patch_id)),
46 };
47
48 let ps = match git_collab::state::PatchState::from_ref(&repo, &ref_name, &full_id) {
49 Ok(s) => s,
50 Err(_) => return internal_error(&state, "Failed to load patch state."),
51 };
52
53 // The CLI's own base resolution, not a second one — see `patch::patch_diff`.
54 let opts = git_collab::patch::DiffOpts::default();
55 let diff_files = match git_collab::patch::patch_diff(&repo, &ps, &opts) {
56 Ok(diff) => collect_diff_files(&diff),
57 Err(_) => Vec::new(),
58 };
59
60 let short_id = git_collab::abbrev::for_patches(&repo)
61 .of(&full_id)
62 .to_string();
63
64 PatchDiffTemplate {
65 site_title: state.site_title.clone(),
66 repo_name,
67 active_section: "patches".to_string(),
68 open_patches,
69 open_issues,
70 patch_id: full_id,
71 short_id,
72 title: ps.title,
73 branch: ps.branch,
74 base_ref: ps.base_ref,
75 file_count: diff_files.len(),
76 diff_files,
77 }
78 .into_response()
79 }
src/server/http/repo/patches.rs
Old New
@@ -150,6 +150,8 @@ pub struct InlineCommentView {
150 150
151 #[derive(Debug)] 151 #[derive(Debug)]
152 pub struct PatchDetailView { 152 pub struct PatchDetailView {
153 /// The full id, for addressing this patch's own sub-routes.
154 pub id: String,
153 pub title: String, 155 pub title: String,
154 pub body: String, 156 pub body: String,
155 pub status: String, 157 pub status: String,
@@ -307,6 +309,7 @@ pub async fn patch_detail(
307 }; 309 };
308 310
309 let patch = PatchDetailView { 311 let patch = PatchDetailView {
312 id: full_id,
310 title: ps.title, 313 title: ps.title,
311 body: ps.body, 314 body: ps.body,
312 status: ps.status.as_str().to_string(), 315 status: ps.status.as_str().to_string(),
src/server/http/templates/diff.html
Old New
@@ -23,45 +23,6 @@
23 {% if diff_files.is_empty() %} 23 {% if diff_files.is_empty() %}
24 <p style="color: #666;">No diff available for this commit.</p> 24 <p style="color: #666;">No diff available for this commit.</p>
25 {% else %} 25 {% else %}
26 <div style="display: grid; gap: 16px;"> 26 {% include "diff_files.html" %}
27 {% for file in diff_files %}
28 <section class="diff-file-block">
29 <div class="diff-file-heading mono">{{ file.path }}</div>
30 {% if file.hunks.is_empty() %}
31 <div style="padding: 12px; color: #666;">No textual changes available.</div>
32 {% else %}
33 <table class="side-by-side-diff">
34 <colgroup>
35 <col class="diff-line-col">
36 <col class="diff-code-col">
37 <col class="diff-line-col">
38 <col class="diff-code-col">
39 </colgroup>
40 <thead>
41 <tr>
42 <th colspan="2">Old</th>
43 <th colspan="2">New</th>
44 </tr>
45 </thead>
46 <tbody>
47 {% for hunk in file.hunks %}
48 <tr>
49 <td class="diff-hunk" colspan="4">{{ hunk.header }}</td>
50 </tr>
51 {% for row in hunk.rows %}
52 <tr>
53 <td class="diff-line-no diff-{{ row.left.kind }}">{{ row.left.line_no }}</td>
54 <td class="diff-code diff-{{ row.left.kind }}">{{ row.left.text }}</td>
55 <td class="diff-line-no diff-{{ row.right.kind }}">{{ row.right.line_no }}</td>
56 <td class="diff-code diff-{{ row.right.kind }}">{{ row.right.text }}</td>
57 </tr>
58 {% endfor %}
59 {% endfor %}
60 </tbody>
61 </table>
62 {% endif %}
63 </section>
64 {% endfor %}
65 </div>
66 {% endif %} 27 {% endif %}
67 {% endblock %} 28 {% endblock %}
src/server/http/templates/diff_files.html
Old New
@@ -0,0 +1,42 @@
1 {# The rendered files of a diff. Shared so that a commit's diff and a patch's
2 diff cannot drift apart in how they look — only in which trees they compare. #}
3 <div style="display: grid; gap: 16px;">
4 {% for file in diff_files %}
5 <section class="diff-file-block">
6 <div class="diff-file-heading mono">{{ file.path }}</div>
7 {% if file.hunks.is_empty() %}
8 <div style="padding: 12px; color: #666;">No textual changes available.</div>
9 {% else %}
10 <table class="side-by-side-diff">
11 <colgroup>
12 <col class="diff-line-col">
13 <col class="diff-code-col">
14 <col class="diff-line-col">
15 <col class="diff-code-col">
16 </colgroup>
17 <thead>
18 <tr>
19 <th colspan="2">Old</th>
20 <th colspan="2">New</th>
21 </tr>
22 </thead>
23 <tbody>
24 {% for hunk in file.hunks %}
25 <tr>
26 <td class="diff-hunk" colspan="4">{{ hunk.header }}</td>
27 </tr>
28 {% for row in hunk.rows %}
29 <tr>
30 <td class="diff-line-no diff-{{ row.left.kind }}">{{ row.left.line_no }}</td>
31 <td class="diff-code diff-{{ row.left.kind }}">{{ row.left.text }}</td>
32 <td class="diff-line-no diff-{{ row.right.kind }}">{{ row.right.line_no }}</td>
33 <td class="diff-code diff-{{ row.right.kind }}">{{ row.right.text }}</td>
34 </tr>
35 {% endfor %}
36 {% endfor %}
37 </tbody>
38 </table>
39 {% endif %}
40 </section>
41 {% endfor %}
42 </div>
src/server/http/templates/patch_detail.html
Old New
@@ -9,6 +9,7 @@
9 &nbsp; by <strong>{{ patch.author }}</strong> 9 &nbsp; by <strong>{{ patch.author }}</strong>
10 &nbsp; <span class="mono" style="color: #666;">{{ patch.branch }} → {{ patch.base_ref }}</span> 10 &nbsp; <span class="mono" style="color: #666;">{{ patch.branch }} → {{ patch.base_ref }}</span>
11 </p> 11 </p>
12 <p><a href="/{{ repo_name }}/patches/{{ patch.id }}/diff">Files changed</a></p>
12 {% if let Some(merge_commit) = patch.merge_commit %} 13 {% if let Some(merge_commit) = patch.merge_commit %}
13 {# The link resolves the full oid; the text matches the Revisions table below. 14 {# The link resolves the full oid; the text matches the Revisions table below.
14 See issue b2a57996. #} 15 See issue b2a57996. #}
src/server/http/templates/patch_diff.html
Old New
@@ -0,0 +1,20 @@
1 {% extends "repo_base.html" %}
2
3 {% block title %}{{ title }} — diff — {{ repo_name }} — {{ site_title }}{% endblock %}
4
5 {% block content %}
6 <h2>{{ title }}</h2>
7 <p>
8 <a class="mono" href="/{{ repo_name }}/patches/{{ patch_id }}">{{ short_id }}</a>
9 &nbsp; <span class="mono" style="color: #666;">{{ branch }} → {{ base_ref }}</span>
10 </p>
11
12 <hr style="margin: 16px 0;">
13
14 {% if diff_files.is_empty() %}
15 <p style="color: #666;">No diff available for this patch. Its head commit may no longer be in this repository.</p>
16 {% else %}
17 <p style="color: #666;">{{ file_count }} file{% if file_count != 1 %}s{% endif %} changed</p>
18 {% include "diff_files.html" %}
19 {% endif %}
20 {% endblock %}
tests/patch_diff_page_test.rs
Old New
@@ -0,0 +1,127 @@
1 //! The web view of a patch's *whole* change, not just its head commit.
2 //!
3 //! `/{repo}/diff/{oid}` is a single-commit view — it diffs a commit against its
4 //! first parent — and the patch page linked its head commit there. On a patch
5 //! of seven commits that renders one commit and one file while `patch diff
6 //! --stat` reports thirty-three, so the page silently disagreed with the record
7 //! it was displaying. The oracle here is that CLI: whatever `patch diff --stat`
8 //! names, the page must show.
9
10 mod common;
11
12 use common::ServerHarness;
13
14 /// A patch whose head commit touches *one* file while the patch touches three,
15 /// so a single-commit view and a range view cannot produce the same answer.
16 fn patch_of_three_commits(harness: &ServerHarness) -> String {
17 let repo = harness.work_repo();
18 repo.git(&["checkout", "-b", "feature"]);
19 repo.commit_file("first.rs", "pub fn first() {}\n", "add first");
20 repo.commit_file("second.rs", "pub fn second() {}\n", "add second");
21 repo.commit_file("third.rs", "pub fn third() {}\n", "add third");
22 let out = repo.run_ok(&["patch", "create", "-t", "Three commits", "-B", "feature"]);
23 repo.git(&["checkout", "main"]);
24 harness.push_head();
25 repo.git(&["push", "origin", "feature"]);
26 harness.push_collab_refs();
27 out.trim()
28 .strip_prefix("Created patch ")
29 .expect("patch create prints the id")
30 .to_string()
31 }
32
33 /// The file paths `patch diff --stat` names, which is what the page must match.
34 fn stat_paths(harness: &ServerHarness, id: &str) -> Vec<String> {
35 harness
36 .work_repo()
37 .run_ok(&["patch", "diff", id, "--stat"])
38 .lines()
39 .filter_map(|line| line.split_once('|'))
40 .map(|(path, _)| path.trim().to_string())
41 .collect()
42 }
43
44 #[test]
45 fn patch_diff_page_shows_every_file_the_patch_touches() {
46 let harness = ServerHarness::new("range");
47 let id = patch_of_three_commits(&harness);
48
49 let paths = stat_paths(&harness, &id);
50 assert_eq!(
51 paths,
52 vec!["first.rs", "second.rs", "third.rs"],
53 "the CLI oracle itself must see all three files"
54 );
55
56 let page = harness.get_ok(&format!("/range/patches/{id}/diff"));
57 for path in &paths {
58 assert!(
59 page.body.contains(path.as_str()),
60 "`patch diff --stat` names {path}, so the patch diff page must show it; \
61 a page missing it is showing one commit instead of the patch"
62 );
63 }
64 }
65
66 #[test]
67 fn patch_detail_links_to_the_range_diff() {
68 let harness = ServerHarness::new("range");
69 let id = patch_of_three_commits(&harness);
70
71 let page = harness.get_ok(&format!("/range/patches/{id}"));
72 let href = page
73 .body
74 .split("href=\"")
75 .filter_map(|rest| rest.split_once('"'))
76 .map(|(href, _)| href)
77 .find(|href| href.contains("/patches/") && href.ends_with("/diff"))
78 .unwrap_or_else(|| {
79 panic!(
80 "the patch page must offer the patch's own diff; without the link the \
81 only diff a reader can reach is one commit of it"
82 )
83 })
84 .to_string();
85
86 let linked = harness.get_ok(&href);
87 assert!(
88 linked.body.contains("first.rs"),
89 "the link from the patch page must reach the patch's whole diff, not one commit \
90 of it: {href} does not show the first commit's file"
91 );
92 }
93
94 /// A patch whose branch was never pushed — the shape an agent worktree leaves
95 /// behind when its branch is deleted after `patch create`. The commits reach
96 /// the server only as revision refs, so a diff that resolved its head by
97 /// branch name would find nothing to show.
98 #[test]
99 fn patch_diff_page_works_without_the_head_branch() {
100 let harness = ServerHarness::new("range");
101 let repo = harness.work_repo();
102 repo.git(&["checkout", "-b", "gone"]);
103 repo.commit_file("orphan.rs", "pub fn orphan() {}\n", "add orphan");
104 let out = repo.run_ok(&["patch", "create", "-t", "Never pushed", "-B", "gone"]);
105 repo.git(&["checkout", "main"]);
106 let id = out
107 .trim()
108 .strip_prefix("Created patch ")
109 .expect("patch create prints the id")
110 .to_string();
111 harness.push_head();
112 harness.push_collab_refs();
113 assert!(
114 !harness
115 .work_repo()
116 .git(&["ls-remote", "--heads", "origin", "gone"])
117 .contains("gone"),
118 "the branch must be absent on the server for this test to mean anything"
119 );
120
121 let page = harness.get_ok(&format!("/range/patches/{id}/diff"));
122 assert!(
123 page.body.contains("orphan.rs"),
124 "the patch's commits reached the server as revision refs, so its diff must \
125 render without the branch that once named them"
126 );
127 }