revset: propagate evaluation errors from other Revset methods

is_empty() could also return Result<bool, _>, but I think the current definition
is also good. If an error occurred, revset.iter() would return at least one
item, so it's not empty.

cli/src/commands/bookmark/move.rs

@@ -13,7 +13,6 @@
 // limitations under the License.
 
 use itertools::Itertools as _;
-use jj_lib::backend::CommitId;
 use jj_lib::object_id::ObjectId as _;
 use jj_lib::op_store::RefTarget;
 use jj_lib::str_util::StringPattern;
@@ -77,26 +76,41 @@ pub fn cmd_bookmark_move(
 
     let target_commit = workspace_command.resolve_single_rev(ui, &args.to)?;
     let matched_bookmarks = {
-        let is_source_commit = if !args.from.is_empty() {
-            workspace_command
+        let is_source_ref: Box<dyn Fn(&RefTarget) -> _> = if !args.from.is_empty() {
+            let is_source_commit = workspace_command
                 .parse_union_revsets(ui, &args.from)?
                 .evaluate()?
-                .containing_fn()
+                .containing_fn();
+            Box::new(move |target: &RefTarget| {
+                for id in target.added_ids() {
+                    if is_source_commit(id)? {
+                        return Ok(true);
+                    }
+                }
+                Ok(false)
+            })
         } else {
-            Box::new(|_: &CommitId| true)
+            Box::new(|_| Ok(true))
         };
         let mut bookmarks = if !args.names.is_empty() {
             find_bookmarks_with(&args.names, |pattern| {
                 repo.view()
                     .local_bookmarks_matching(pattern)
-                    .filter(|(_, target)| target.added_ids().any(&is_source_commit))
-                    .map(Ok)
+                    .filter_map(|(name, target)| {
+                        is_source_ref(target)
+                            .map(|matched| matched.then_some((name, target)))
+                            .transpose()
+                    })
             })?
         } else {
             repo.view()
                 .local_bookmarks()
-                .filter(|(_, target)| target.added_ids().any(&is_source_commit))
-                .collect()
+                .filter_map(|(name, target)| {
+                    is_source_ref(target)
+                        .map(|matched| matched.then_some((name, target)))
+                        .transpose()
+                })
+                .try_collect()?
         };
         // Noop matches aren't error, but should be excluded from stats.
         bookmarks.retain(|(_, old_target)| old_target.as_normal() != Some(target_commit.id()));

cli/src/commands/git/push.rs

@@ -336,7 +336,7 @@ fn validate_commits_ready_to_push(
             .evaluate()?
             .containing_fn()
     } else {
-        Box::new(|_: &CommitId| false)
+        Box::new(|_: &CommitId| Ok(false))
     };
 
     for commit in workspace_helper
@@ -362,7 +362,7 @@ fn validate_commits_ready_to_push(
         if commit.has_conflict()? {
             reasons.push("it has conflicts");
         }
-        if !args.allow_private && is_private(commit.id()) {
+        if !args.allow_private && is_private(commit.id())? {
             reasons.push("it is private");
         }
         if !reasons.is_empty() {

cli/src/commit_templater.rs

@@ -743,7 +743,7 @@ fn builtin_commit_methods<'repo>() -> CommitTemplateBuildMethodFnMap<'repo, Comm
                 .keyword_cache
                 .is_immutable_fn(language, function.name_span)?
                 .clone();
-            let out_property = self_property.map(move |commit| is_immutable(commit.id()));
+            let out_property = self_property.and_then(move |commit| Ok(is_immutable(commit.id())?));
             Ok(L::wrap_boolean(out_property))
         },
     );
@@ -757,7 +757,7 @@ fn builtin_commit_methods<'repo>() -> CommitTemplateBuildMethodFnMap<'repo, Comm
                     Ok(evaluate_user_revset(language, diagnostics, span, revset)?.containing_fn())
                 })?;
 
-            let out_property = self_property.map(move |commit| is_contained(commit.id()));
+            let out_property = self_property.and_then(move |commit| Ok(is_contained(commit.id())?));
             Ok(L::wrap_boolean(out_property))
         },
     );
@@ -1020,7 +1020,7 @@ impl RefName {
             .get_or_try_init(|| {
                 let self_ids = self.target.added_ids().cloned().collect_vec();
                 let other_ids = tracking.target.added_ids().cloned().collect_vec();
-                Ok(revset::walk_revs(repo, &self_ids, &other_ids)?.count_estimate())
+                Ok(revset::walk_revs(repo, &self_ids, &other_ids)?.count_estimate()?)
             })
             .copied()
     }
@@ -1035,7 +1035,7 @@ impl RefName {
             .get_or_try_init(|| {
                 let self_ids = self.target.added_ids().cloned().collect_vec();
                 let other_ids = tracking.target.added_ids().cloned().collect_vec();
-                Ok(revset::walk_revs(repo, &other_ids, &self_ids)?.count_estimate())
+                Ok(revset::walk_revs(repo, &other_ids, &self_ids)?.count_estimate()?)
             })
             .copied()
     }

lib/src/default_index/revset_engine.rs

@@ -191,20 +191,21 @@ impl<I: AsCompositeIndex + Clone> Revset for RevsetImpl<I> {
         self.positions().next().is_none()
     }
 
-    fn count_estimate(&self) -> (usize, Option<usize>) {
+    fn count_estimate(&self) -> Result<(usize, Option<usize>), RevsetEvaluationError> {
         if cfg!(feature = "testing") {
             // Exercise the estimation feature in tests. (If we ever have a Revset
             // implementation in production code that returns estimates, we can probably
             // remove this and rewrite the associated tests.)
             let count = self.positions().take(10).count();
             if count < 10 {
-                (count, Some(count))
+                Ok((count, Some(count)))
             } else {
-                (10, None)
+                Ok((10, None))
             }
         } else {
+            // TODO: propagate error
             let count = self.positions().count();
-            (count, Some(count))
+            Ok((count, Some(count)))
         }
     }
 
@@ -213,7 +214,7 @@ impl<I: AsCompositeIndex + Clone> Revset for RevsetImpl<I> {
         Self: 'a,
     {
         let positions = PositionsAccumulator::new(self.index.clone(), self.inner.positions());
-        Box::new(move |commit_id| positions.contains(commit_id))
+        Box::new(move |commit_id| Ok(positions.contains(commit_id)))
     }
 }
 

lib/src/revset.rs

@@ -2207,13 +2207,14 @@ pub trait Revset: fmt::Debug {
     where
         Self: 'a;
 
+    /// Returns true if iterator will emit at least one commit or error.
     fn is_empty(&self) -> bool;
 
     /// Inclusive lower bound and, optionally, inclusive upper bound of how many
     /// commits are in the revset. The implementation can use its discretion as
     /// to how much effort should be put into the estimation, and how accurate
     /// the resulting estimate should be.
-    fn count_estimate(&self) -> (usize, Option<usize>);
+    fn count_estimate(&self) -> Result<(usize, Option<usize>), RevsetEvaluationError>;
 
     /// Returns a closure that checks if a commit is contained within the
     /// revset.
@@ -2226,7 +2227,7 @@ pub trait Revset: fmt::Debug {
 }
 
 /// Function that checks if a commit is contained within the revset.
-pub type RevsetContainingFn<'a> = dyn Fn(&CommitId) -> bool + 'a;
+pub type RevsetContainingFn<'a> = dyn Fn(&CommitId) -> Result<bool, RevsetEvaluationError> + 'a;
 
 pub trait RevsetIteratorExt<'index, I> {
     fn commits(self, store: &Arc<Store>) -> RevsetCommitIterator<I>;

lib/tests/test_revset.rs

@@ -3765,8 +3765,8 @@ fn test_revset_containing_fn() {
     let revset = revset_for_commits(repo.as_ref(), &[&commit_b, &commit_d]);
 
     let revset_has_commit = revset.containing_fn();
-    assert!(!revset_has_commit(commit_a.id()));
-    assert!(revset_has_commit(commit_b.id()));
-    assert!(!revset_has_commit(commit_c.id()));
-    assert!(revset_has_commit(commit_d.id()));
+    assert!(!revset_has_commit(commit_a.id()).unwrap());
+    assert!(revset_has_commit(commit_b.id()).unwrap());
+    assert!(!revset_has_commit(commit_c.id()).unwrap());
+    assert!(revset_has_commit(commit_d.id()).unwrap());
 }