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

@@ -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,9 @@ 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();
+                rewriter
+                    .simplify_ancestor_merge()
+                    .map_err(|err| BackendError::Other(err.into()))?;
             }
             let num_new_heads = rewriter.new_parents().len();
 

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<Item = &CommitId>) -> Vec<CommitId> {
+    fn heads(
+        &self,
+        candidate_ids: &mut dyn Iterator<Item = &CommitId>,
+    ) -> Result<Vec<CommitId>, 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>(

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()]
         );
 

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<Item = &CommitId>) -> Vec<CommitId> {
+    fn heads(
+        &self,
+        candidates: &mut dyn Iterator<Item = &CommitId>,
+    ) -> Result<Vec<CommitId>, IndexError> {
         self.as_composite().heads(candidates)
     }
 

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<Item = &CommitId>) -> Vec<CommitId> {
+    fn heads(
+        &self,
+        candidates: &mut dyn Iterator<Item = &CommitId>,
+    ) -> Result<Vec<CommitId>, IndexError> {
         self.as_composite().heads(candidates)
     }
 

lib/src/index.rs

@@ -41,6 +41,11 @@ pub struct IndexReadError(pub Box<dyn std::error::Error + Send + Sync>);
 #[error(transparent)]
 pub struct IndexWriteError(pub Box<dyn std::error::Error + Send + Sync>);
 
+/// Returned by [`Index`] backend in case of an error.
+#[derive(Debug, Error)]
+#[error(transparent)]
+pub struct IndexError(pub Box<dyn std::error::Error + Send + Sync>);
+
 /// 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<Item = &CommitId>) -> Vec<CommitId>;
+    fn heads(
+        &self,
+        candidates: &mut dyn Iterator<Item = &CommitId>,
+    ) -> Result<Vec<CommitId>, IndexError>;
 
     /// Resolves the revset `expression` against the index and corresponding
     /// `store`.

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();
         }

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,9 @@ pub fn rebase_commit_with_options(
 ) -> BackendResult<RebasedCommit> {
     // If specified, don't create commit where one parent is an ancestor of another.
     if options.simplify_ancestor_merge {
-        rewriter.simplify_ancestor_merge();
+        rewriter
+            .simplify_ancestor_merge()
+            .map_err(|err| BackendError::Other(err.into()))?;
     }
 
     let single_parent = match &rewriter.new_parents[..] {