From bda8e026717d0963bdda6a0323d35f6317191d16 Mon Sep 17 00:00:00 2001 From: Codex GPT-6 Date: Thu, 17 Sep 2026 13:47:48 +0100 Subject: [PATCH] fix(gix-revision): stop merge-base walks when one paint side is exhausted The merge-base walk kept following unrelated ancestry while any queued commit remained non-stale, even after one paint color could no longer contribute a merge base. Its termination check also scanned the queue on every iteration. Adapt Git's side-exhaustion optimization from `02d7ba092d`: count non-stale queued commits carrying each color and stop when either count reaches zero in the topologically ordered region. Candidates contribute to both counts so they are processed before termination. Track queue membership to handle flag changes and duplicate inputs without counting a commit twice. Keep the existing stale-only termination for missing, zero, or saturated generations, where date ordering can revive an apparently exhausted color. Continue removing redundant candidates after the paint walk. Use existing fixtures to prove that unrelated ancestry is skipped and cover pending candidates, duplicate inputs, graph reuse, and clock skew. The skipped-ancestry regression fails before this change. Validation: - `GIX_TEST_IGNORE_ARCHIVES=1 cargo test -p gix-revision --all-features` - The same 114 tests with `GIX_TEST_FIXTURE_HASH=sha256` - `cargo fmt --all -- --check` - `cargo clippy -p gix-revision --all-features --all-targets -- -D warnings -A unknown-lints --no-deps` Clippy without `--no-deps` encounters a pre-existing `collapsible_if` warning in `gix-tempfile/src/registry.rs`. --- gix-revision/src/merge_base/function.rs | 75 +++++++++++++++++++---- gix-revision/src/merge_base/mod.rs | 2 + gix-revision/tests/revision/merge_base.rs | 75 +++++++++++++++++++++++ 3 files changed, 140 insertions(+), 12 deletions(-) diff --git a/gix-revision/src/merge_base/function.rs b/gix-revision/src/merge_base/function.rs index ba7b0cfd200..f05d30f441b 100644 --- a/gix-revision/src/merge_base/function.rs +++ b/gix-revision/src/merge_base/function.rs @@ -22,6 +22,8 @@ use crate::{Graph, PriorityQueue, merge_base::Flags}; /// /// For repeated calls, be sure to re-use `graph` as its content will be kept and reused for a great speed-up. The contained flags /// will automatically be cleared. +/// With a commit-graph providing nonzero, unsaturated generations, the walk stops once either side's non-stale +/// frontier is exhausted and no merge-base candidates remain queued. pub fn merge_base( first: ObjectId, others: &[ObjectId], @@ -148,34 +150,84 @@ fn remove_redundant( .collect()) } +struct PaintQueue { + queue: PriorityQueue, + /// Non-stale queued commits carrying each color. Candidates count toward both sides, keeping the walk alive + /// until they have been recorded, even if no exclusively colored commits remain on one side. + non_stale: [usize; 2], +} + +impl PaintQueue { + fn update_counts(&mut self, flags: Flags, add: bool) { + if flags.contains(Flags::STALE) { + return; + } + for (side, count) in [Flags::COMMIT1, Flags::COMMIT2].into_iter().zip(&mut self.non_stale) { + if flags.contains(side) { + if add { + *count += 1; + } else { + *count -= 1; + } + } + } + } + + fn insert(&mut self, commit_id: ObjectId, commit: &mut graph::Commit, flags: Flags) { + if commit.data.contains(Flags::ENQUEUED) { + self.update_counts(commit.data, false); + } else { + self.queue.insert(GenThenTime::from(&*commit), commit_id); + commit.data |= Flags::ENQUEUED; + } + commit.data |= flags; + self.update_counts(commit.data, true); + } + + fn pop(&mut self, graph: &mut Graph<'_, '_, graph::Commit>) -> Option<(GenThenTime, ObjectId)> { + let (info, commit_id) = self.queue.pop()?; + // Keep this commit counted until after the exit check so the last pending candidate is processed. + // Side exhaustion is only final when children are visited before parents. Missing, zero, or saturated + // generations fall back to date ordering, which can propagate a color to an already visited commit. + if self.non_stale == [0, 0] + || (self.non_stale.contains(&0) + && info.generation > 0 + && info.generation < gix_commitgraph::GENERATION_NUMBER_MAX) + { + return None; + } + let commit = graph.get_mut(&commit_id).expect("everything queued is in graph"); + commit.data.remove(Flags::ENQUEUED); + self.update_counts(commit.data, false); + Some((info, commit_id)) + } +} + fn paint_down_to_common( first: ObjectId, others: &[ObjectId], graph: &mut Graph<'_, '_, graph::Commit>, ) -> Result, Error> { - let mut queue = PriorityQueue::::new(); + let mut queue = PaintQueue { + queue: PriorityQueue::new(), + non_stale: [0; 2], + }; graph .get_or_insert_full_commit(first, |commit| { - commit.data |= Flags::COMMIT1; - queue.insert(GenThenTime::from(&*commit), first); + queue.insert(first, commit, Flags::COMMIT1); }) .map_err(|_| Simple("could not insert commit into graph"))?; for other in others { graph .get_or_insert_full_commit(*other, |commit| { - commit.data |= Flags::COMMIT2; - queue.insert(GenThenTime::from(&*commit), *other); + queue.insert(*other, commit, Flags::COMMIT2); }) .map_err(|_| Simple("could not insert commit into graph"))?; } let mut out = Vec::new(); - while queue - .iter_unordered() - .any(|id| graph.get(id).is_some_and(|commit| !commit.data.contains(Flags::STALE))) - { - let (info, commit_id) = queue.pop().expect("we have non-stale"); + while let Some((info, commit_id)) = queue.pop(graph) { let commit = graph.get_mut(&commit_id).expect("everything queued is in graph"); let mut flags_without_result = commit.data & (Flags::COMMIT1 | Flags::COMMIT2 | Flags::STALE); if flags_without_result == (Flags::COMMIT1 | Flags::COMMIT2) { @@ -190,8 +242,7 @@ fn paint_down_to_common( graph .get_or_insert_full_commit(parent_id, |parent| { if (parent.data & flags_without_result) != flags_without_result { - parent.data |= flags_without_result; - queue.insert(GenThenTime::from(&*parent), parent_id); + queue.insert(parent_id, parent, flags_without_result); } }) .map_err(|_| Simple("could not insert parent commit into graph"))?; diff --git a/gix-revision/src/merge_base/mod.rs b/gix-revision/src/merge_base/mod.rs index a42c0a30214..3599d27c66e 100644 --- a/gix-revision/src/merge_base/mod.rs +++ b/gix-revision/src/merge_base/mod.rs @@ -11,6 +11,8 @@ bitflags::bitflags! { const STALE = 1 << 2; /// The commit was already put ontto the results list. const RESULT = 1 << 3; + /// The commit is currently in the paint queue. + const ENQUEUED = 1 << 4; } } diff --git a/gix-revision/tests/revision/merge_base.rs b/gix-revision/tests/revision/merge_base.rs index 1cb6eafb3f8..61881e1dd8e 100644 --- a/gix-revision/tests/revision/merge_base.rs +++ b/gix-revision/tests/revision/merge_base.rs @@ -40,6 +40,81 @@ fn validate() -> crate::Result { Ok(()) } +#[test] +fn exhausted_side_skips_unrelated_history() -> crate::Result { + let root = gix_testtools::scripted_fixture_read_only("make_merge_base_repos.sh")?; + let odb = odb_at(root.join(".git/objects"))?; + let tip_commit_id = tag_commit_id(&root, "PL")?; + let base_commit_id = tag_commit_id(&root, "C2")?; + let unrelated_commit_id = tag_commit_id(&root, "L0")?; + + // PL merges the C and L chains. Once C2 is found, the rest of L cannot + // provide another merge base, even though its queued commits are not stale. + for use_commitgraph in [false, true] { + let cache = use_commitgraph + .then(|| gix_commitgraph::Graph::from_info_dir(&odb.store_ref().path().join("info"))) + .transpose()?; + for (first_commit_id, other_commit_id) in [(tip_commit_id, base_commit_id), (base_commit_id, tip_commit_id)] { + let mut graph = gix_revision::Graph::new(&odb, cache.as_ref()); + for others in [ + &[other_commit_id][..], + &[other_commit_id, other_commit_id][..], + &[other_commit_id][..], + ] { + assert_eq!( + merge_base(first_commit_id, others, &mut graph)?, + Some(nonempty::NonEmpty::new(base_commit_id)), + "the pending common ancestor survives side exhaustion, duplicates, and graph reuse" + ); + assert_eq!( + graph.contains(&unrelated_commit_id), + !use_commitgraph, + "only reliable generation ordering lets the walk skip the unrelated L0 ancestor" + ); + } + } + } + Ok(()) +} + +#[test] +fn unreliable_generations_do_not_allow_side_exhaustion() -> crate::Result { + let root = gix_testtools::scripted_fixture_read_only("make_merge_base_repos.sh")?; + let odb = odb_at(root.join(".git/objects"))?; + // G and H share B, but clock skew visits B's ancestor E first. Missing, + // zero, or saturated generations cannot prevent an exhausted color from returning. + // Zero represents a legacy commit-graph without computed generations. + let first_commit_id = tag_commit_id(&root, "G")?; + let other_commit_id = tag_commit_id(&root, "H")?; + let base_commit_id = tag_commit_id(&root, "B")?; + for generation in [None, Some(0), Some(gix_commitgraph::GENERATION_NUMBER_MAX)] { + let mut graph = gix_revision::Graph::new(&odb, None); + for name in ["A", "B", "C", "D", "E", "F", "G", "H"] { + graph.get_or_insert_full_commit(tag_commit_id(&root, name)?, |commit| { + commit.generation = generation; + })?; + } + for (first_commit_id, other_commit_id) in + [(first_commit_id, other_commit_id), (other_commit_id, first_commit_id)] + { + assert_eq!( + merge_base(first_commit_id, &[other_commit_id, other_commit_id], &mut graph)?, + Some(nonempty::NonEmpty::new(base_commit_id)), + "generation {generation:?} requires finishing the date-ordered walk despite temporary side exhaustion" + ); + } + } + Ok(()) +} + +fn tag_commit_id(root: &std::path::Path, name: &str) -> crate::Result { + Ok(gix_hash::ObjectId::from_hex( + std::fs::read_to_string(root.join(".git/refs/tags").join(name))? + .trim() + .as_bytes(), + )?) +} + mod octopus { use crate::{hex_to_id, odb_at};