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" + ); + } } 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