From 74f3365aa070799151f5af3ce66e347ef97ea4dd Mon Sep 17 00:00:00 2001 From: Peer Sommerlund Date: Sat, 11 Jul 2026 09:30:29 +0200 Subject: [PATCH 1/2] Test if topology order is violated In case of a diamond graph where the common ancestor has a newer date than some of its children, the walk order of the current code will violate topology order. --- asyncgit/src/sync/branch/merge_commit.rs | 109 ++++++++++++++++++++++- 1 file changed, 108 insertions(+), 1 deletion(-) diff --git a/asyncgit/src/sync/branch/merge_commit.rs b/asyncgit/src/sync/branch/merge_commit.rs index ec1ea498bb..4733d37023 100644 --- a/asyncgit/src/sync/branch/merge_commit.rs +++ b/asyncgit/src/sync/branch/merge_commit.rs @@ -101,7 +101,7 @@ mod test { branch_compare_upstream, remotes::{fetch, push::push_branch}, tests::{ - debug_cmd_print, get_commit_ids, repo_clone, + debug_cmd_print, get_commit_ids, repo_clone, repo_init, repo_init_bare, write_commit_file, write_commit_file_at, }, RepoState, @@ -278,4 +278,111 @@ mod test { let commits = get_commit_ids(&clone1, 10); assert_eq!(commits.len(), 1); } + + /// Verify that the walker preserves topology order. This means that + /// no parent is visited before any of its children. + #[test] + fn test_topology_order() { + /* Test case + a diamon shaped graph, where the common ancestor is younger than + one of its children. + + GP--P1--M + \ / + --P2 + + */ + + let (_repo_dir, repo) = repo_init().unwrap(); + + // 1. Grandparent (GP) - Newest + let gp = write_commit_file_at( + &repo, + "gp.txt", + "gp", + "gp", + Time::new(1000, 0), + ); + + // 2. Parent 1 (P1) - Older + let p1 = write_commit_file_at( + &repo, + "p1.txt", + "p1", + "p1", + Time::new(500, 0), + ); + + // 3. Parent 2 (P2) - Older (diverging from GP) + // Reset HEAD to GP so P2 becomes a child of GP + repo.reset( + repo.find_object(gp.into(), None) + .unwrap() + .as_commit() + .unwrap() + .as_object(), + git2::ResetType::Hard, + None, + ) + .unwrap(); + let p2 = write_commit_file_at( + &repo, + "p2.txt", + "p2", + "p2", + Time::new(400, 0), + ); + + // 4. Merge commit (M) - The starting point of our walk + // The heap now contains [p1, p2]. + // If we pop p1, we add gp. The heap is [gp, p2]. + // Because gp(1000) > p2(400), the walker returns gp before p2. + // This is a violation: p2 is a child of gp and must be visited first. + let p1_commit = repo.find_commit(p1.into()).unwrap(); + let p2_commit = repo.find_commit(p2.into()).unwrap(); + let tree = repo + .find_tree(repo.index().unwrap().write_tree().unwrap()) + .unwrap(); + let sig = repo.signature().unwrap(); + let m = repo + .commit( + Some("HEAD"), + &sig, + &sig, + "Merge p1 into p2", + &tree, + &[&p2_commit, &p1_commit], + ) + .unwrap(); + let m = CommitId::new(m); + + // Expected Topological Order: [M, P1, P2, GP] or [M, P2, P1, GP] + // Actual Defective Order: [M, P1, GP, P2] + // (GP jumps ahead of P2 because 1000 > 400) + + let commits = get_commit_ids(&repo, 14); + for (i, id) in commits.iter().enumerate() { + println!("DEBUG: commits[{}] = {:?}", i, id); + // Print the message of the commit to identify it + let repo_path = &repo.path().to_path_buf().into(); + let details = + crate::sync::get_commit_details(repo_path, *id) + .unwrap(); + println!( + "DEBUG: Message: {:?}", + details.message.map(|m| m.combine()) + ); + } + println!("DEBUG: Expected M is {:?}", m); + println!("DEBUG: P1 is {:?}", &p1); + println!("DEBUG: P2 is {:?}", &p2); + println!("DEBUG: GP is {:?}", &gp); + assert_eq!(commits[0], m); + assert!(commits.contains(&p1)); + assert!(commits.contains(&p2)); + assert_eq!( + commits[3], gp, + "Violation: Grandparent must be the last commit" + ); + } } From 5d9efeff47d9afbaee9dcbf47940fee121fb1bc8 Mon Sep 17 00:00:00 2001 From: Peer Sommerlund Date: Tue, 28 Jul 2026 07:32:23 +0200 Subject: [PATCH 2/2] Make sure topology order is preserved This is preparation for a log graph implementation. If commits are sorted by commit time, a graph may have to draw a parent before its child. This is confusing to the user and require extra memory for the graph render algorithm. --- asyncgit/src/sync/logwalker.rs | 82 ++++++++++++---------------------- 1 file changed, 29 insertions(+), 53 deletions(-) diff --git a/asyncgit/src/sync/logwalker.rs b/asyncgit/src/sync/logwalker.rs index 81a6fd321e..875c19d9ef 100644 --- a/asyncgit/src/sync/logwalker.rs +++ b/asyncgit/src/sync/logwalker.rs @@ -1,66 +1,50 @@ use super::{CommitId, SharedCommitFilterFn}; use crate::error::Result; -use git2::{Commit, Oid, Repository}; +use git2::{Repository, Revwalk, Sort}; use gix::revision::Walk; -use std::{ - cmp::Ordering, - collections::{BinaryHeap, HashSet}, -}; -struct TimeOrderedCommit<'a>(Commit<'a>); - -impl Eq for TimeOrderedCommit<'_> {} - -impl PartialEq for TimeOrderedCommit<'_> { - fn eq(&self, other: &Self) -> bool { - self.0.time().eq(&other.0.time()) - } -} - -impl PartialOrd for TimeOrderedCommit<'_> { - fn partial_cmp(&self, other: &Self) -> Option { - Some(self.cmp(other)) - } -} - -impl Ord for TimeOrderedCommit<'_> { - fn cmp(&self, other: &Self) -> Ordering { - self.0.time().cmp(&other.0.time()) - } -} - -/// +/// Visit commits in topological order, and date order where possible. +/// If a filter is provided, only commits that pass the filter are returned. pub struct LogWalker<'a> { - commits: BinaryHeap>, - visited: HashSet, + /// Revision walk engine + walk: Revwalk<'a>, + /// Total number of commits that have been visited + visited_count: usize, + /// Upper limit on buffer size limit: usize, + /// The source of commits repo: &'a Repository, + /// Filter on which commits will be returned when reading filter: Option, } impl<'a> LogWalker<'a> { - /// + /// Create a new log walker + /// with an upper limit on number of commits visited in one batch. pub fn new(repo: &'a Repository, limit: usize) -> Result { - let c = repo.head()?.peel_to_commit()?; + let head = repo.head()?.peel_to_commit()?; - let mut commits = BinaryHeap::with_capacity(10); - commits.push(TimeOrderedCommit(c)); + let mut walk = repo.revwalk()?; + // TOPOLOGICAL + TIME guarantees parents come after children, + // and ties/independent branches are ordered by timestamp (--date-order). + walk.set_sorting(Sort::TOPOLOGICAL | Sort::TIME)?; + walk.push(head.id())?; Ok(Self { - commits, + walk, + visited_count: 0, limit, - visited: HashSet::with_capacity(1000), repo, filter: None, }) } - /// - pub fn visited(&self) -> usize { - self.visited.len() + /// Number of visited commits + pub const fn visited(&self) -> usize { + self.visited_count } - /// + /// Add a filter to use when reading commits #[must_use] pub fn filter( self, @@ -69,16 +53,14 @@ impl<'a> LogWalker<'a> { Self { filter, ..self } } - /// + /// Get a batch of commits pub fn read(&mut self, out: &mut Vec) -> Result { let mut count = 0_usize; - while let Some(c) = self.commits.pop() { - for p in c.0.parents() { - self.visit(p); - } + for oid_result in self.walk.by_ref() { + let oid = oid_result?; + let id: CommitId = oid.into(); - let id: CommitId = c.0.id().into(); let commit_should_be_included = if let Some(ref filter) = self.filter { filter(self.repo, &id)? @@ -90,6 +72,7 @@ impl<'a> LogWalker<'a> { out.push(id); } + self.visited_count += 1; count += 1; if count == self.limit { break; @@ -98,13 +81,6 @@ impl<'a> LogWalker<'a> { Ok(count) } - - // - fn visit(&mut self, c: Commit<'a>) { - if self.visited.insert(c.id()) { - self.commits.push(TimeOrderedCommit(c)); - } - } } /// This is separate from `LogWalker` because filtering currently (June 2024) works through