From 112604b6d382c2a54699561ca27181727ea1f677 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C5=ABdolfs=20O=C5=A1i=C5=86=C5=A1?= Date: Wed, 2 Sep 2026 17:48:01 +0200 Subject: [PATCH] Fix truncated commit list for merge-commit diffs The diff endpoint walked back from the head commit and stopped at the first commit equal to `base`. `surf::History` is an unhidden revwalk in commit-date order, so a head that merges the target branch in reaches `base` through one of its parents and truncates the walk there, dropping every commit the other parent contributes. A patch whose head is such a merge showed a single commit no matter how many it really carried. --- crates/radicle-httpd/src/api/v1/repos.rs | 58 ++++++-- crates/radicle-httpd/src/test.rs | 174 +++++++++++++++++++++++ 2 files changed, 219 insertions(+), 13 deletions(-) diff --git a/crates/radicle-httpd/src/api/v1/repos.rs b/crates/radicle-httpd/src/api/v1/repos.rs index 4fe87377e..08bcc8828 100644 --- a/crates/radicle-httpd/src/api/v1/repos.rs +++ b/crates/radicle-httpd/src/api/v1/repos.rs @@ -578,8 +578,8 @@ async fn diff_handler( ) -> impl IntoResponse { let rid = ctx.resolve_repo(&rid)?; let response = api::blocking(move || { - let (repo, _) = ctx.repo(rid)?; - let repo = Repository::open(repo.path())?; + let (storage, _) = ctx.repo(rid)?; + let repo = Repository::open(storage.path())?; let base = repo.commit(base)?; let commit = repo.commit(oid)?; let diff = repo.diff(base.id, commit.id)?; @@ -622,17 +622,21 @@ async fn diff_handler( } }); - let commits = repo - .history(commit.id)? - .take_while(|c| { - if let Ok(c) = c { - c.id != base.id - } else { - false - } - }) - .map(|r| r.map(|c| api::json::commit::Commit::new(&c).as_json())) - .collect::, _>>()?; + // Hide `base` from the walk rather than stopping at the first commit + // that equals it. A merge commit reaches `base` through one of its + // parents, so truncating there drops every commit the other parent + // contributes, and a revision whose head merges the target branch in + // looks like a single-commit revision. + let raw = radicle::git::raw::Repository::open(storage.path())?; + let mut walk = raw.revwalk()?; + walk.push(commit.id.into())?; + walk.hide(base.id.into())?; + + let commits = walk + .filter_map(|oid| oid.ok()) + .filter_map(|oid| repo.commit(Oid::from(oid)).ok()) + .map(|c| api::json::commit::Commit::new(&c).as_json()) + .collect::>(); Ok::<_, Error>(json!({ "diff": diff, "files": files, "commits": commits })) }) @@ -2481,6 +2485,34 @@ mod routes { ); } + /// The head of the diff is a merge commit whose second parent is the base. + /// A commit-date-ordered walk reaches the base first, which used to + /// truncate the list to the merge alone. + #[tokio::test] + async fn test_repos_diff_merge_commit() { + let tmp = tempfile::tempdir().unwrap(); + let MergeFixture { + ctx, + rid, + base, + head, + feature, + } = seed_merge(tmp.path()); + let app = super::router(ctx); + + let response = get(&app, format!("/repos/{rid}/diff/{base}/{head}")).await; + assert_eq!(response.status(), StatusCode::OK); + + let json = response.json().await; + let commits = json["commits"].as_array().unwrap(); + let ids = commits + .iter() + .map(|c| c["id"].as_str().unwrap().to_string()) + .collect::>(); + + assert_eq!(ids, vec![head.to_string(), feature.to_string()]); + } + #[tokio::test] async fn test_repos_diff_stats() { let tmp = tempfile::tempdir().unwrap(); diff --git a/crates/radicle-httpd/src/test.rs b/crates/radicle-httpd/src/test.rs index ed851ab86..28fc790e5 100644 --- a/crates/radicle-httpd/src/test.rs +++ b/crates/radicle-httpd/src/test.rs @@ -471,6 +471,180 @@ pub fn seed_multi_peer(dir: &Path) -> Context { ctx } +/// A repository whose default branch tip is a merge commit. +/// +/// Returned by [`seed_merge`]. +pub struct MergeFixture { + pub ctx: Context, + pub rid: radicle::identity::RepoId, + /// Tip of the merge target, used as the `base` of the diff. + pub base: radicle::git::Oid, + /// The merge commit, used as the `head` of the diff. + pub head: radicle::git::Oid, + /// The commit contributed by the merged-in side. + pub feature: radicle::git::Oid, +} + +/// Seed a repository whose default branch tip is a merge commit. +/// +/// The topology, with commit dates in parentheses: +/// +/// ```text +/// root (t1) ──┬── feature (t2) ──┐ +/// │ ├── merge (t4) <- `head`, tip of `master` +/// └── base (t3) ──┘ +/// ``` +/// +/// `base` is deliberately given a *newer* commit date than `feature`, so that a +/// commit-date-ordered revwalk from the merge reaches `base` before it reaches +/// `feature`. That is the case that used to truncate the commit list of +/// `GET /repos/:rid/diff/:base/:oid` down to just the merge itself. +pub fn seed_merge(dir: &Path) -> MergeFixture { + const DEFAULT_BRANCH: &str = "master"; + + fn tree<'a>( + repo: &'a radicle::git::raw::Repository, + entries: &[(&str, radicle::git::raw::Oid)], + ) -> radicle::git::raw::Tree<'a> { + let mut builder = repo.treebuilder(None).unwrap(); + for (name, oid) in entries { + builder + .insert(name, *oid, i32::from(radicle::git::raw::FileMode::Blob)) + .unwrap(); + } + repo.find_tree(builder.write().unwrap()).unwrap() + } + + crate::logger::init().ok(); + + let home = dir.join("radicle"); + let profile = profile(home.as_path(), [0xfe; 32]); + let signer = SigningKey::from_seed(Seed::new([0xfe; 32])); + + env::set_var(env::GIT_COMMITTER_DATE, TIMESTAMP.to_string()); + + let workdir = dir.join("hello-merge"); + fs::create_dir_all(&workdir).unwrap(); + + let mut opts = radicle::git::raw::RepositoryInitOptions::new(); + opts.initial_head(DEFAULT_BRANCH); + let repo = radicle::git::raw::Repository::init_opts(&workdir, &opts).unwrap(); + + let sig = |secs: i64| { + radicle::git::raw::Signature::new( + "Alice Liddell", + "alice@radicle.xyz", + &radicle::git::raw::Time::new(secs, 0), + ) + .unwrap() + }; + + // The two sides touch different files, so the merge tree is just the union + // of both and no merge machinery is needed to build it. + let readme = repo.blob("Hello World!\n".as_bytes()).unwrap(); + let feature_blob = repo.blob("A feature.\n".as_bytes()).unwrap(); + let base_blob = repo.blob("Moved on.\n".as_bytes()).unwrap(); + + let root_tree = tree(&repo, &[("README", readme)]); + let root = repo + .commit( + None, + &sig(1673001014), + &sig(1673001014), + "Initial commit\n", + &root_tree, + &[], + ) + .unwrap(); + let root = repo.find_commit(root).unwrap(); + + let feature_tree = tree(&repo, &[("README", readme), ("FEATURE", feature_blob)]); + let feature = repo + .commit( + None, + &sig(1673002014), + &sig(1673002014), + "Add a feature\n", + &feature_tree, + &[&root], + ) + .unwrap(); + + let base_tree = tree(&repo, &[("README", readme), ("MASTER", base_blob)]); + let base = repo + .commit( + None, + &sig(1673003014), + &sig(1673003014), + "Advance master\n", + &base_tree, + &[&root], + ) + .unwrap(); + + let merge_tree = tree( + &repo, + &[ + ("README", readme), + ("FEATURE", feature_blob), + ("MASTER", base_blob), + ], + ); + let head = repo + .commit( + None, + &sig(1673004014), + &sig(1673004014), + "Merge branch 'master' into feature\n", + &merge_tree, + &[ + &repo.find_commit(feature).unwrap(), + &repo.find_commit(base).unwrap(), + ], + ) + .unwrap(); + + // Point the default branch at the merge so that `rad init` pushes the whole + // topology into storage. + let branch = format!("refs/heads/{DEFAULT_BRANCH}"); + repo.reference(&branch, head, true, "test: point master at the merge") + .unwrap(); + repo.set_head(&branch).unwrap(); + + let mut checkout = radicle::git::raw::build::CheckoutBuilder::new(); + repo.checkout_head(Some(checkout.force())).unwrap(); + + let repo = radicle::git::raw::Repository::open(&workdir).unwrap(); + let (rid, _, _) = radicle::rad::init( + &repo, + project::ProjectName::from_str("hello-merge").unwrap(), + "Rad repository whose head is a merge commit", + RefString::try_from(DEFAULT_BRANCH).unwrap(), + Visibility::default(), + &signer, + &profile.storage, + ) + .unwrap(); + + let options = crate::Options { + aliases: std::collections::HashMap::new(), + listen: axum_listener::DualAddr::Tcp(std::net::SocketAddr::from(([0, 0, 0, 0], 8080))), + cache: Some(crate::DEFAULT_CACHE_SIZE), + search: None, + }; + let web_config = crate::api::WebConfig::from_profile(&profile); + let ctx = Context::new(Arc::new(profile), web_config, &options) + .expect("test context creation failed"); + + MergeFixture { + ctx, + rid, + base: base.into(), + head: head.into(), + feature: feature.into(), + } +} + pub async fn get(app: &Router, path: impl ToString) -> Response { Response( app.clone()