fix: Use revwalk to determine consecutive range

Author: Esgariot

asyncgit/src/sync/mod.rs

@@ -24,6 +24,7 @@ mod rebase;
 pub mod remotes;
 mod repository;
 mod reset;
+mod revwalk;
 mod reword;
 pub mod sign;
 mod staging;
@@ -65,6 +66,7 @@ pub use config::{
 };
 pub use diff::get_diff_commit;
 pub use git2::BranchType;
+pub use git2::Sort;
 pub use hooks::{
 	hooks_commit_msg, hooks_post_commit, hooks_pre_commit,
 	hooks_prepare_commit_msg, HookResult, PrepareCommitMsgSource,
@@ -87,6 +89,7 @@ pub use remotes::{
 pub(crate) use repository::{gix_repo, repo};
 pub use repository::{RepoPath, RepoPathRef};
 pub use reset::{reset_repo, reset_stage, reset_workdir};
+pub use revwalk::revwalk;
 pub use reword::reword;
 pub use staging::{discard_lines, stage_lines};
 pub use stash::{

asyncgit/src/sync/revwalk.rs

@@ -0,0 +1,58 @@
+use std::ops::Bound;
+
+use crate::Result;
+use git2::{Commit, Oid};
+
+use super::{repo, CommitId, RepoPath};
+
+/// Performs a Git revision walk on `repo_path`, optionally bounded by `start` and `end` commits,
+/// sorted according to `sort`. The revwalk iterator bound by repository's lifetime is exposed through
+/// the `iter_fn`.
+///
+///
+pub fn revwalk<R>(
+	repo_path: &RepoPath,
+	start: Bound<&CommitId>,
+	end: Bound<&CommitId>,
+	sort: git2::Sort,
+	iter_fn: impl FnOnce(
+		&mut (dyn Iterator<Item = Result<Oid>>),
+	) -> Result<R>,
+) -> Result<R> {
+	let repo = repo(repo_path)?;
+	let mut revwalk = repo.revwalk()?;
+	revwalk.set_sorting(sort)?;
+	let start = resolve(&repo, start)?;
+	let end = resolve(&repo, end)?;
+
+	if let Some(s) = start {
+		revwalk.hide(s.id())?;
+	}
+	if let Some(e) = end {
+		revwalk.push(e.id())?;
+	}
+	let ret = iter_fn(&mut revwalk.map(|r| {
+		r.map_err(|x| crate::Error::Generic(x.to_string()))
+	}));
+	ret
+}
+
+fn resolve<'r>(
+	repo: &'r git2::Repository,
+	commit: Bound<&CommitId>,
+) -> Result<Option<Commit<'r>>> {
+	match commit {
+		Bound::Included(c) => {
+			let commit = repo.find_commit(c.get_oid())?;
+			Ok(Some(commit))
+		}
+		Bound::Excluded(s) => {
+			let commit = repo.find_commit(s.get_oid())?;
+			let res = (commit.parent_count() == 1)
+				.then(|| commit.parent(0))
+				.transpose()?;
+			Ok(res)
+		}
+		Bound::Unbounded => Ok(None),
+	}
+}

src/components/commitlist.rs

@@ -14,8 +14,8 @@ use crate::{
 };
 use anyhow::Result;
 use asyncgit::sync::{
-	self, checkout_commit, BranchDetails, BranchInfo, CommitId,
-	RepoPathRef, Tags,
+	self, checkout_commit, revwalk, BranchDetails, BranchInfo,
+	CommitId, RepoPathRef, Sort, Tags,
 };
 use chrono::{DateTime, Local};
 use crossterm::event::Event;
@@ -29,8 +29,8 @@ use ratatui::{
 	Frame,
 };
 use std::{
-	borrow::Cow, cell::Cell, cmp, collections::BTreeMap, rc::Rc,
-	time::Instant,
+	borrow::Cow, cell::Cell, cmp, collections::BTreeMap, ops::Bound,
+	rc::Rc, time::Instant,
 };
 
 const ELEMENTS_PER_LINE: usize = 9;
@@ -131,37 +131,52 @@ impl CommitList {
 	}
 
 	/// Build string of marked or selected (if none are marked) commit ids
-	fn concat_selected_commit_ids(&self) -> Option<String> {
+	fn concat_selected_commit_ids(&self) -> Result<Option<String>> {
 		match self.marked.as_slice() {
-			[] => self
+			[] => Ok(self
 				.items
 				.iter()
 				.nth(
 					self.selection
 						.saturating_sub(self.items.index_offset()),
 				)
-				.map(|e| e.id.to_string()),
-			[latest, .., earliest]
-				if self
-					.marked()
-					.windows(2)
-					.all(|w| w[0].0 + 1 == w[1].0) =>
-			{
-				Some(format!("{}^..{}", earliest.1, latest.1))
+				.map(|e| e.id.to_string())),
+			[(_idx, commit)] => Ok(Some(commit.to_string())),
+			[latest, .., earliest] => {
+				let marked_rev = self.marked.iter().rev();
+				let marked_topo_consecutive = revwalk(
+					&self.repo.borrow(),
+					Bound::Excluded(&earliest.1),
+					Bound::Included(&latest.1),
+					Sort::TOPOLOGICAL | Sort::REVERSE,
+					|revwalk| {
+						revwalk.zip(marked_rev).try_fold(
+							true,
+							|acc, (r, m)| {
+								let revwalked = CommitId::new(r?);
+								let marked = m.1;
+								Ok(acc && (revwalked == marked))
+							},
+						)
+					},
+				)?;
+				let yank = if marked_topo_consecutive {
+					format!("{}^..{}", earliest.1, latest.1)
+				} else {
+					self.marked
+						.iter()
+						.map(|(_idx, commit)| commit.to_string())
+						.join(" ")
+				};
+				Ok(Some(yank))
 			}
-			marked => Some(
-				marked
-					.iter()
-					.map(|(_idx, commit)| commit.to_string())
-					.join(" "),
-			),
 		}
 	}
 
 	/// Copy currently marked or selected (if none are marked) commit ids
 	/// to clipboard
 	pub fn copy_commit_hash(&self) -> Result<()> {
-		if let Some(yank) = self.concat_selected_commit_ids() {
+		if let Some(yank) = self.concat_selected_commit_ids()? {
 			crate::clipboard::copy_string(&yank)?;
 			self.queue.push(InternalEvent::ShowInfoMsg(
 				strings::copy_success(&yank),
@@ -1008,7 +1023,9 @@ mod tests {
 	#[test]
 	fn test_copy_commit_list_empty() {
 		assert_eq!(
-			CommitList::default().concat_selected_commit_ids(),
+			CommitList::default()
+				.concat_selected_commit_ids()
+				.unwrap(),
 			None
 		);
 	}
@@ -1023,7 +1040,7 @@ mod tests {
 		// offset by two, so we expect commit id 2 for
 		// selection = 4
 		assert_eq!(
-			cl.concat_selected_commit_ids(),
+			cl.concat_selected_commit_ids().unwrap(),
 			Some(String::from(
 				"0000000000000000000000000000000000000002"
 			))
@@ -1038,35 +1055,37 @@ mod tests {
 			..cl
 		};
 		assert_eq!(
-			cl.concat_selected_commit_ids(),
+			cl.concat_selected_commit_ids().unwrap(),
 			Some(String::from(
 				"0000000000000000000000000000000000000001",
 			))
 		);
 	}
 
 	#[test]
+	#[ignore = "needs real repository to run test. Will be moved to revwalk module."]
 	fn test_copy_commit_range_marked() {
 		let cl = build_commit_list_with_some_commits();
 		let cl = CommitList {
 			marked: build_marked_from_indices(&cl, &[4, 5, 6, 7]),
 			..cl
 		};
 		assert_eq!(
-			cl.concat_selected_commit_ids(),
+			cl.concat_selected_commit_ids().unwrap(),
 			Some(String::from("0000000000000000000000000000000000000005^..0000000000000000000000000000000000000002"))
 		);
 	}
 
 	#[test]
+	#[ignore = "needs real repository to run test. Will be moved to revwalk module."]
 	fn test_copy_commit_random_marked() {
 		let cl = build_commit_list_with_some_commits();
 		let cl = CommitList {
 			marked: build_marked_from_indices(&cl, &[4, 7]),
 			..cl
 		};
 		assert_eq!(
-			cl.concat_selected_commit_ids(),
+			cl.concat_selected_commit_ids().unwrap(),
 			Some(String::from(concat!(
 				"0000000000000000000000000000000000000002 ",
 				"0000000000000000000000000000000000000005"