fix: Ensure topological continuity when yanking commit range as <oldest>^..<newest>

CHANGELOG.md

@@ -8,6 +8,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## Unreleased
 * execute git-hooks directly if possible (on *nix) else use sh instead of bash (without reading SHELL variable) [[@Joshix](https://github.com/Joshix-1)] ([#2483](https://github.com/extrawurst/gitui/pull/2483))
 
+### Breaking Changes
+
 ### Added
 * Files and status tab support pageUp and pageDown  [[@fatpandac](https://github.com/fatpandac)] ([#1951](https://github.com/extrawurst/gitui/issues/1951))
 * support loading custom syntax highlighting themes from a file [[@acuteenvy](https://github.com/acuteenvy)] ([#2565](https://github.com/gitui-org/gitui/pull/2565))
@@ -31,6 +33,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixes
 * resolve `core.hooksPath` relative to `GIT_WORK_TREE` [[@naseschwarz](https://github.com/naseschwarz)] ([#2571](https://github.com/gitui-org/gitui/issues/2571))
 * yanking commit ranges no longer generates incorrect dotted range notations, but lists each individual commit [[@naseschwarz](https://github.com/naseschwarz)] (https://github.com/gitui-org/gitui/issues/2576)
+* check if selected range is [topologically continuous](https://git-scm.com/docs/git-log#Documentation/git-log.txt---topo-order) before producing `<oldest>^..<newest>` for yanked commit ranges. [[@Esgariot](https://github.com/Esgariot)] ([#2441](https://github.com/extrawurst/gitui/pull/2441))
 
 ## [0.27.0] - 2024-01-14
 

asyncgit/src/sync/mod.rs

@@ -24,6 +24,7 @@ mod rebase;
 pub mod remotes;
 mod repository;
 mod reset;
+pub mod revwalk;
 mod reword;
 pub mod sign;
 mod staging;

asyncgit/src/sync/revwalk.rs

@@ -0,0 +1,146 @@
+//! git revwalk utils
+use super::{repo, CommitId, RepoPath};
+use crate::Result;
+use git2::Oid;
+use std::ops::ControlFlow;
+
+/// Checks if `commits` range is topologically continuous
+///
+/// Supports only linear paths - presence of merge commits results in `false`.
+pub fn is_continuous(
+	repo_path: &RepoPath,
+	commits: &[CommitId],
+) -> Result<bool> {
+	match commits {
+		[] | [_] => Ok(true),
+		commits => {
+			let repo = repo(repo_path)?;
+			let mut revwalk = repo.revwalk()?;
+			revwalk.set_sorting(git2::Sort::TOPOLOGICAL)?;
+			revwalk.push(commits[0].get_oid())?;
+			let revwalked: Vec<Oid> =
+				revwalk
+					.take(commits.len())
+					.collect::<std::result::Result<Vec<_>, _>>()?;
+
+			if revwalked.len() != commits.len() {
+				return Ok(false);
+			}
+
+			match revwalked.iter().zip(commits).try_fold(
+				Ok(true),
+				|acc, (r, c)| match acc
+					.map(|acc| acc && (&(CommitId::from(*r)) == c))
+				{
+					ok @ Ok(true) => ControlFlow::Continue(ok),
+					otherwise => ControlFlow::Break(otherwise),
+				},
+			) {
+				ControlFlow::Continue(v) | ControlFlow::Break(v) => v,
+			}
+		}
+	}
+}
+#[cfg(test)]
+mod tests_is_continuous {
+	use crate::sync::{
+		checkout_commit, commit, merge_commit,
+		revwalk::is_continuous, tests::repo_init_empty, RepoPath,
+	};
+
+	#[test]
+	fn test_linear_commits_are_continuous() {
+		// * c3 (HEAD)
+		// * c2
+		// * c1
+		// * c0 (root)
+
+		let (_td, repo) = repo_init_empty().unwrap();
+		let root = repo.path().parent().unwrap();
+		let repo_path: &RepoPath =
+			&root.as_os_str().to_str().unwrap().into();
+		let _c0 = commit(repo_path, "commit 0").unwrap();
+		let c1 = commit(repo_path, "commit 1").unwrap();
+		let c2 = commit(repo_path, "commit 2").unwrap();
+		let c3 = commit(repo_path, "commit 3").unwrap();
+
+		let result = is_continuous(repo_path, &[c3, c2, c1]).unwrap();
+		assert!(result, "linear commits should be continuous");
+
+		let result = is_continuous(repo_path, &[c2]).unwrap();
+		assert!(result, "single commit should be continuous");
+
+		let result = is_continuous(repo_path, &[]).unwrap();
+		assert!(result, "empty range should be continuous");
+	}
+
+	#[test]
+	fn test_merge_commits_break_continuity() {
+		// *   c4 (HEAD)
+		// |\
+		// | * c3
+		// * | c2
+		// |/
+		// * c1
+		// * c0 (root)
+
+		let (_td, repo) = repo_init_empty().unwrap();
+		let root = repo.path().parent().unwrap();
+		let repo_path: &RepoPath =
+			&root.as_os_str().to_str().unwrap().into();
+
+		let c0 = commit(repo_path, "commit 0").unwrap();
+		let c1 = commit(repo_path, "commit 1").unwrap();
+		let c2 = commit(repo_path, "commit 2").unwrap();
+
+		checkout_commit(repo_path, c1).unwrap();
+		let c3 = commit(repo_path, "commit 3").unwrap();
+
+		let c4 =
+			merge_commit(repo_path, "commit 4", &[c2, c3]).unwrap();
+
+		let result = is_continuous(repo_path, &[c4, c2, c1]).unwrap();
+		assert!(
+			!result,
+			"range including merge should not be continuous"
+		);
+
+		let result = is_continuous(repo_path, &[c4, c3, c1]).unwrap();
+		assert!(
+			!result,
+			"range including merge should not be continuous (following second parent commit)"
+		);
+
+		let result = is_continuous(repo_path, &[c2, c1, c0]).unwrap();
+		assert!(
+			result,
+			"linear range before merge should be continuous"
+		);
+	}
+
+	#[test]
+	fn test_non_continuous_commits() {
+		// * c4 (HEAD)
+		// * c3
+		// * c2
+		// * c1
+		// * c0 (root)
+
+		let (_td, repo) = repo_init_empty().unwrap();
+		let root = repo.path().parent().unwrap();
+		let repo_path: &RepoPath =
+			&root.as_os_str().to_str().unwrap().into();
+
+		let _c0 = commit(repo_path, "commit 0").unwrap();
+		let c1 = commit(repo_path, "commit 1").unwrap();
+		let c2 = commit(repo_path, "commit 2").unwrap();
+		let c3 = commit(repo_path, "commit 3").unwrap();
+		let c4 = commit(repo_path, "commit 4").unwrap();
+
+		let result = is_continuous(repo_path, &[c4, c3, c1]).unwrap();
+		assert!(!result, "commit range with gap should return false");
+
+		let result = is_continuous(repo_path, &[c1, c2, c3]).unwrap();
+		assert!(!result, "wrong order should return false");
+	}
+}

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, Tags,
 };
 use chrono::{DateTime, Local};
 use crossterm::event::Event;
@@ -131,8 +131,8 @@ impl CommitList {
 	}
 
 	/// Build string of marked or selected (if none are marked) commit ids
-	fn concat_selected_commit_ids(&self) -> Option<String> {
-		match self.marked.as_slice() {
+	fn concat_selected_commit_ids(&self) -> Result<Option<String>> {
+		let ret = match self.marked.as_slice() {
 			[] => self
 				.items
 				.iter()
@@ -141,19 +141,28 @@ impl CommitList {
 						.saturating_sub(self.items.index_offset()),
 				)
 				.map(|e| e.id.to_string()),
+			[latest, .., earliest]
+				if revwalk::is_continuous(
+					&self.repo.borrow(),
+					&self.marked.iter().map(|x| x.1).collect_vec(),
+				)? =>
+			{
+				Some(format!("{}^..{}", earliest.1, latest.1))
+			}
 			marked => Some(
 				marked
 					.iter()
 					.map(|(_idx, commit)| commit.to_string())
 					.join(" "),
 			),
-		}
+		};
+		Ok(ret)
 	}
 
 	/// 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),
@@ -1000,7 +1009,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
 		);
 	}
@@ -1015,7 +1026,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"
 			))
@@ -1030,44 +1041,10 @@ mod tests {
 			..cl
 		};
 		assert_eq!(
-			cl.concat_selected_commit_ids(),
+			cl.concat_selected_commit_ids().unwrap(),
 			Some(String::from(
 				"0000000000000000000000000000000000000001",
 			))
 		);
 	}
-
-	#[test]
-	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(),
-			Some(String::from(concat!(
-				"0000000000000000000000000000000000000002 ",
-				"0000000000000000000000000000000000000003 ",
-				"0000000000000000000000000000000000000004 ",
-				"0000000000000000000000000000000000000005"
-			)))
-		);
-	}
-
-	#[test]
-	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(),
-			Some(String::from(concat!(
-				"0000000000000000000000000000000000000002 ",
-				"0000000000000000000000000000000000000005"
-			)))
-		);
-	}
 }