Change walk order from ByCommitTime to topological order

CHANGELOG.md

@@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * open the external editor from the status diff view [[@WaterWhisperer](https://github.com/WaterWhisperer)] ([#2805](https://github.com/gitui-org/gitui/issues/2805))
 * automatically convert spaces to dashes when creating or renaming a branch [[@pbouillon]](https//pbouillon.github.io)] ([#2916](https://github.com/gitui-org/gitui/pull/2916))
 * support rewording non-HEAD commits when `commit.gpgsign` is enabled (gpg format only) [[@guerinoni](https://github.com/guerinoni)] ([#2959](https://github.com/gitui-org/gitui/pull/2959))
+* always preserve topology order before sorting commits by date
 
 ### Fixes
 * crash when opening submodule ([#2895](https://github.com/gitui-org/gitui/issues/2895))

asyncgit/src/error.rs

@@ -73,6 +73,10 @@ pub enum GixError {
 	#[error("gix::status::tree_index::Error error: {0}")]
 	StatusTreeIndex(#[from] Box<gix::status::tree_index::Error>),
 
+	///
+	#[error("gix::traverse::commit::topo error: {0}")]
+	Topo(#[from] gix::traverse::commit::topo::Error),
+
 	///
 	#[error("gix::worktree::open_index::Error error: {0}")]
 	WorktreeOpenIndex(#[from] Box<gix::worktree::open_index::Error>),
@@ -333,6 +337,12 @@ impl From<gix::status::tree_index::Error> for Error {
 	}
 }
 
+impl From<gix::traverse::commit::topo::Error> for Error {
+	fn from(error: gix::traverse::commit::topo::Error) -> Self {
+		Self::Gix(GixError::from(error))
+	}
+}
+
 impl From<gix::worktree::open_index::Error> for GixError {
 	fn from(error: gix::worktree::open_index::Error) -> Self {
 		Self::WorktreeOpenIndex(Box::new(error))

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

asyncgit/src/sync/logwalker.rs

@@ -1,66 +1,51 @@
 use super::{CommitId, SharedCommitFilterFn};
 use crate::error::Result;
-use git2::{Commit, Oid, Repository};
-use gix::revision::Walk;
-use std::{
-	cmp::Ordering,
-	collections::{BinaryHeap, HashSet},
-};
+use git2::{Repository, Revwalk, Sort};
+use gix::traverse::commit::topo;
+use gix::traverse::commit::Topo;
 
-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<Ordering> {
-		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<TimeOrderedCommit<'a>>,
-	visited: HashSet<Oid>,
+	/// 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<SharedCommitFilterFn>,
 }
 
 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<Self> {
-		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 +54,14 @@ impl<'a> LogWalker<'a> {
 		Self { filter, ..self }
 	}
 
-	///
+	/// Get a batch of commits
 	pub fn read(&mut self, out: &mut Vec<CommitId>) -> Result<usize> {
 		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 +73,7 @@ impl<'a> LogWalker<'a> {
 				out.push(id);
 			}
 
+			self.visited_count += 1;
 			count += 1;
 			if count == self.limit {
 				break;
@@ -98,13 +82,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
@@ -118,7 +95,7 @@ impl<'a> LogWalker<'a> {
 /// A more long-term option is to refactor filtering to work with a `gix::Repository` and to remove
 /// `LogWalker` once this is done, but this is a larger effort.
 pub struct LogWalkerWithoutFilter<'a> {
-	walk: Walk<'a>,
+	walk: Topo<&'a gix::Repository, fn(&gix::hash::oid) -> bool>,
 	limit: usize,
 	visited: usize,
 }
@@ -137,12 +114,12 @@ impl<'a> LogWalkerWithoutFilter<'a> {
 
 		let tips = [commit.id];
 
-		let platform = repo
-			.rev_walk(tips)
-			.sorting(gix::revision::walk::Sorting::ByCommitTime(gix::traverse::commit::simple::CommitTimeOrder::NewestFirst))
-			.use_commit_graph(false);
-
-		let walk = platform.all()?;
+		let walk = topo::Builder::new(&*repo)
+			// Show no parents before all of its children are shown,
+			// but otherwise show commits in the commit timestamp order.
+			.sorting(topo::Sorting::DateOrder)
+			.with_tips(tips)
+			.build()?;
 
 		Ok(Self {
 			walk,