Merge pull request #1564 from Byron/improvements

improvements

gitoxide-core/src/repository/merge_base.rs

@@ -20,7 +20,8 @@ pub fn merge_base(
  .collect::<Result<_, _>>()?;
 
  let cache = repo.commit_graph_if_enabled()?;
- let bases = repo.merge_bases_many_with_cache(first_id, &other_ids, cache.as_ref())?;
+ let mut graph = repo.revision_graph(cache.as_ref());
+ let bases = repo.merge_bases_many_with_graph(first_id, &other_ids, &mut graph)?;
  if bases.is_empty() {
  bail!("No base found for {first} and {others}", others = others.join(", "))
  }

gix-negotiate/src/consecutive.rs

@@ -23,7 +23,7 @@ impl Algorithm {
  let mut is_common = false;
  let mut has_mark = false;
  if let Some(commit) = graph
- .try_lookup_or_insert_commit(id, |data| {
+ .get_or_insert_commit(id, |data| {
  has_mark = data.flags.intersects(mark);
  data.flags |= mark;
  is_common = data.flags.contains(Flags::COMMON);
@@ -47,7 +47,7 @@ impl Algorithm {
  ) -> Result<(), Error> {
  let mut is_common = false;
  if let Some(commit) = graph
- .try_lookup_or_insert_commit(id, |data| is_common = data.flags.contains(Flags::COMMON))?
+ .get_or_insert_commit(id, |data| is_common = data.flags.contains(Flags::COMMON))?
  .filter(|_| !is_common)
  {
  let mut queue = gix_revwalk::PriorityQueue::from_iter(Some((commit.commit_time, (id, 0_usize))));
@@ -64,11 +64,11 @@ impl Algorithm {
  {
  self.add_to_queue(id, Flags::SEEN, graph)?;
  } else if matches!(ancestors, Ancestors::AllUnseen) || generation < 2 {
- if let Some(commit) = graph.try_lookup_or_insert_commit(id, |_| {})? {
+ if let Some(commit) = graph.get_or_insert_commit(id, |_| {})? {
  for parent_id in commit.parents.clone() {
  let mut prev_flags = Flags::default();
  if let Some(parent) = graph
- .try_lookup_or_insert_commit(parent_id, |data| {
+ .get_or_insert_commit(parent_id, |data| {
  prev_flags = data.flags;
  data.flags |= Flags::COMMON;
  })?

gix-negotiate/src/lib.rs

@@ -144,4 +144,4 @@ pub trait Negotiator {
 }
 
 /// An error that happened during any of the methods on a [`Negotiator`].
-pub type Error = gix_revwalk::graph::try_lookup_or_insert_default::Error;
+pub type Error = gix_revwalk::graph::get_or_insert_default::Error;

gix-negotiate/src/skipping.rs

@@ -20,7 +20,7 @@ impl Default for Algorithm {
 impl Algorithm {
  /// Add `id` to our priority queue and *add* `flags` to it.
  fn add_to_queue(&mut self, id: ObjectId, mark: Flags, graph: &mut crate::Graph<'_, '_>) -> Result<(), Error> {
- let commit = graph.try_lookup_or_insert_commit(id, |entry| {
+ let commit = graph.get_or_insert_commit(id, |entry| {
  entry.flags |= mark | Flags::SEEN;
  })?;
  if let Some(timestamp) = commit.map(|c| c.commit_time) {
@@ -35,15 +35,15 @@ impl Algorithm {
  fn mark_common(&mut self, id: ObjectId, graph: &mut crate::Graph<'_, '_>) -> Result<(), Error> {
  let mut is_common = false;
  if let Some(commit) = graph
- .try_lookup_or_insert_commit(id, |entry| {
+ .get_or_insert_commit(id, |entry| {
  is_common = entry.flags.contains(Flags::COMMON);
  entry.flags |= Flags::COMMON;
  })?
  .filter(|_| !is_common)
  {
  let mut queue = gix_revwalk::PriorityQueue::from_iter(Some((commit.commit_time, id)));
  while let Some(id) = queue.pop_value() {
- if let Some(commit) = graph.try_lookup_or_insert_commit(id, |entry| {
+ if let Some(commit) = graph.get_or_insert_commit(id, |entry| {
  if !entry.flags.contains(Flags::POPPED) {
  self.non_common_revs -= 1;
  }
@@ -56,7 +56,7 @@ impl Algorithm {
  }
  let mut was_unseen_or_common = false;
  if let Some(parent) = graph
- .try_lookup_or_insert_commit(parent_id, |entry| {
+ .get_or_insert_commit(parent_id, |entry| {
  was_unseen_or_common =
  !entry.flags.contains(Flags::SEEN) || entry.flags.contains(Flags::COMMON);
  entry.flags |= Flags::COMMON;

gix-revision/src/merge_base.rs

@@ -18,19 +18,15 @@ bitflags::bitflags! {
 #[derive(Debug, thiserror::Error)]
 #[allow(missing_docs)]
 pub enum Error {
- #[error(transparent)]
- IterParents(#[from] gix_revwalk::graph::commit::iter_parents::Error),
- #[error("A commit could not be found")]
- FindExistingCommit(#[from] gix_object::find::existing_iter::Error),
- #[error("A commit could not be decoded during traversal")]
- Decode(#[from] gix_object::decode::Error),
+ #[error("A commit could not be inserted into the graph")]
+ InsertCommit(#[from] gix_revwalk::graph::get_or_insert_default::Error),
 }
 
 pub(crate) mod function {
  use super::Error;
  use crate::{merge_base::Flags, Graph, PriorityQueue};
  use gix_hash::ObjectId;
- use gix_revwalk::graph::LazyCommit;
+ use gix_revwalk::graph;
  use std::cmp::Ordering;
 
  /// Given a commit at `first` id, traverse the commit `graph` and return all possible merge-base between it and `others`,
@@ -39,19 +35,23 @@ pub(crate) mod function {
  ///
  /// Note that this function doesn't do any work if `first` is contained in `others`, which is when `first` will be returned
  /// as only merge-base right away. This is even the case if some commits of `others` are disjoint.
+ ///
+ /// # Performance
+ ///
+ /// For repeated calls, be sure to re-use `graph` as its content will be kept and reused for a great speed-up. The contained flags
+ /// will automatically be cleared.
  pub fn merge_base(
  first: ObjectId,
  others: &[ObjectId],
- graph: &mut Graph<'_, '_, Flags>,
+ graph: &mut Graph<'_, '_, graph::Commit<Flags>>,
  ) -> Result<Option<Vec<ObjectId>>, Error> {
  let _span = gix_trace::coarse!("gix_revision::merge_base()", ?first, ?others);
  if others.is_empty() || others.contains(&first) {
  return Ok(Some(vec![first]));
  }
 
- graph.clear();
+ graph.clear_commit_data(|f| *f = Flags::empty());
  let bases = paint_down_to_common(first, others, graph)?;
- graph.clear();
 
  let bases = remove_redundant(&bases, graph)?;
  Ok((!bases.is_empty()).then_some(bases))
@@ -61,11 +61,12 @@ pub(crate) mod function {
  /// That way, we return only the topologically most recent commits in `commits`.
  fn remove_redundant(
  commits: &[(ObjectId, GenThenTime)],
- graph: &mut Graph<'_, '_, Flags>,
+ graph: &mut Graph<'_, '_, graph::Commit<Flags>>,
  ) -> Result<Vec<ObjectId>, Error> {
  if commits.is_empty() {
  return Ok(Vec::new());
  }
+ graph.clear_commit_data(|f| *f = Flags::empty());
  let _span = gix_trace::detail!("gix_revision::remove_redundant()", num_commits = %commits.len());
  let sorted_commits = {
  let mut v = commits.to_vec();
@@ -77,36 +78,46 @@ pub(crate) mod function {
 
  let mut walk_start = Vec::with_capacity(commits.len());
  for (id, _) in commits {
- graph.insert_parents_with_lookup(id, &mut |parent_id, parent_data, maybe_flags| -> Result<_, Error> {
- if maybe_flags.map_or(true, |flags| !flags.contains(Flags::STALE)) {
- walk_start.push((parent_id, GenThenTime::try_from(parent_data)?));
- }
- Ok(Flags::empty())
- })?;
- graph.insert(*id, Flags::RESULT);
+ let commit = graph.get_mut(id).expect("previously added");
+ commit.data |= Flags::RESULT;
+ for parent_id in commit.parents.clone() {
+ graph.get_or_insert_full_commit(parent_id, |parent| {
+ // prevent double-addition
+ if !parent.data.contains(Flags::STALE) {
+ parent.data |= Flags::STALE;
+ walk_start.push((parent_id, GenThenTime::from(&*parent)));
+ }
+ })?;
+ }
  }
  walk_start.sort_by(|a, b| a.0.cmp(&b.0));
+ // allow walking everything at first.
+ walk_start
+ .iter_mut()
+ .for_each(|(id, _)| graph.get_mut(id).expect("added previously").data.remove(Flags::STALE));
  let mut count_still_independent = commits.len();
 
  let mut stack = Vec::new();
  while let Some((commit_id, commit_info)) = walk_start.pop().filter(|_| count_still_independent > 1) {
  stack.clear();
- graph.insert(commit_id, Flags::STALE);
+ graph.get_mut(&commit_id).expect("added").data |= Flags::STALE;
  stack.push((commit_id, commit_info));
 
  while let Some((commit_id, commit_info)) = stack.last().copied() {
- let flags = graph.get_mut(&commit_id).expect("all commits have been added");
- if flags.contains(Flags::RESULT) {
- flags.remove(Flags::RESULT);
+ let commit = graph.get_mut(&commit_id).expect("all commits have been added");
+ let commit_parents = commit.parents.clone();
+ if commit.data.contains(Flags::RESULT) {
+ commit.data.remove(Flags::RESULT);
  count_still_independent -= 1;
  if count_still_independent <= 1 {
  break;
  }
- if commit_id == sorted_commits[min_gen_pos].0 {
+ if *commit_id == *sorted_commits[min_gen_pos].0 {
  while min_gen_pos < commits.len() - 1
  && graph
  .get(&sorted_commits[min_gen_pos].0)
  .expect("already added")
+ .data
  .contains(Flags::STALE)
  {
  min_gen_pos += 1;
@@ -120,87 +131,81 @@ pub(crate) mod function {
  continue;
  }
 
- let mut pushed_one_parent = false;
- graph.insert_parents_with_lookup(&commit_id, &mut |parent_id,
- parent_data,
- maybe_flags|
- -> Result<_, Error> {
- let is_new_parent = !pushed_one_parent
- && maybe_flags.map_or(true, |flags| {
- let res = !flags.contains(Flags::STALE);
- *flags |= Flags::STALE;
- res
- });
- if is_new_parent {
- stack.push((parent_id, GenThenTime::try_from(parent_data)?));
- pushed_one_parent = true;
+ let previous_len = stack.len();
+ for parent_id in &commit_parents {
+ if graph
+ .get_or_insert_full_commit(*parent_id, |parent| {
+ if !parent.data.contains(Flags::STALE) {
+ parent.data |= Flags::STALE;
+ stack.push((*parent_id, GenThenTime::from(&*parent)));
+ }
+ })?
+ .is_some()
+ {
+ break;
  }
- Ok(Flags::STALE)
- })?;
+ }
 
- if !pushed_one_parent {
+ if previous_len == stack.len() {
  stack.pop();
  }
  }
  }
 
  Ok(commits
  .iter()
- .filter_map(|(id, _info)| graph.get(id).filter(|flags| !flags.contains(Flags::STALE)).map(|_| *id))
+ .filter_map(|(id, _info)| {
+ graph
+ .get(id)
+ .filter(|commit| !commit.data.contains(Flags::STALE))
+ .map(|_| *id)
+ })
  .collect())
  }
 
  fn paint_down_to_common(
  first: ObjectId,
  others: &[ObjectId],
- graph: &mut Graph<'_, '_, Flags>,
+ graph: &mut Graph<'_, '_, graph::Commit<Flags>>,
  ) -> Result<Vec<(ObjectId, GenThenTime)>, Error> {
  let mut queue = PriorityQueue::<GenThenTime, ObjectId>::new();
- graph.insert_data(first, |commit| -> Result<_, Error> {
- queue.insert(commit.try_into()?, first);
- Ok(Flags::COMMIT1)
+ graph.get_or_insert_full_commit(first, |commit| {
+ commit.data |= Flags::COMMIT1;
+ queue.insert(GenThenTime::from(&*commit), first);
  })?;
 
  for other in others {
- graph.insert_data(*other, |commit| -> Result<_, Error> {
- queue.insert(commit.try_into()?, *other);
- Ok(Flags::COMMIT2)
+ graph.get_or_insert_full_commit(*other, |commit| {
+ commit.data |= Flags::COMMIT2;
+ queue.insert(GenThenTime::from(&*commit), *other);
  })?;
  }
 
  let mut out = Vec::new();
- while queue
- .iter_unordered()
- .any(|id| graph.get(id).map_or(false, |data| !data.contains(Flags::STALE)))
- {
+ while queue.iter_unordered().any(|id| {
+ graph
+ .get(id)
+ .map_or(false, |commit| !commit.data.contains(Flags::STALE))
+ }) {
  let (info, commit_id) = queue.pop().expect("we have non-stale");
- let flags_mut = graph.get_mut(&commit_id).expect("everything queued is in graph");
- let mut flags_without_result = *flags_mut & (Flags::COMMIT1 | Flags::COMMIT2 | Flags::STALE);
+ let commit = graph.get_mut(&commit_id).expect("everything queued is in graph");
+ let mut flags_without_result = commit.data & (Flags::COMMIT1 | Flags::COMMIT2 | Flags::STALE);
  if flags_without_result == (Flags::COMMIT1 | Flags::COMMIT2) {
- if !flags_mut.contains(Flags::RESULT) {
- *flags_mut |= Flags::RESULT;
+ if !commit.data.contains(Flags::RESULT) {
+ commit.data |= Flags::RESULT;
  out.push((commit_id, info));
  }
  flags_without_result |= Flags::STALE;
  }
 
- graph.insert_parents_with_lookup(&commit_id, &mut |parent_id, parent, ex_flags| -> Result<_, Error> {
- let queue_info = match ex_flags {
- Some(ex_flags) => {
- if (*ex_flags & flags_without_result) != flags_without_result {
- *ex_flags |= flags_without_result;
- Some(GenThenTime::try_from(parent)?)
- } else {
- None
- }
+ for parent_id in commit.parents.clone() {
+ graph.get_or_insert_full_commit(parent_id, |parent| {
+ if (parent.data & flags_without_result) != flags_without_result {
+ parent.data |= flags_without_result;
+ queue.insert(GenThenTime::from(&*parent), parent_id);
  }
- None => Some(GenThenTime::try_from(parent)?),
- };
- if let Some(info) = queue_info {
- queue.insert(info, parent_id);
- }
- Ok(flags_without_result)
- })?;
+ })?;
+ }
  }
 
  Ok(out)
@@ -215,15 +220,12 @@ pub(crate) mod function {
  time: gix_date::SecondsSinceUnixEpoch,
  }
 
- impl TryFrom<gix_revwalk::graph::LazyCommit<'_, '_>> for GenThenTime {
- type Error = gix_object::decode::Error;
-
- fn try_from(commit: LazyCommit<'_, '_>) -> Result<Self, Self::Error> {
- let (generation, timestamp) = commit.generation_and_timestamp()?;
- Ok(GenThenTime {
- generation: generation.unwrap_or(gix_commitgraph::GENERATION_NUMBER_INFINITY),
- time: timestamp,
- })
+ impl From<&graph::Commit<Flags>> for GenThenTime {
+ fn from(commit: &graph::Commit<Flags>) -> Self {
+ GenThenTime {
+ generation: commit.generation.unwrap_or(gix_commitgraph::GENERATION_NUMBER_INFINITY),
+ time: commit.commit_time,
+ }
  }
  }
 

gix-revision/tests/merge_base/mod.rs

@@ -17,7 +17,6 @@ mod baseline {
  .then(|| gix_commitgraph::Graph::from_info_dir(&odb.store_ref().path().join("info")).unwrap());
  for expected in parse_expectations(&baseline_path)? {
  let mut graph = gix_revision::Graph::new(&odb, cache.as_ref());
-
  let actual = merge_base(expected.first, &expected.others, &mut graph)?;
  assert_eq!(
  actual,
@@ -27,6 +26,17 @@ mod baseline {
  input = expected.plain_input
  );
  }
+ let mut graph = gix_revision::Graph::new(&odb, cache.as_ref());
+ for expected in parse_expectations(&baseline_path)? {
+ let actual = merge_base(expected.first, &expected.others, &mut graph)?;
+ assert_eq!(
+ actual,
+ expected.bases,
+ "sample (reused graph) {file:?}:{input}",
+ file = baseline_path.with_extension("").file_name(),
+ input = expected.plain_input
+ );
+ }
  }
  }
  assert_ne!(count, 0, "there must be at least one baseline");