diff --git a/git-object/src/commit/ref_iter.rs b/git-object/src/commit/ref_iter.rs index 29ee69475ba..145d1a0e2b2 100644 --- a/git-object/src/commit/ref_iter.rs +++ b/git-object/src/commit/ref_iter.rs @@ -65,6 +65,15 @@ impl<'a> CommitRefIter<'a> { _ => None, }) } + + /// Returns the committer signature if there is no decoding error. + /// Errors are coerced into options, hiding whether there was an error or not. The caller knows if there was an error or not. + pub fn committer(&mut self) -> Option> { + self.find_map(|t| match t { + Ok(Token::Committer { signature }) => Some(signature), + _ => None, + }) + } } impl<'a> CommitRefIter<'a> { diff --git a/git-traverse/src/commit.rs b/git-traverse/src/commit.rs index 00ef52ad0bd..ce2b604b812 100644 --- a/git-traverse/src/commit.rs +++ b/git-traverse/src/commit.rs @@ -4,9 +4,11 @@ pub struct Ancestors { predicate: Predicate, state: StateMut, mode: Parents, + sorting: Sorting, } /// Specify how to handle commit parents during traversal. +#[derive(Copy, Clone)] pub enum Parents { /// Traverse all parents, useful for traversing the entire ancestry. All, @@ -20,6 +22,22 @@ impl Default for Parents { } } +/// Specify how to sort commits during traversal. +#[derive(Copy, Clone)] +pub enum Sorting { + /// Default order, sort commit looking up the first reachable parent + GraphOrder, + /// Order commit looking up the most recent parent, since only parents are looked up + /// this ordering is partial + ByCommitterDate, +} + +impl Default for Sorting { + fn default() -> Self { + Sorting::GraphOrder + } +} + /// pub mod ancestors { use std::{ @@ -31,7 +49,7 @@ pub mod ancestors { use git_object::CommitRefIter; use quick_error::quick_error; - use crate::commit::{Ancestors, Parents}; + use crate::commit::{Ancestors, Parents, Sorting}; quick_error! { /// The error is part of the item returned by the [Ancestors] iterator. @@ -55,6 +73,8 @@ pub mod ancestors { next: VecDeque, buf: Vec, seen: BTreeSet, + parents_with_date: Vec<(ObjectId, u32)>, + parents_buf: Vec, } impl State { @@ -71,6 +91,12 @@ pub mod ancestors { self.mode = mode; self } + + /// Set the sorting method, either topological or by author date + pub fn sorting(mut self, sorting: Sorting) -> Self { + self.sorting = sorting; + self + } } impl Ancestors bool, StateMut> @@ -138,6 +164,7 @@ pub mod ancestors { predicate, state, mode: Default::default(), + sorting: Default::default(), } } } @@ -151,6 +178,80 @@ pub mod ancestors { type Item = Result; fn next(&mut self) -> Option { + match self.sorting { + Sorting::GraphOrder => self.graph_sort_next(), + Sorting::ByCommitterDate => self.next_by_commit_date(), + } + } + } + + impl Ancestors + where + Find: for<'a> FnMut(&oid, &'a mut Vec) -> Option>, + Predicate: FnMut(&oid) -> bool, + StateMut: BorrowMut, + { + fn next_by_commit_date(&mut self) -> Option> { + let state = self.state.borrow_mut(); + state.parents_with_date.clear(); + state.parents_buf.clear(); + let res = state.next.pop_front(); + + if let Some(oid) = res { + match (self.find)(&oid, &mut state.buf) { + Some(mut commit_iter) => { + if let Some(Err(decode_tree_err)) = commit_iter.next() { + return Some(Err(decode_tree_err.into())); + } + + for token in commit_iter { + match token { + Ok(git_object::commit::ref_iter::Token::Parent { id }) => { + let parent = (self.find)(id.as_ref(), &mut state.parents_buf); + + let parent_committer_date = parent + .and_then(|mut parent| parent.committer().map(|committer| committer.time)); + + if let Some(parent_committer_date) = parent_committer_date { + state.parents_with_date.push((id, parent_committer_date.time)); + } + + if matches!(self.mode, Parents::First) { + break; + } + } + Ok(_unused_token) => break, + Err(err) => return Some(Err(err.into())), + } + } + } + None => return Some(Err(Error::NotFound { oid })), + } + } + + state + .parents_with_date + .sort_by(|(_, time), (_, other_time)| time.cmp(&other_time).reverse()); + for parent in &state.parents_with_date { + let id = parent.0; + let was_inserted = state.seen.insert(id); + + if was_inserted && (self.predicate)(&id) { + state.next.push_back(id); + } + } + + res.map(Ok) + } + } + + impl Ancestors + where + Find: for<'a> FnMut(&oid, &'a mut Vec) -> Option>, + Predicate: FnMut(&oid) -> bool, + StateMut: BorrowMut, + { + fn graph_sort_next(&mut self) -> Option> { let state = self.state.borrow_mut(); let res = state.next.pop_front(); if let Some(oid) = res { diff --git a/git-traverse/tests/commit/mod.rs b/git-traverse/tests/commit/mod.rs index fb1a86808d9..5e130ab977e 100644 --- a/git-traverse/tests/commit/mod.rs +++ b/git-traverse/tests/commit/mod.rs @@ -1,85 +1,86 @@ mod ancestor { + use crate::hex_to_id; use git_hash::{oid, ObjectId}; use git_odb::{linked::Store, pack::FindExt}; use git_traverse::commit; - use crate::hex_to_id; - - fn db() -> crate::Result { - let dir = git_testtools::scripted_fixture_repo_read_only("make_traversal_repo_for_commits.sh")?; - let db = Store::at(dir.join(".git").join("objects"))?; - Ok(db) + struct TraversalAssertion<'a> { + init_script: &'a str, + tips: &'a [&'a str], + expected: &'a [&'a str], + mode: commit::Parents, + sorting: commit::Sorting, } - fn filtered_iter( - tips: impl IntoIterator>, - predicate: impl FnMut(&oid) -> bool, - ) -> impl Iterator> { - let db = db().expect("db instantiation works as its definitely valid"); - commit::Ancestors::filtered( - tips, - commit::ancestors::State::default(), - move |oid, buf| db.find_commit_iter(oid, buf).ok().map(|t| t.0), - predicate, - ) - } + impl<'a> TraversalAssertion<'a> { + fn new(init_script: &'a str, tips: &'a [&'a str], expected: &'a [&'a str]) -> Self { + TraversalAssertion { + init_script, + tips, + expected, + mode: Default::default(), + sorting: Default::default(), + } + } - fn check_filtered_traversal_with_shared_reference( - tips: &[&str], - expected: &[&str], - predicate: impl FnMut(&oid) -> bool, - ) -> crate::Result { - let tips: Vec<_> = tips.iter().copied().map(hex_to_id).collect(); - let oids: Result, _> = filtered_iter(tips.iter().cloned(), predicate).collect(); - let expected: Vec<_> = tips - .into_iter() - .chain(expected.iter().map(|hex_id| hex_to_id(hex_id))) - .collect(); - assert_eq!(oids?, expected); - Ok(()) - } + fn with_mode(&mut self, mode: commit::Parents) -> &mut Self { + self.mode = mode; + self + } - fn new_iter( - tips: impl IntoIterator>, - mode: commit::Parents, - ) -> impl Iterator> { - let db = db().expect("db instantiation works as its definitely valid"); - commit::Ancestors::new(tips, commit::ancestors::State::default(), move |oid, buf| { - db.find_commit_iter(oid, buf).ok().map(|t| t.0) - }) - .mode(mode) + fn with_sorting(&mut self, sorting: commit::Sorting) -> &mut Self { + self.sorting = sorting; + self + } } - fn check_traversal(tips: &[&str], expected: &[&str]) -> crate::Result { - check_traversal_with_mode(tips, expected, Default::default()) - } + impl TraversalAssertion<'_> { + fn setup(&self) -> crate::Result<(Store, Vec, Vec)> { + let dir = git_testtools::scripted_fixture_repo_read_only(self.init_script)?; + let store = Store::at(dir.join(".git").join("objects"))?; + let tips: Vec<_> = self.tips.iter().copied().map(hex_to_id).collect(); + let expected: Vec = tips + .clone() + .into_iter() + .chain(self.expected.iter().map(|hex_id| hex_to_id(hex_id))) + .collect(); + Ok((store, tips, expected)) + } + fn check_with_predicate(&mut self, predicate: impl FnMut(&oid) -> bool) -> crate::Result<()> { + let (store, tips, expected) = self.setup()?; - fn check_traversal_with_mode(tips: &[&str], expected: &[&str], mode: commit::Parents) -> crate::Result { - let tips: Vec<_> = tips.iter().copied().map(hex_to_id).collect(); - let oids: Result, _> = new_iter(tips.iter().cloned(), mode).collect(); - let expected: Vec<_> = tips - .into_iter() - .chain(expected.iter().map(|hex_id| hex_to_id(hex_id))) + let oids: Result, _> = commit::Ancestors::filtered( + tips, + commit::ancestors::State::default(), + move |oid, buf| store.find_commit_iter(oid, buf).ok().map(|t| t.0), + predicate, + ) + .sorting(self.sorting) + .mode(self.mode) .collect(); - assert_eq!(oids?, expected); - Ok(()) - } - #[test] - fn instantiate_with_arc() -> crate::Result { - let _ = new_iter(vec![git_hash::ObjectId::null_sha1()], Default::default()); - Ok(()) - } + assert_eq!(oids?, expected); + Ok(()) + } - #[test] - fn instantiate_with_box() -> crate::Result { - let _ = new_iter(vec![git_hash::ObjectId::null_sha1()], Default::default()); - Ok(()) + fn check(&self) -> crate::Result { + let (store, tips, expected) = self.setup()?; + let oids: Result, _> = + commit::Ancestors::new(tips, commit::ancestors::State::default(), move |oid, buf| { + store.find_commit_iter(oid, buf).ok().map(|t| t.0) + }) + .sorting(self.sorting) + .mode(self.mode) + .collect(); + assert_eq!(oids?, expected); + Ok(()) + } } #[test] fn linear_history_no_branch() -> crate::Result { - check_traversal( + TraversalAssertion::new( + "make_traversal_repo_for_commits.sh", &["9556057aee5abb06912922e9f26c46386a816822"], &[ "17d78c64cef6c33a10a604573fd2c429e477fd63", @@ -87,11 +88,13 @@ mod ancestor { "134385f6d781b7e97062102c6a483440bfda2a03", ], ) + .check() } #[test] fn simple_branch_with_merge() -> crate::Result { - check_traversal( + TraversalAssertion::new( + "make_traversal_repo_for_commits.sh", &["01ec18a3ebf2855708ad3c9d244306bc1fae3e9b"], &[ "efd9a841189668f1bab5b8ebade9cd0a1b139a37", @@ -103,11 +106,13 @@ mod ancestor { "134385f6d781b7e97062102c6a483440bfda2a03", ], ) + .check() } #[test] fn simple_branch_first_parent_only() -> crate::Result { - check_traversal_with_mode( + TraversalAssertion::new( + "make_traversal_repo_for_commits.sh", &["01ec18a3ebf2855708ad3c9d244306bc1fae3e9b"], &[ "efd9a841189668f1bab5b8ebade9cd0a1b139a37", @@ -116,13 +121,15 @@ mod ancestor { "9902e3c3e8f0c569b4ab295ddf473e6de763e1e7", "134385f6d781b7e97062102c6a483440bfda2a03", ], - commit::Parents::First, ) + .with_mode(commit::Parents::First) + .check() } #[test] fn multiple_tips() -> crate::Result { - check_traversal( + TraversalAssertion::new( + "make_traversal_repo_for_commits.sh", &[ "01ec18a3ebf2855708ad3c9d244306bc1fae3e9b", "9556057aee5abb06912922e9f26c46386a816822", @@ -136,6 +143,7 @@ mod ancestor { "134385f6d781b7e97062102c6a483440bfda2a03", ], ) + .check() } #[test] @@ -144,7 +152,8 @@ mod ancestor { // at least one of its ancestors, so this test is kind of dubious. But we do want // `Ancestors` to not eagerly blacklist all of a commit's ancestors when blacklisting that // one commit, and this test happens to check that. - check_filtered_traversal_with_shared_reference( + TraversalAssertion::new( + "make_traversal_repo_for_commits.sh", &["01ec18a3ebf2855708ad3c9d244306bc1fae3e9b"], &[ "efd9a841189668f1bab5b8ebade9cd0a1b139a37", @@ -154,8 +163,8 @@ mod ancestor { "9902e3c3e8f0c569b4ab295ddf473e6de763e1e7", "134385f6d781b7e97062102c6a483440bfda2a03", ], - |id| id != hex_to_id("9152eeee2328073cf23dcf8e90c949170b711659"), ) + .check_with_predicate(|id| id != hex_to_id("9152eeee2328073cf23dcf8e90c949170b711659")) } #[test] @@ -163,22 +172,52 @@ mod ancestor { // The `self.seen` check should come before the `self.predicate` check, as we don't know how // expensive calling `self.predicate` may be. let mut seen = false; - check_filtered_traversal_with_shared_reference( + TraversalAssertion::new( + "make_traversal_repo_for_commits.sh", &["01ec18a3ebf2855708ad3c9d244306bc1fae3e9b"], &[ "efd9a841189668f1bab5b8ebade9cd0a1b139a37", "ce2e8ffaa9608a26f7b21afc1db89cadb54fd353", "9152eeee2328073cf23dcf8e90c949170b711659", ], - move |id| { - if id == hex_to_id("9556057aee5abb06912922e9f26c46386a816822") { - assert!(!seen); - seen = true; - false - } else { - true - } - }, ) + .check_with_predicate(move |id| { + if id == hex_to_id("9556057aee5abb06912922e9f26c46386a816822") { + assert!(!seen); + seen = true; + false + } else { + true + } + }) + } + + #[test] + fn graph_sorted_commits() -> crate::Result { + TraversalAssertion::new( + "make_traversal_repo_for_commits_with_dates.sh", + &["288e509293165cb5630d08f4185bdf2445bf6170"], + &[ + "9902e3c3e8f0c569b4ab295ddf473e6de763e1e7", + "bcb05040a6925f2ff5e10d3ae1f9264f2e8c43ac", + "134385f6d781b7e97062102c6a483440bfda2a03", + ], + ) + .check() + } + + #[test] + fn committer_date_sorted_commits() -> crate::Result { + TraversalAssertion::new( + "make_traversal_repo_for_commits_with_dates.sh", + &["288e509293165cb5630d08f4185bdf2445bf6170"], + &[ + "bcb05040a6925f2ff5e10d3ae1f9264f2e8c43ac", + "9902e3c3e8f0c569b4ab295ddf473e6de763e1e7", + "134385f6d781b7e97062102c6a483440bfda2a03", + ], + ) + .with_sorting(commit::Sorting::ByCommitterDate) + .check() } } diff --git a/git-traverse/tests/fixtures/make_traversal_repo_for_commits_with_dates.sh b/git-traverse/tests/fixtures/make_traversal_repo_for_commits_with_dates.sh new file mode 100755 index 00000000000..7df53a04439 --- /dev/null +++ b/git-traverse/tests/fixtures/make_traversal_repo_for_commits_with_dates.sh @@ -0,0 +1,21 @@ +#!/bin/bash +set -eu -o pipefail + +git init -q +git config commit.gpgsign false +git config merge.ff false + +# Commit in year 2000 +git checkout -q -b main +GIT_COMMITTER_DATE="2000-01-02 00:00:00 +0000" git commit -q --allow-empty -m c1 #134385f6d781b7e97062102c6a483440bfda2a03- + +# Commit in year 2001 +git checkout -q -b branch1 +GIT_COMMITTER_DATE="2001-01-02 00:00:00 +0000" git commit -q --allow-empty -m b1c1 #bcb05040a6925f2ff5e10d3ae1f9264f2e8c43ac- + +# Commit in year 2000 +git checkout -q main +GIT_COMMITTER_DATE="2000-01-02 00:00:00 +0000" git commit -q --allow-empty -m c2 #9902e3c3e8f0c569b4ab295ddf473e6de763e1e7- + +# Commit from branch1 made in 2001 merged in 2002 +GIT_COMMITTER_DATE="2002-01-02 00:00:00 +0000" git merge branch1 -m m1b1 #288e509293165cb5630d08f4185bdf2445bf6170-