diff --git a/CHANGELOG.md b/CHANGELOG.md index e60180cdc5..7eae542c7a 100644 --- a/CHANGELOG.md +++ b/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 `^..` for yanked commit ranges. [[@Esgariot](https://github.com/Esgariot)] ([#2441](https://github.com/extrawurst/gitui/pull/2441)) ## [0.27.0] - 2024-01-14 diff --git a/asyncgit/src/sync/mod.rs b/asyncgit/src/sync/mod.rs index 09d4e6ef53..18b901bb9d 100644 --- a/asyncgit/src/sync/mod.rs +++ b/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; diff --git a/asyncgit/src/sync/revwalk.rs b/asyncgit/src/sync/revwalk.rs new file mode 100644 index 0000000000..5bd6896eab --- /dev/null +++ b/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 { + 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 = + revwalk + .take(commits.len()) + .collect::, _>>()?; + + 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"); + } +} diff --git a/src/components/commitlist.rs b/src/components/commitlist.rs index 06c48e751b..9ef6a6b0fb 100644 --- a/src/components/commitlist.rs +++ b/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 { - match self.marked.as_slice() { + fn concat_selected_commit_ids(&self) -> Result> { + 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" - ))) - ); - } }