From 761572d5b1e2cb34f4d98050e6e14a09055060c2 Mon Sep 17 00:00:00 2001 From: Esgariot Date: Tue, 3 Dec 2024 01:10:41 +0100 Subject: [PATCH 01/11] fix: Reverse commit range before yanking Produce `^..` when yanking consecutive range. Now, given consecutive marked selection, gitui's selection matches `git log`'s output in commit range. --- src/components/commitlist.rs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/components/commitlist.rs b/src/components/commitlist.rs index 06c48e751b..2240a4c1ea 100644 --- a/src/components/commitlist.rs +++ b/src/components/commitlist.rs @@ -141,6 +141,14 @@ impl CommitList { .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)) + } marked => Some( marked .iter() @@ -1046,12 +1054,7 @@ mod tests { }; assert_eq!( cl.concat_selected_commit_ids(), - Some(String::from(concat!( - "0000000000000000000000000000000000000002 ", - "0000000000000000000000000000000000000003 ", - "0000000000000000000000000000000000000004 ", - "0000000000000000000000000000000000000005" - ))) + Some(String::from("0000000000000000000000000000000000000005^..0000000000000000000000000000000000000002")) ); } From 33540c642716e5f69edfdaad78bf91239b036113 Mon Sep 17 00:00:00 2001 From: Esgariot Date: Tue, 3 Dec 2024 01:40:21 +0100 Subject: [PATCH 02/11] misc: Update changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e60180cdc5..448ebc58ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,9 @@ 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 +* reverse commit range before yanking marked commits, producing `^..` for consecutive commit ranges. [[@Esgariot](https://github.com/Esgariot)] ([#2441](https://github.com/extrawurst/gitui/pull/2441)) + ### 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)) From a4b1f8893ea40c1cd9ffc08a4704ef903a20892d Mon Sep 17 00:00:00 2001 From: Esgariot Date: Sat, 5 Apr 2025 18:12:49 +0200 Subject: [PATCH 03/11] fix: Use revwalk to determine consecutive range --- asyncgit/src/sync/mod.rs | 3 ++ asyncgit/src/sync/revwalk.rs | 58 +++++++++++++++++++++++++++++ src/components/commitlist.rs | 71 +++++++++++++++++++++++------------- 3 files changed, 106 insertions(+), 26 deletions(-) create mode 100644 asyncgit/src/sync/revwalk.rs diff --git a/asyncgit/src/sync/mod.rs b/asyncgit/src/sync/mod.rs index 09d4e6ef53..9310323da4 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; +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::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::{ diff --git a/asyncgit/src/sync/revwalk.rs b/asyncgit/src/sync/revwalk.rs new file mode 100644 index 0000000000..cfa0c4da68 --- /dev/null +++ b/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( + repo_path: &RepoPath, + start: Bound<&CommitId>, + end: Bound<&CommitId>, + sort: git2::Sort, + iter_fn: impl FnOnce( + &mut (dyn Iterator>), + ) -> Result, +) -> Result { + 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>> { + 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), + } +} diff --git a/src/components/commitlist.rs b/src/components/commitlist.rs index 2240a4c1ea..57699ea457 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, 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 { + fn concat_selected_commit_ids(&self) -> Result> { 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,7 +1055,7 @@ mod tests { ..cl }; assert_eq!( - cl.concat_selected_commit_ids(), + cl.concat_selected_commit_ids().unwrap(), Some(String::from( "0000000000000000000000000000000000000001", )) @@ -1046,6 +1063,7 @@ mod tests { } #[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 { @@ -1053,12 +1071,13 @@ mod tests { ..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 { @@ -1066,7 +1085,7 @@ mod tests { ..cl }; assert_eq!( - cl.concat_selected_commit_ids(), + cl.concat_selected_commit_ids().unwrap(), Some(String::from(concat!( "0000000000000000000000000000000000000002 ", "0000000000000000000000000000000000000005" From ce20664b361d7d3f1e81c1c4e8376201e53fae18 Mon Sep 17 00:00:00 2001 From: Esgariot Date: Sat, 5 Apr 2025 18:13:00 +0200 Subject: [PATCH 04/11] misc: Formatting --- asyncgit/src/sync/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/asyncgit/src/sync/mod.rs b/asyncgit/src/sync/mod.rs index 9310323da4..9129be22b4 100644 --- a/asyncgit/src/sync/mod.rs +++ b/asyncgit/src/sync/mod.rs @@ -66,6 +66,7 @@ pub use config::{ }; pub use diff::get_diff_commit; pub use git2::BranchType; +pub use git2::ResetType; pub use git2::Sort; pub use hooks::{ hooks_commit_msg, hooks_post_commit, hooks_pre_commit, @@ -111,8 +112,6 @@ pub use utils::{ stage_add_all, stage_add_file, stage_addremoved, Head, }; -pub use git2::ResetType; - /// test utils #[cfg(test)] pub mod tests { From 9c68fe94582d0472e9e23e18c3ed46f036f5c3aa Mon Sep 17 00:00:00 2001 From: Esgariot Date: Mon, 5 May 2025 23:37:58 +0200 Subject: [PATCH 05/11] fix: Resolve clippy diagnostics --- asyncgit/src/sync/revwalk.rs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/asyncgit/src/sync/revwalk.rs b/asyncgit/src/sync/revwalk.rs index cfa0c4da68..5cf4246412 100644 --- a/asyncgit/src/sync/revwalk.rs +++ b/asyncgit/src/sync/revwalk.rs @@ -5,11 +5,10 @@ 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`. -/// +/// Performs a Git revision walk. /// +/// The revwalk is 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( repo_path: &RepoPath, start: Bound<&CommitId>, @@ -31,10 +30,13 @@ pub fn revwalk( 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 + { + #![allow(clippy::let_and_return)] + let ret = iter_fn(&mut revwalk.map(|r| { + r.map_err(|x| crate::Error::Generic(x.to_string())) + })); + ret + } } fn resolve<'r>( From 011fdec2c803ea1f2d60c709dc0dcf62e1539fe2 Mon Sep 17 00:00:00 2001 From: Esgariot Date: Sat, 10 May 2025 00:22:20 +0200 Subject: [PATCH 06/11] fix: Don't use Result for early return in try_fold Move is_topo_consecutive into revwalk::is_continuous. Drop single selection match case, it's handled by catch-all. --- asyncgit/src/sync/mod.rs | 3 +- asyncgit/src/sync/revwalk.rs | 32 +++++++++++++++++++++- src/components/commitlist.rs | 53 ++++++++++++++---------------------- 3 files changed, 52 insertions(+), 36 deletions(-) diff --git a/asyncgit/src/sync/mod.rs b/asyncgit/src/sync/mod.rs index 9129be22b4..2eb0818270 100644 --- a/asyncgit/src/sync/mod.rs +++ b/asyncgit/src/sync/mod.rs @@ -24,7 +24,7 @@ mod rebase; pub mod remotes; mod repository; mod reset; -mod revwalk; +pub mod revwalk; mod reword; pub mod sign; mod staging; @@ -90,7 +90,6 @@ pub use remotes::{ pub(crate) use repository::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::{ diff --git a/asyncgit/src/sync/revwalk.rs b/asyncgit/src/sync/revwalk.rs index 5cf4246412..5e6007dbdd 100644 --- a/asyncgit/src/sync/revwalk.rs +++ b/asyncgit/src/sync/revwalk.rs @@ -1,4 +1,5 @@ -use std::ops::Bound; +//! git revwalk utils +use std::ops::{Bound, ControlFlow}; use crate::Result; use git2::{Commit, Oid}; @@ -58,3 +59,32 @@ fn resolve<'r>( Bound::Unbounded => Ok(None), } } + +/// Checks if `commits` range is continuous under `sort` flags. +pub fn is_continuous( + repo_path: &RepoPath, + sort: git2::Sort, + commits: &[CommitId], +) -> Result { + match commits { + [] | [_] => Ok(true), + [start, .., end] => revwalk( + repo_path, + Bound::Excluded(start), + Bound::Included(end), + sort, + |revwalk| match revwalk.zip(commits).try_fold( + Ok(true), + |acc, (r, c)| match r + .map(CommitId::new) + .and_then(|r| acc.map(|acc| acc && (&r == c))) + { + Ok(true) => ControlFlow::Continue(Ok(true)), + otherwise => ControlFlow::Break(otherwise), + }, + ) { + ControlFlow::Continue(v) | ControlFlow::Break(v) => v, + }, + ), + } +} diff --git a/src/components/commitlist.rs b/src/components/commitlist.rs index 57699ea457..5b173c97c8 100644 --- a/src/components/commitlist.rs +++ b/src/components/commitlist.rs @@ -29,8 +29,8 @@ use ratatui::{ Frame, }; use std::{ - borrow::Cow, cell::Cell, cmp, collections::BTreeMap, ops::Bound, - rc::Rc, time::Instant, + borrow::Cow, cell::Cell, cmp, collections::BTreeMap, rc::Rc, + time::Instant, }; const ELEMENTS_PER_LINE: usize = 9; @@ -132,45 +132,32 @@ impl CommitList { /// Build string of marked or selected (if none are marked) commit ids fn concat_selected_commit_ids(&self) -> Result> { - match self.marked.as_slice() { - [] => Ok(self + let ret = match self.marked.as_slice() { + [] => self .items .iter() .nth( self.selection .saturating_sub(self.items.index_offset()), ) - .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( + .map(|e| e.id.to_string()), + [latest, .., earliest] + if revwalk::is_continuous( &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)) + Sort::TOPOLOGICAL, + &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 From 4523f2a1778b1abc28b4a1e51246400592a165b2 Mon Sep 17 00:00:00 2001 From: Esgariot Date: Fri, 20 Jun 2025 20:57:19 +0200 Subject: [PATCH 07/11] feat: inline `revwalk` into `is_continuous` `revwalk` turned out too complicated, include simplified version into `is_continuous`. Add tests. --- asyncgit/src/sync/revwalk.rs | 195 ++++++++++++++++++++++------------- src/components/commitlist.rs | 3 +- 2 files changed, 124 insertions(+), 74 deletions(-) diff --git a/asyncgit/src/sync/revwalk.rs b/asyncgit/src/sync/revwalk.rs index 5e6007dbdd..e4bc44f34d 100644 --- a/asyncgit/src/sync/revwalk.rs +++ b/asyncgit/src/sync/revwalk.rs @@ -1,90 +1,141 @@ //! git revwalk utils -use std::ops::{Bound, ControlFlow}; - -use crate::Result; -use git2::{Commit, Oid}; - use super::{repo, CommitId, RepoPath}; +use crate::Result; +use git2::Oid; +use std::ops::ControlFlow; -/// Performs a Git revision walk. +/// Checks if `commits` range is topologically continuous /// -/// The revwalk is 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( - repo_path: &RepoPath, - start: Bound<&CommitId>, - end: Bound<&CommitId>, - sort: git2::Sort, - iter_fn: impl FnOnce( - &mut (dyn Iterator>), - ) -> Result, -) -> Result { - 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())?; - } - { - #![allow(clippy::let_and_return)] - 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>> { - 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), - } -} - -/// Checks if `commits` range is continuous under `sort` flags. +/// Supports only linear paths - presence of merge commits results in `false`. pub fn is_continuous( repo_path: &RepoPath, - sort: git2::Sort, commits: &[CommitId], ) -> Result { match commits { [] | [_] => Ok(true), - [start, .., end] => revwalk( - repo_path, - Bound::Excluded(start), - Bound::Included(end), - sort, - |revwalk| match revwalk.zip(commits).try_fold( + commits => { + let repo = repo(repo_path)?; + let mut revwalk = repo.revwalk()?; + revwalk.set_sorting(git2::Sort::TOPOLOGICAL)?; + revwalk.simplify_first_parent()?; + 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 r - .map(CommitId::new) - .and_then(|r| acc.map(|acc| acc && (&r == c))) + |acc, (r, c)| match acc + .map(|acc| acc && (&(CommitId::from(*r)) == c)) { - Ok(true) => ControlFlow::Continue(Ok(true)), + 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, &[c2, c1]).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 5b173c97c8..e5f529da63 100644 --- a/src/components/commitlist.rs +++ b/src/components/commitlist.rs @@ -15,7 +15,7 @@ use crate::{ use anyhow::Result; use asyncgit::sync::{ self, checkout_commit, revwalk, BranchDetails, BranchInfo, - CommitId, RepoPathRef, Sort, Tags, + CommitId, RepoPathRef, Tags, }; use chrono::{DateTime, Local}; use crossterm::event::Event; @@ -144,7 +144,6 @@ impl CommitList { [latest, .., earliest] if revwalk::is_continuous( &self.repo.borrow(), - Sort::TOPOLOGICAL, &self.marked.iter().map(|x| x.1).collect_vec(), )? => { From 888360101de099d88150106b06a759288d3227e5 Mon Sep 17 00:00:00 2001 From: Esgariot Date: Fri, 20 Jun 2025 20:58:49 +0200 Subject: [PATCH 08/11] test: Remove test cases handled by `revwalk` ones --- src/components/commitlist.rs | 31 ------------------------------- 1 file changed, 31 deletions(-) diff --git a/src/components/commitlist.rs b/src/components/commitlist.rs index e5f529da63..9ef6a6b0fb 100644 --- a/src/components/commitlist.rs +++ b/src/components/commitlist.rs @@ -1047,35 +1047,4 @@ mod tests { )) ); } - - #[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().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().unwrap(), - Some(String::from(concat!( - "0000000000000000000000000000000000000002 ", - "0000000000000000000000000000000000000005" - ))) - ); - } } From d6a488326258cd60243904f85611ae71c3a1af89 Mon Sep 17 00:00:00 2001 From: Esgariot Date: Fri, 20 Jun 2025 21:15:39 +0200 Subject: [PATCH 09/11] misc: Revert changes that were not required --- asyncgit/src/sync/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/asyncgit/src/sync/mod.rs b/asyncgit/src/sync/mod.rs index 2eb0818270..18b901bb9d 100644 --- a/asyncgit/src/sync/mod.rs +++ b/asyncgit/src/sync/mod.rs @@ -66,8 +66,6 @@ pub use config::{ }; pub use diff::get_diff_commit; pub use git2::BranchType; -pub use git2::ResetType; -pub use git2::Sort; pub use hooks::{ hooks_commit_msg, hooks_post_commit, hooks_pre_commit, hooks_prepare_commit_msg, HookResult, PrepareCommitMsgSource, @@ -111,6 +109,8 @@ pub use utils::{ stage_add_all, stage_add_file, stage_addremoved, Head, }; +pub use git2::ResetType; + /// test utils #[cfg(test)] pub mod tests { From 1d3e8e5b6284c55b28d5b96bbd1d145265cc01c2 Mon Sep 17 00:00:00 2001 From: Esgariot Date: Fri, 20 Jun 2025 21:20:30 +0200 Subject: [PATCH 10/11] fix: Reword and move changelog entry --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 448ebc58ed..7eae542c7a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * 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 -* reverse commit range before yanking marked commits, producing `^..` for consecutive commit ranges. [[@Esgariot](https://github.com/Esgariot)] ([#2441](https://github.com/extrawurst/gitui/pull/2441)) ### Added * Files and status tab support pageUp and pageDown [[@fatpandac](https://github.com/fatpandac)] ([#1951](https://github.com/extrawurst/gitui/issues/1951)) @@ -34,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 From cd31aeed673350accc5ce80b0f8bf3ebd606414e Mon Sep 17 00:00:00 2001 From: Esgariot Date: Fri, 20 Jun 2025 21:33:50 +0200 Subject: [PATCH 11/11] fix: Do not simplify first parent. Add test case --- asyncgit/src/sync/revwalk.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/asyncgit/src/sync/revwalk.rs b/asyncgit/src/sync/revwalk.rs index e4bc44f34d..5bd6896eab 100644 --- a/asyncgit/src/sync/revwalk.rs +++ b/asyncgit/src/sync/revwalk.rs @@ -17,7 +17,6 @@ pub fn is_continuous( let repo = repo(repo_path)?; let mut revwalk = repo.revwalk()?; revwalk.set_sorting(git2::Sort::TOPOLOGICAL)?; - revwalk.simplify_first_parent()?; revwalk.push(commits[0].get_oid())?; let revwalked: Vec = revwalk @@ -90,7 +89,7 @@ mod tests_is_continuous { let repo_path: &RepoPath = &root.as_os_str().to_str().unwrap().into(); - let _c0 = commit(repo_path, "commit 0").unwrap(); + let c0 = commit(repo_path, "commit 0").unwrap(); let c1 = commit(repo_path, "commit 1").unwrap(); let c2 = commit(repo_path, "commit 2").unwrap(); @@ -106,7 +105,13 @@ mod tests_is_continuous { "range including merge should not be continuous" ); - let result = is_continuous(repo_path, &[c2, c1]).unwrap(); + 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"