From a16555f9af882d9324feb172d5258c7ae6c6a590 Mon Sep 17 00:00:00 2001 From: avmikhailov Date: Tue, 14 Jan 2025 13:24:33 +0100 Subject: [PATCH] index: Return Result from heads there are many ways to implement `heads` for your custom backend: one involves calling `self.evaluate_revset` with a proper revset. However, it can return an error, but `heads` does not allow it. In our implementation of the index backend we do exactly the above and we ended up with several unwraps which we are trying to avoid as it could potentially crash our prod jobs. P.S. The same logic applies to many methods in this trait, but I am doing one at a time. --- cli/src/commands/simplify_parents.rs | 7 ++++++- lib/src/default_index/composite.rs | 11 ++++++++--- lib/src/default_index/mod.rs | 21 ++++++++++++++------- lib/src/default_index/mutable.rs | 6 +++++- lib/src/default_index/readonly.rs | 6 +++++- lib/src/index.rs | 10 +++++++++- lib/src/repo.rs | 4 ++++ lib/src/rewrite.rs | 12 +++++++++--- 8 files changed, 60 insertions(+), 17 deletions(-) diff --git a/cli/src/commands/simplify_parents.rs b/cli/src/commands/simplify_parents.rs index 6c12692526..a842d36077 100644 --- a/cli/src/commands/simplify_parents.rs +++ b/cli/src/commands/simplify_parents.rs @@ -2,6 +2,7 @@ use std::collections::HashSet; use clap_complete::ArgValueCandidates; use itertools::Itertools; +use jj_lib::backend::BackendError; use jj_lib::revset::RevsetExpression; use crate::cli_util::CommandHelper; @@ -87,7 +88,11 @@ pub(crate) fn cmd_simplify_parents( .transform_descendants(commit_ids, |mut rewriter| { let num_old_heads = rewriter.new_parents().len(); if commit_ids_set.contains(rewriter.old_commit().id()) && num_old_heads > 1 { - rewriter.simplify_ancestor_merge(); + // TODO: BackendError is not the right error here because + // the error does not come from `Backend`, but `Index`. + rewriter + .simplify_ancestor_merge() + .map_err(|err| BackendError::Other(err.into()))?; } let num_new_heads = rewriter.new_parents().len(); diff --git a/lib/src/default_index/composite.rs b/lib/src/default_index/composite.rs index ca03e3b4b1..bacd5721b0 100644 --- a/lib/src/default_index/composite.rs +++ b/lib/src/default_index/composite.rs @@ -43,6 +43,7 @@ use crate::hex_util; use crate::index::AllHeadsForGcUnsupported; use crate::index::ChangeIdIndex; use crate::index::Index; +use crate::index::IndexError; use crate::object_id::HexPrefix; use crate::object_id::ObjectId; use crate::object_id::PrefixResolution; @@ -487,15 +488,19 @@ impl Index for &CompositeIndex { Ok(Box::new(self.all_heads())) } - fn heads(&self, candidate_ids: &mut dyn Iterator) -> Vec { + fn heads( + &self, + candidate_ids: &mut dyn Iterator, + ) -> Result, IndexError> { let candidate_positions: BTreeSet<_> = candidate_ids .map(|id| self.commit_id_to_pos(id).unwrap()) .collect(); - self.heads_pos(candidate_positions) + Ok(self + .heads_pos(candidate_positions) .iter() .map(|pos| self.entry_by_pos(*pos).commit_id()) - .collect() + .collect()) } fn evaluate_revset<'index>( diff --git a/lib/src/default_index/mod.rs b/lib/src/default_index/mod.rs index 63d47960fe..dbee6ed8a1 100644 --- a/lib/src/default_index/mod.rs +++ b/lib/src/default_index/mod.rs @@ -1180,32 +1180,39 @@ mod tests { index.add_commit_data(id_5.clone(), new_change_id(), &[id_4.clone(), id_2.clone()]); // Empty input - assert!(index.heads(&mut [].iter()).is_empty()); + assert!(index.heads(&mut [].iter()).unwrap().is_empty()); // Single head - assert_eq!(index.heads(&mut [id_4.clone()].iter()), vec![id_4.clone()]); + assert_eq!( + index.heads(&mut [id_4.clone()].iter()).unwrap(), + vec![id_4.clone()] + ); // Single head and parent assert_eq!( - index.heads(&mut [id_4.clone(), id_1].iter()), + index.heads(&mut [id_4.clone(), id_1].iter()).unwrap(), vec![id_4.clone()] ); // Single head and grand-parent assert_eq!( - index.heads(&mut [id_4.clone(), id_0].iter()), + index.heads(&mut [id_4.clone(), id_0].iter()).unwrap(), vec![id_4.clone()] ); // Multiple heads assert_eq!( - index.heads(&mut [id_4.clone(), id_3.clone()].iter()), + index + .heads(&mut [id_4.clone(), id_3.clone()].iter()) + .unwrap(), vec![id_3.clone(), id_4] ); // Merge commit and ancestors assert_eq!( - index.heads(&mut [id_5.clone(), id_2].iter()), + index.heads(&mut [id_5.clone(), id_2].iter()).unwrap(), vec![id_5.clone()] ); // Merge commit and other commit assert_eq!( - index.heads(&mut [id_5.clone(), id_3.clone()].iter()), + index + .heads(&mut [id_5.clone(), id_3.clone()].iter()) + .unwrap(), vec![id_3.clone(), id_5.clone()] ); diff --git a/lib/src/default_index/mutable.rs b/lib/src/default_index/mutable.rs index 353550dbeb..53f674eae1 100644 --- a/lib/src/default_index/mutable.rs +++ b/lib/src/default_index/mutable.rs @@ -51,6 +51,7 @@ use crate::file_util::persist_content_addressed_temp_file; use crate::index::AllHeadsForGcUnsupported; use crate::index::ChangeIdIndex; use crate::index::Index; +use crate::index::IndexError; use crate::index::MutableIndex; use crate::index::ReadonlyIndex; use crate::object_id::HexPrefix; @@ -517,7 +518,10 @@ impl Index for DefaultMutableIndex { Ok(Box::new(self.as_composite().all_heads())) } - fn heads(&self, candidates: &mut dyn Iterator) -> Vec { + fn heads( + &self, + candidates: &mut dyn Iterator, + ) -> Result, IndexError> { self.as_composite().heads(candidates) } diff --git a/lib/src/default_index/readonly.rs b/lib/src/default_index/readonly.rs index 111680b632..e1a177a818 100644 --- a/lib/src/default_index/readonly.rs +++ b/lib/src/default_index/readonly.rs @@ -41,6 +41,7 @@ use crate::backend::CommitId; use crate::index::AllHeadsForGcUnsupported; use crate::index::ChangeIdIndex; use crate::index::Index; +use crate::index::IndexError; use crate::index::MutableIndex; use crate::index::ReadonlyIndex; use crate::object_id::HexPrefix; @@ -633,7 +634,10 @@ impl Index for DefaultReadonlyIndex { Ok(Box::new(self.as_composite().all_heads())) } - fn heads(&self, candidates: &mut dyn Iterator) -> Vec { + fn heads( + &self, + candidates: &mut dyn Iterator, + ) -> Result, IndexError> { self.as_composite().heads(candidates) } diff --git a/lib/src/index.rs b/lib/src/index.rs index b1263f5bbb..1a98ec496c 100644 --- a/lib/src/index.rs +++ b/lib/src/index.rs @@ -41,6 +41,11 @@ pub struct IndexReadError(pub Box); #[error(transparent)] pub struct IndexWriteError(pub Box); +/// Returned by [`Index`] backend in case of an error. +#[derive(Debug, Error)] +#[error(transparent)] +pub struct IndexError(pub Box); + /// An error returned if `Index::all_heads_for_gc()` is not supported by the /// index backend. #[derive(Debug, Error)] @@ -117,7 +122,10 @@ pub trait Index: Send + Sync { /// Returns the subset of commit IDs in `candidates` which are not ancestors /// of other commits in `candidates`. If a commit id is duplicated in the /// `candidates` list it will appear at most once in the output. - fn heads(&self, candidates: &mut dyn Iterator) -> Vec; + fn heads( + &self, + candidates: &mut dyn Iterator, + ) -> Result, IndexError>; /// Resolves the revset `expression` against the index and corresponding /// `store`. diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 1c82e3c3c0..947da81cb3 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -1438,9 +1438,13 @@ impl MutableRepo { // An empty head_ids set is padded with the root_commit_id, but the // root id is unwanted during the heads resolution. view.head_ids.remove(root_commit_id); + // It is unclear if `heads` can never fail for default implementation, + // but it can definitely fail for non-default implementations. + // TODO: propagate errors. view.head_ids = self .index() .heads(&mut view.head_ids.iter()) + .unwrap() .into_iter() .collect(); } diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index 3ade0fbe04..5ff01d5059 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -34,6 +34,7 @@ use crate::commit::CommitIteratorExt; use crate::commit_builder::CommitBuilder; use crate::dag_walk; use crate::index::Index; +use crate::index::IndexError; use crate::matchers::Matcher; use crate::matchers::Visit; use crate::merged_tree::MergedTree; @@ -197,14 +198,15 @@ impl<'repo> CommitRewriter<'repo> { /// If a merge commit would end up with one parent being an ancestor of the /// other, then filter out the ancestor. - pub fn simplify_ancestor_merge(&mut self) { + pub fn simplify_ancestor_merge(&mut self) -> Result<(), IndexError> { let head_set: HashSet<_> = self .mut_repo .index() - .heads(&mut self.new_parents.iter()) + .heads(&mut self.new_parents.iter())? .into_iter() .collect(); self.new_parents.retain(|parent| head_set.contains(parent)); + Ok(()) } /// Records the old commit as abandoned with the new parents. @@ -304,7 +306,11 @@ pub fn rebase_commit_with_options( ) -> BackendResult { // If specified, don't create commit where one parent is an ancestor of another. if options.simplify_ancestor_merge { - rewriter.simplify_ancestor_merge(); + // TODO: BackendError is not the right error here because + // the error does not come from `Backend`, but `Index`. + rewriter + .simplify_ancestor_merge() + .map_err(|err| BackendError::Other(err.into()))?; } let single_parent = match &rewriter.new_parents[..] {