From 3148baf1e89c20ed9cd41f42bdaa5eb8f60065f6 Mon Sep 17 00:00:00 2001 From: Seongmin Lee Date: Mon, 28 Sep 2026 20:59:57 +0900 Subject: [PATCH] gix-pack,gitmirror: exclude objects in boundary trees from pushed packs Signed-off-by: Seongmin Lee --- .../crates/gitmirror-xrpc/src/routes/git.rs | 99 ++++---- .../gix-pack/src/data/output/count/mod.rs | 7 +- .../src/data/output/count/objects/mod.rs | 2 + .../data/output/count/objects/reachable.rs | 216 ++++++++++++++++++ 4 files changed, 269 insertions(+), 55 deletions(-) create mode 100644 knot2/third_party/gix-pack/src/data/output/count/objects/reachable.rs diff --git a/gitmirror/crates/gitmirror-xrpc/src/routes/git.rs b/gitmirror/crates/gitmirror-xrpc/src/routes/git.rs index 14dd2ad68..5c47aefaf 100644 --- a/gitmirror/crates/gitmirror-xrpc/src/routes/git.rs +++ b/gitmirror/crates/gitmirror-xrpc/src/routes/git.rs @@ -614,9 +614,8 @@ fn commit_log_walk<'repo>( } } } else { - let peel = |id| -> anyhow::Result { - Ok(repo.find_object(id)?.peel_to_commit()?.id) - }; + let peel = + |id| -> anyhow::Result { Ok(repo.find_object(id)?.peel_to_commit()?.id) }; for range in ranges { let revspec = repo.rev_parse(range.as_bstr())?; let spec = revspec.detach(); @@ -1001,31 +1000,14 @@ fn safe_knot_host(endpoint: &str, policy: KnotPolicy) -> Result String { let url = knot.url(); match url.port() { - // `did:web` percent-encodes the port separator; everything after it is the method-specific - // id, where a bare `:` would read as a path segment. Some(port) => format!("did:web:{}%3A{port}", url.host_str().unwrap_or_default()), None => format!("did:web:{}", url.host_str().unwrap_or_default()), } } -/// The packfile the knot is missing: every object reachable from `new_commit_id` that neither -/// `hidden` nor anything `advertised` already accounts for — its ref tips and its `.have` lines, -/// which name the tips of its alternates. -/// -/// Only the ids this repository can actually load make it into the exclusion set: the walk reads -/// each one to mark its ancestry uninteresting, so an advertised tip a lagging mirror does not yet -/// have would otherwise fail the walk rather than merely widen the pack. -/// -/// `None` when the knot already holds every object the update names, which a fast-forward within -/// one repository is: there is then nothing to build a pack out of. A knot reads the body to its -/// end and stages nothing (`knot-pack::receive::stage_pack_bytes` treats empty as no pack), so the -/// body simply stops after the command block. fn build_pack( scratch: &gitmirror_git::scratch::TempRepository, advertised: &gix_receive_pack::ReceivePackAdvertisement, @@ -1052,42 +1034,28 @@ fn build_pack( let mut objects = scratch.objects.clone().into_inner(); objects.prevent_pack_unload(); - // The walk loads every hidden id to mark its ancestry uninteresting, so one it cannot load - // fails the whole walk — and the knot advertises refs a mirror that is behind may not have yet. - // Peeling is what turns an advertised annotated tag into the commit the walk wants. - let hidden: Vec<_> = hidden + let haves = hidden .into_iter() + // TODO: chain synced ref tips in local mirror .chain(advertised.remote_refs.values().copied()) - .chain(advertised.advertised_haves.iter().copied()) - .filter_map(|id| Some(scratch.find_object(id).ok()?.peel_to_commit().ok()?.id)) - .collect(); - let commits: Vec = scratch - .rev_walk([new_commit_id]) - .with_hidden(hidden) - .all()? - .map(|info| info.map(|info| info.id)) - .collect::>()?; - if commits.is_empty() { - return Ok(None); - } - - // ponytail: the exclusion is commit-level only, so an object the knot already has can still be - // sent when a commit's own diff reintroduces it — `gix-pack` keeps the set of objects it has - // seen private and offers no way to seed it with the remote's. The fix is hand-rolled counting - // straight into `Vec`. - let (counts, _) = output::count::objects( + .chain(advertised.advertised_haves.iter().copied()); + let (counts, outcome) = output::count::reachable( objects.clone(), - Box::new(commits.into_iter().map(Ok)), + [new_commit_id], + haves, &Discard, &gix::interrupt::IS_INTERRUPTED, - output::count::objects::Options { - // TODO(boltless): get thread limit like knot does - thread_limit: Some(1), - chunk_size: CHUNK_SIZE, - input_object_expansion: - output::count::objects::ObjectExpansion::TreeAdditionsComparedToAncestor, - }, )?; + info!( + commits = outcome.commits, + boundary = outcome.boundary, + objects = outcome.objects, + ignored_haves = outcome.ignored_haves, + "counted pack" + ); + if counts.is_empty() { + return Ok(None); + } let num_entries = u32::try_from(counts.len()) .map_err(|_| anyhow::anyhow!("{} objects do not fit into a pack", counts.len()))?; @@ -1105,12 +1073,8 @@ fn build_pack( }, ); - // Entries have to reach the writer in the order they were counted, because an offset delta - // refers to its base by index into that very sequence. let mut in_order = InOrderIter::from(counted); let mut pack = Vec::new(); - // The `PACK` header and the trailing checksum are the writer's own doing; driving it to - // exhaustion is what produces them. let mut writer = output::bytes::FromEntriesIter::new( in_order.by_ref(), &mut pack, @@ -1776,6 +1740,33 @@ mod merge_commit_tests { drop(fixture.root); } + #[test] + fn merging_what_the_knot_has_sends_only_the_merge() { + let fixture = two_mirrors("line1\nline2\nTARGET\n", "SOURCE\nline2\nline3\n"); + let repo = scratch(&fixture); + let rebased = repo + .rebase(fixture.source, fixture.onto, committer()) + .expect("a clean rebase"); + let tree = repo.find_commit(rebased).unwrap().tree_id().unwrap(); + let merge = repo + .new_commit("merge", tree, [fixture.onto, fixture.source]) + .unwrap() + .id; + + let pack = build_pack( + &repo, + &advertisement(fixture.onto), + merge, + vec![fixture.onto, fixture.source], + ) + .unwrap() + .unwrap(); + // The merge commit, its root tree and the merged `a.txt`; `source.txt` is in a parent. + let entries = u32::from_be_bytes(pack[8..12].try_into().unwrap()); + assert_eq!(entries, 3); + drop(fixture.root); + } + #[test] fn a_knot_endpoint_becomes_a_base_url_and_only_a_public_https_one() { let deployed = KnotPolicy { diff --git a/knot2/third_party/gix-pack/src/data/output/count/mod.rs b/knot2/third_party/gix-pack/src/data/output/count/mod.rs index ff9966362..bc22343b8 100644 --- a/knot2/third_party/gix-pack/src/data/output/count/mod.rs +++ b/knot2/third_party/gix-pack/src/data/output/count/mod.rs @@ -44,9 +44,14 @@ impl Count { #[path = "objects/mod.rs"] mod objects_impl; -pub use objects_impl::{objects, objects_unthreaded}; +pub use objects_impl::{objects, objects_unthreaded, reachable::reachable}; /// pub mod objects { pub use super::objects_impl::{Error, ObjectExpansion, Options, Outcome}; } + +/// +pub mod reachable { + pub use super::objects_impl::reachable::{Error, Outcome}; +} diff --git a/knot2/third_party/gix-pack/src/data/output/count/objects/mod.rs b/knot2/third_party/gix-pack/src/data/output/count/objects/mod.rs index bc1bdbf91..41a34b556 100644 --- a/knot2/third_party/gix-pack/src/data/output/count/objects/mod.rs +++ b/knot2/third_party/gix-pack/src/data/output/count/objects/mod.rs @@ -13,6 +13,8 @@ pub use types::{Error, ObjectExpansion, Options, Outcome}; mod tree; +pub(in crate::data::output::count) mod reachable; + /// Generate [`Count`][output::Count]s from input `objects` with object expansion based on [`options`][Options] /// to learn which objects would constitute a pack. This step is required to know exactly how many objects would /// be in a pack while keeping data around to minimize database object access. diff --git a/knot2/third_party/gix-pack/src/data/output/count/objects/reachable.rs b/knot2/third_party/gix-pack/src/data/output/count/objects/reachable.rs new file mode 100644 index 000000000..a42fa5223 --- /dev/null +++ b/knot2/third_party/gix-pack/src/data/output/count/objects/reachable.rs @@ -0,0 +1,216 @@ +use std::sync::atomic::{AtomicBool, Ordering}; + +use gix_hash::{ObjectId, oid}; +use gix_hashtable::HashSet; +use gix_object::{CommitRefIter, Kind, TagRefIter, TreeRefIter, bstr::BStr, tree::EntryRef}; +use gix_traverse::tree::{Visit, breadthfirst, visit::Action}; + +use crate::{ + FindExt, + data::output::{self, count::PackLocation}, +}; + +/// The error returned by [`reachable()`]. +#[derive(Debug, thiserror::Error)] +#[allow(missing_docs)] +pub enum Error { + #[error(transparent)] + FindExisting(#[from] gix_object::find::existing::Error), + #[error("want {id} could not be peeled to a commit")] + UnpeelableWant { id: ObjectId }, + #[error(transparent)] + Walk(#[from] gix_traverse::commit::simple::Error), + #[error(transparent)] + TreeTraverse(#[from] breadthfirst::Error), + #[error("Operation interrupted")] + Interrupted, +} + +/// Statistics of a [`reachable()`] run. +#[derive(Default, PartialEq, Eq, Debug, Hash, Ord, PartialOrd, Clone, Copy)] +pub struct Outcome { + /// Commits reachable from the wants but not the haves. + pub commits: usize, + /// Commits the receiver has that the new ones have as parents. + pub boundary: usize, + /// The amount of counts returned. + pub objects: usize, + /// Haves that are missing from `db` or don't peel to a commit, and were ignored. + pub ignored_haves: usize, +} + +/// Count the objects reachable from `wants` that a receiver holding `haves` lacks. +pub fn reachable( + db: Find, + wants: impl IntoIterator, + haves: impl IntoIterator, + progress: &dyn gix_features::progress::Count, + should_interrupt: &AtomicBool, +) -> Result<(Vec, Outcome), Error> +where + Find: crate::Find + gix_object::Find, +{ + let progress = progress.counter(); + let mut outcome = Outcome::default(); + let mut buf = Vec::new(); + let mut out = Vec::new(); + + let mut want_commits = Vec::new(); + for id in wants { + want_commits.push( + peel_to_commit(&db, id, &mut buf, Some(&mut out)) + .ok_or(Error::UnpeelableWant { id })?, + ); + } + let mut have_commits = Vec::new(); + for id in haves { + match peel_to_commit(&db, id, &mut buf, None) { + Some(id) => have_commits.push(id), + None => outcome.ignored_haves += 1, + } + } + + let mut commits = Vec::new(); + let mut parents = Vec::new(); + for info in gix_traverse::commit::Simple::new(want_commits, &db).hide(have_commits)? { + let info = info?; + parents.extend(info.parent_ids); + commits.push(info.id); + } + outcome.commits = commits.len(); + + let mut seen: HashSet = commits.iter().copied().collect(); + seen.extend(out.iter().copied()); + let mut state = breadthfirst::State::default(); + for parent in parents { + if seen.insert(parent) { + outcome.boundary += 1; + add_tree( + &db, + &parent, + &mut state, + &mut buf, + &mut Collect { + seen: &mut seen, + out: None, + }, + )?; + } + } + for commit in commits { + if should_interrupt.load(Ordering::Relaxed) { + return Err(Error::Interrupted); + } + out.push(commit); + let before = out.len(); + add_tree( + &db, + &commit, + &mut state, + &mut buf, + &mut Collect { + seen: &mut seen, + out: Some(&mut out), + }, + )?; + progress.fetch_add(1 + out.len() - before, Ordering::Relaxed); + } + + outcome.objects = out.len(); + let counts = out + .into_iter() + .map(|id| output::Count { + id, + entry_pack_location: PackLocation::NotLookedUp, + }) + .collect(); + Ok((counts, outcome)) +} + +fn peel_to_commit( + db: &impl crate::Find, + mut id: ObjectId, + buf: &mut Vec, + mut tags: Option<&mut Vec>, +) -> Option { + loop { + let (obj, _) = db.try_find(&id, buf).ok()??; + match obj.kind { + Kind::Commit => return Some(id), + Kind::Tag => { + if let Some(tags) = tags.as_deref_mut() { + tags.push(id); + } + id = TagRefIter::from_bytes(obj.data, obj.object_hash) + .target_id() + .expect("every tag has a target"); + } + _ => return None, + } + } +} + +fn add_tree( + db: &Find, + commit: &oid, + state: &mut breadthfirst::State, + buf: &mut Vec, + collect: &mut Collect<'_>, +) -> Result<(), Error> +where + Find: crate::Find + gix_object::Find, +{ + let (obj, _) = db.find(commit, buf)?; + let tree = CommitRefIter::from_bytes(obj.data, obj.object_hash) + .tree_id() + .expect("every commit has a tree"); + if !collect.add(&tree) { + return Ok(()); + } + let (obj, _) = db.find(&tree, buf)?; + breadthfirst( + TreeRefIter::from_bytes(obj.data, obj.object_hash), + state, + db, + collect, + )?; + Ok(()) +} + +struct Collect<'a> { + seen: &'a mut HashSet, + out: Option<&'a mut Vec>, +} + +impl Collect<'_> { + fn add(&mut self, id: &oid) -> bool { + let new = self.seen.insert(id.to_owned()); + if let (true, Some(out)) = (new, self.out.as_deref_mut()) { + out.push(id.to_owned()); + } + new + } +} + +impl Visit for Collect<'_> { + fn pop_back_tracked_path_and_set_current(&mut self) {} + + fn pop_front_tracked_path_and_set_current(&mut self) {} + + fn push_back_tracked_path_component(&mut self, _component: &BStr) {} + + fn push_path_component(&mut self, _component: &BStr) {} + + fn pop_path_component(&mut self) {} + + fn visit_tree(&mut self, entry: &EntryRef<'_>) -> Action { + std::ops::ControlFlow::Continue(self.add(entry.oid)) + } + + fn visit_nontree(&mut self, entry: &EntryRef<'_>) -> Action { + if !entry.mode.is_commit() { + self.add(entry.oid); + } + std::ops::ControlFlow::Continue(true) + } +} -- 2.51.2