revset: allow iterators to return evaluation errors

Custom backends may rely on networking or other unreliable implementations to support revsets, this change allows them to return errors cleanly instead of panicking. For simplicity, only the public-facing Revset and RevsetGraph types are changed in this commit; the internal revset engine remains mostly unchanged and error-free since it cannot generally produce errors.
jj-vcs · Oct 11, 2024 · 9b9dde7 · 9b9dde7
1 parent d8aabda
commit 9b9dde7
Show file tree

Hide file tree

Showing 22 changed files with 257 additions and 148 deletions.
diff --git a/cli/examples/custom-commit-templater/main.rs b/cli/examples/custom-commit-templater/main.rs
@@ -75,6 +75,7 @@ impl MostDigitsInId {
                 .evaluate_programmatic(repo)
                 .unwrap()
                 .iter()
+                .unwrapped()
                 .map(|id| num_digits_in_id(&id))
                 .max()
                 .unwrap_or(0)
@@ -102,6 +103,7 @@ impl PartialSymbolResolver for TheDigitestResolver {
                 .evaluate_programmatic(repo)
                 .map_err(|err| RevsetResolutionError::Other(err.into()))?
                 .iter()
+                .unwrapped()
                 .filter(|id| num_digits_in_id(id) == self.cache.count(repo))
                 .collect_vec(),
         ))

diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs
@@ -763,6 +763,7 @@ impl WorkspaceCommandEnvironment {
         })?;
 
         if let Some(commit_id) = commit_id_iter.next() {
+            let commit_id = commit_id?;
             let error = if &commit_id == repo.store().root_commit_id() {
                 user_error(format!(
                     "The root commit {} is immutable",

diff --git a/cli/src/commands/bookmark/list.rs b/cli/src/commands/bookmark/list.rs
@@ -14,6 +14,7 @@
 
 use std::collections::HashSet;
 
+use itertools::Itertools;
 use jj_lib::git;
 use jj_lib::revset::RevsetExpression;
 use jj_lib::str_util::StringPattern;
@@ -100,7 +101,8 @@ pub fn cmd_bookmark_list(
             // Intersects with the set of local bookmark targets to minimize the lookup
             // space.
             expression.intersect_with(&RevsetExpression::bookmarks(StringPattern::everything()));
-            let filtered_targets: HashSet<_> = expression.evaluate_to_commit_ids()?.collect();
+            let filtered_targets: HashSet<_> =
+                expression.evaluate_to_commit_ids()?.try_collect()?;
             bookmark_names.extend(
                 view.local_bookmarks()
                     .filter(|(_, target)| {

diff --git a/cli/src/commands/debug/revset.rs b/cli/src/commands/debug/revset.rs
@@ -69,7 +69,7 @@ pub fn cmd_debug_revset(
 
     writeln!(ui.stdout(), "-- Commit IDs:")?;
     for commit_id in revset.iter() {
-        writeln!(ui.stdout(), "{}", commit_id.hex())?;
+        writeln!(ui.stdout(), "{}", commit_id?.hex())?;
     }
     Ok(())
 }
diff --git a/cli/src/commands/duplicate.rs b/cli/src/commands/duplicate.rs
@@ -15,6 +15,7 @@
 use std::io::Write;
 
 use indexmap::IndexMap;
+use itertools::Itertools;
 use jj_lib::backend::CommitId;
 use jj_lib::commit::Commit;
 use jj_lib::repo::Repo;
@@ -48,7 +49,7 @@ pub(crate) fn cmd_duplicate(
     let to_duplicate: Vec<CommitId> = workspace_command
         .parse_union_revsets(ui, &args.revisions)?
         .evaluate_to_commit_ids()?
-        .collect(); // in reverse topological order
+        .try_collect()?; // in reverse topological order
     if to_duplicate.is_empty() {
         writeln!(ui.status(), "No revisions to duplicate.")?;
         return Ok(());

diff --git a/cli/src/commands/fix.rs b/cli/src/commands/fix.rs
@@ -150,7 +150,7 @@ pub(crate) fn cmd_fix(
         workspace_command.parse_union_revsets(ui, &args.source)?
     }
     .evaluate_to_commit_ids()?
-    .collect();
+    .try_collect()?;
     workspace_command.check_rewritable(root_commits.iter())?;
     let matcher = workspace_command
         .parse_file_patterns(ui, &args.paths)?

diff --git a/cli/src/commands/git/push.rs b/cli/src/commands/git/push.rs
@@ -612,7 +612,9 @@ fn find_bookmarks_targeted_by_revisions<'a>(
         .intersection(&RevsetExpression::bookmarks(StringPattern::everything()));
         let current_bookmarks_revset = current_bookmarks_expression
             .evaluate_programmatic(workspace_command.repo().as_ref())?;
-        revision_commit_ids.extend(current_bookmarks_revset.iter());
+        for commit_id in current_bookmarks_revset.iter() {
+            revision_commit_ids.insert(commit_id?);
+        }
         if revision_commit_ids.is_empty() {
             writeln!(
                 ui.warning_default(),
@@ -631,7 +633,9 @@ fn find_bookmarks_targeted_by_revisions<'a>(
                 "No bookmarks point to the specified revisions: {rev_arg}"
             )?;
         }
-        revision_commit_ids.extend(commit_ids);
+        for commit_id in commit_ids {
+            revision_commit_ids.insert(commit_id?);
+        }
     }
     let bookmarks_targeted = workspace_command
         .repo()

diff --git a/cli/src/commands/log.rs b/cli/src/commands/log.rs
@@ -19,6 +19,7 @@ use jj_lib::graph::TopoGroupedGraphIterator;
 use jj_lib::repo::Repo;
 use jj_lib::revset::RevsetExpression;
 use jj_lib::revset::RevsetFilterPredicate;
+use jj_lib::revset::RevsetIterator;
 use jj_lib::revset::RevsetIteratorExt;
 use jj_lib::settings::ConfigResultExt as _;
 use jj_lib::settings::UserSettings;
@@ -172,11 +173,13 @@ pub(crate) fn cmd_log(
             let mut graph = get_graphlog(graph_style, formatter.raw());
             let forward_iter = TopoGroupedGraphIterator::new(revset.iter_graph());
             let iter: Box<dyn Iterator<Item = _>> = if args.reversed {
-                Box::new(ReverseGraphIterator::new(forward_iter))
+                Box::new(ReverseGraphIterator::new(forward_iter)?)
             } else {
                 Box::new(forward_iter)
             };
-            for (commit_id, edges) in iter.take(limit) {
+            for node in iter.take(limit) {
+                let (commit_id, edges) = node?;
+
                 // The graph is keyed by (CommitId, is_synthetic)
                 let mut graphlog_edges = vec![];
                 // TODO: Should we update revset.iter_graph() to yield this flag instead of all
@@ -254,8 +257,8 @@ pub(crate) fn cmd_log(
                 }
             }
         } else {
-            let iter: Box<dyn Iterator<Item = CommitId>> = if args.reversed {
-                Box::new(revset.iter().reversed())
+            let iter: Box<dyn RevsetIterator<CommitId>> = if args.reversed {
+                Box::new(revset.iter().reversed()?)
             } else {
                 Box::new(revset.iter())
             };

diff --git a/cli/src/commands/new.rs b/cli/src/commands/new.rs
@@ -242,6 +242,7 @@ fn ensure_no_commit_loop(
         .iter()
         .next()
     {
+        let commit_id = commit_id?;
         return Err(user_error(format!(
             "Refusing to create a loop: commit {} would be both an ancestor and a descendant of \
              the new commit",

diff --git a/cli/src/commands/operation/diff.rs b/cli/src/commands/operation/diff.rs
@@ -202,16 +202,17 @@ pub fn show_op_diff(
             let graph_iter =
                 TopoGroupedGraphIterator::new(ordered_change_ids.iter().map(|change_id| {
                     let parent_change_ids = change_parents.get(change_id).unwrap();
-                    (
+                    Ok((
                         change_id.clone(),
                         parent_change_ids
                             .iter()
                             .map(|parent_change_id| GraphEdge::direct(parent_change_id.clone()))
                             .collect_vec(),
-                    )
+                    ))
                 }));
 
-            for (change_id, edges) in graph_iter {
+            for node in graph_iter {
+                let (change_id, edges) = node?;
                 let modified_change = changes.get(&change_id).unwrap();
                 let edges = edges
                     .iter()

diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs
@@ -645,6 +645,7 @@ fn ensure_no_commit_loop(
         .iter()
         .next()
     {
+        let commit_id = commit_id?;
         return Err(user_error(format!(
             "Refusing to create a loop: commit {} would be both an ancestor and a descendant of \
              the rebased commits",

diff --git a/cli/src/commands/simplify_parents.rs b/cli/src/commands/simplify_parents.rs
@@ -48,10 +48,10 @@ pub(crate) fn cmd_simplify_parents(
             .parse_union_revsets(ui, &args.revisions)?
             .expression(),
     );
-    let commit_ids = workspace_command
+    let commit_ids: Vec<_> = workspace_command
         .attach_revset_evaluator(revs)
         .evaluate_to_commit_ids()?
-        .collect_vec();
+        .try_collect()?;
     workspace_command.check_rewritable(&commit_ids)?;
     let commit_ids_set: HashSet<_> = commit_ids.iter().cloned().collect();
     let num_orig_commits = commit_ids.len();

diff --git a/cli/src/commands/status.rs b/cli/src/commands/status.rs
@@ -111,7 +111,7 @@ pub(crate) fn cmd_status(
             let wc_revset = RevsetExpression::commit(wc_commit.id().clone());
 
             // Ancestors with conflicts, excluding the current working copy commit.
-            let ancestors_conflicts = workspace_command
+            let ancestors_conflicts: Vec<_> = workspace_command
                 .attach_revset_evaluator(
                     wc_revset
                         .parents()
@@ -120,7 +120,7 @@ pub(crate) fn cmd_status(
                         .minus(&workspace_command.env().immutable_expression()),
                 )
                 .evaluate_to_commit_ids()?
-                .collect();
+                .try_collect()?;
 
             workspace_command.report_repo_conflicts(formatter, repo, ancestors_conflicts)?;
         } else {

diff --git a/cli/src/revset_util.rs b/cli/src/revset_util.rs
@@ -33,6 +33,7 @@ use jj_lib::revset::RevsetDiagnostics;
 use jj_lib::revset::RevsetEvaluationError;
 use jj_lib::revset::RevsetExpression;
 use jj_lib::revset::RevsetExtensions;
+use jj_lib::revset::RevsetIterator;
 use jj_lib::revset::RevsetIteratorExt as _;
 use jj_lib::revset::RevsetParseContext;
 use jj_lib::revset::RevsetParseError;
@@ -106,7 +107,7 @@ impl<'repo> RevsetExpressionEvaluator<'repo> {
     /// sorted in reverse topological order.
     pub fn evaluate_to_commit_ids(
         &self,
-    ) -> Result<Box<dyn Iterator<Item = CommitId> + 'repo>, UserRevsetEvaluationError> {
+    ) -> Result<Box<dyn RevsetIterator<CommitId> + 'repo>, UserRevsetEvaluationError> {
         Ok(self.evaluate()?.iter())
     }
 

diff --git a/lib/src/default_index/revset_engine.rs b/lib/src/default_index/revset_engine.rs
@@ -59,6 +59,7 @@ use crate::revset::ResolvedPredicateExpression;
 use crate::revset::Revset;
 use crate::revset::RevsetEvaluationError;
 use crate::revset::RevsetFilterPredicate;
+use crate::revset::RevsetIterator;
 use crate::revset::GENERATION_RANGE_FULL;
 use crate::rewrite;
 use crate::store::Store;
@@ -130,11 +131,11 @@ impl<I: AsCompositeIndex + Clone> RevsetImpl<I> {
     pub fn iter_graph_impl(
         &self,
         skip_transitive_edges: bool,
-    ) -> impl Iterator<Item = (CommitId, Vec<GraphEdge<CommitId>>)> {
+    ) -> impl RevsetIterator<(CommitId, Vec<GraphEdge<CommitId>>)> {
         let index = self.index.clone();
         let walk = self.inner.positions();
         let mut graph_walk = RevsetGraphWalk::new(walk, skip_transitive_edges);
-        iter::from_fn(move || graph_walk.next(index.as_composite()))
+        iter::from_fn(move || graph_walk.next(index.as_composite()).map(Ok))
     }
 }
 
@@ -147,7 +148,7 @@ impl<I> fmt::Debug for RevsetImpl<I> {
 }
 
 impl<I: AsCompositeIndex + Clone> Revset for RevsetImpl<I> {
-    fn iter<'a>(&self) -> Box<dyn Iterator<Item = CommitId> + 'a>
+    fn iter<'a>(&self) -> Box<dyn RevsetIterator<CommitId> + 'a>
     where
         Self: 'a,
     {
@@ -156,11 +157,11 @@ impl<I: AsCompositeIndex + Clone> Revset for RevsetImpl<I> {
         Box::new(iter::from_fn(move || {
             let index = index.as_composite();
             let pos = walk.next(index)?;
-            Some(index.entry_by_pos(pos).commit_id())
+            Some(Ok(index.entry_by_pos(pos).commit_id()))
         }))
     }
 
-    fn commit_change_ids<'a>(&self) -> Box<dyn Iterator<Item = (CommitId, ChangeId)> + 'a>
+    fn commit_change_ids<'a>(&self) -> Box<dyn RevsetIterator<(CommitId, ChangeId)> + 'a>
     where
         Self: 'a,
     {
@@ -170,11 +171,11 @@ impl<I: AsCompositeIndex + Clone> Revset for RevsetImpl<I> {
             let index = index.as_composite();
             let pos = walk.next(index)?;
             let entry = index.entry_by_pos(pos);
-            Some((entry.commit_id(), entry.change_id()))
+            Some(Ok((entry.commit_id(), entry.change_id())))
         }))
     }
 
-    fn iter_graph<'a>(&self) -> Box<dyn Iterator<Item = (CommitId, Vec<GraphEdge<CommitId>>)> + 'a>
+    fn iter_graph<'a>(&self) -> Box<dyn RevsetIterator<(CommitId, Vec<GraphEdge<CommitId>>)> + 'a>
     where
         Self: 'a,
     {

diff --git a/lib/src/git.rs b/lib/src/git.rs
@@ -380,6 +380,7 @@ fn abandon_unreachable_commits(
         .evaluate_programmatic(mut_repo)
         .unwrap()
         .iter()
+        .unwrapped()
         .collect_vec();
     for abandoned_commit in &abandoned_commits {
         mut_repo.record_abandoned_commit(abandoned_commit.clone());