revset: split RevsetError into RevsetResolution/EvaluationError

This makes it clear that RevsetExpression::Present node is noop at the
evaluation stage.

RevsetEvaluationError::StoreError is unused right now, but I'm not sure if
it should be removed. It makes some sense that evaluate() can propagate
StoreError as it has access to the store.

lib/src/default_index_store.rs

@@ -44,7 +44,7 @@ use crate::index::{
 use crate::nightly_shims::BTreeSetExt;
 use crate::op_store::OperationId;
 use crate::operation::Operation;
-use crate::revset::{Revset, RevsetError, RevsetExpression};
+use crate::revset::{Revset, RevsetEvaluationError, RevsetExpression};
 use crate::store::Store;
 use crate::{backend, dag_walk, default_revset_engine};
 
@@ -732,7 +732,7 @@ impl Index for MutableIndexImpl {
         expression: &RevsetExpression,
         store: &Arc<Store>,
         visible_heads: &[CommitId],
-    ) -> Result<Box<dyn Revset<'index> + 'index>, RevsetError> {
+    ) -> Result<Box<dyn Revset<'index> + 'index>, RevsetEvaluationError> {
         let revset_impl = default_revset_engine::evaluate(
             expression,
             store,
@@ -1820,7 +1820,7 @@ impl Index for ReadonlyIndexImpl {
         expression: &RevsetExpression,
         store: &Arc<Store>,
         visible_heads: &[CommitId],
-    ) -> Result<Box<dyn Revset<'index> + 'index>, RevsetError> {
+    ) -> Result<Box<dyn Revset<'index> + 'index>, RevsetEvaluationError> {
         let revset_impl = default_revset_engine::evaluate(
             expression,
             store,

lib/src/default_revset_engine.rs

@@ -26,8 +26,8 @@ use crate::default_revset_graph_iterator::RevsetGraphIterator;
 use crate::index::{HexPrefix, Index, PrefixResolution};
 use crate::matchers::{EverythingMatcher, Matcher, PrefixMatcher};
 use crate::revset::{
-    ChangeIdIndex, Revset, RevsetError, RevsetExpression, RevsetFilterPredicate, RevsetGraphEdge,
-    GENERATION_RANGE_FULL,
+    ChangeIdIndex, Revset, RevsetEvaluationError, RevsetExpression, RevsetFilterPredicate,
+    RevsetGraphEdge, GENERATION_RANGE_FULL,
 };
 use crate::store::Store;
 use crate::{backend, rewrite};
@@ -507,7 +507,7 @@ pub fn evaluate<'index>(
     index: &'index dyn Index,
     composite_index: CompositeIndex<'index>,
     visible_heads: &[CommitId],
-) -> Result<RevsetImpl<'index>, RevsetError> {
+) -> Result<RevsetImpl<'index>, RevsetEvaluationError> {
     let context = EvaluationContext {
         store: store.clone(),
         index,
@@ -529,7 +529,7 @@ impl<'index, 'heads> EvaluationContext<'index, 'heads> {
     fn evaluate(
         &self,
         expression: &RevsetExpression,
-    ) -> Result<Box<dyn InternalRevset<'index> + 'index>, RevsetError> {
+    ) -> Result<Box<dyn InternalRevset<'index> + 'index>, RevsetEvaluationError> {
         match expression {
             RevsetExpression::Symbol(_)
             | RevsetExpression::Branches(_)
@@ -644,11 +644,7 @@ impl<'index, 'heads> EvaluationContext<'index, 'heads> {
                 predicate: build_predicate_fn(self.store.clone(), self.index, predicate),
             })),
             RevsetExpression::AsFilter(candidates) => self.evaluate(candidates),
-            RevsetExpression::Present(candidates) => match self.evaluate(candidates) {
-                Ok(set) => Ok(set),
-                Err(RevsetError::NoSuchRevision(_)) => Ok(Box::new(EagerRevset::empty())),
-                r @ Err(RevsetError::AmbiguousIdPrefix(_) | RevsetError::StoreError(_)) => r,
-            },
+            RevsetExpression::Present(candidates) => self.evaluate(candidates),
             RevsetExpression::NotIn(complement) => {
                 let set1 = self.evaluate(&RevsetExpression::All)?;
                 let set2 = self.evaluate(complement)?;

lib/src/index.rs

@@ -23,7 +23,7 @@ use crate::commit::Commit;
 use crate::default_index_store::{IndexEntry, RevWalk};
 use crate::op_store::OperationId;
 use crate::operation::Operation;
-use crate::revset::{Revset, RevsetError, RevsetExpression};
+use crate::revset::{Revset, RevsetEvaluationError, RevsetExpression};
 use crate::store::Store;
 
 #[derive(Debug, Error)]
@@ -73,7 +73,7 @@ pub trait Index: Send + Sync {
         expression: &RevsetExpression,
         store: &Arc<Store>,
         visible_heads: &[CommitId],
-    ) -> Result<Box<dyn Revset<'index> + 'index>, RevsetError>;
+    ) -> Result<Box<dyn Revset<'index> + 'index>, RevsetEvaluationError>;
 }
 
 pub trait ReadonlyIndex: Send + Sync {

lib/src/revset.rs

@@ -37,8 +37,9 @@ use crate::repo::Repo;
 use crate::repo_path::{FsPathParseError, RepoPath};
 use crate::store::Store;
 
+/// Error occurred during symbol resolution.
 #[derive(Debug, Error)]
-pub enum RevsetError {
+pub enum RevsetResolutionError {
     #[error("Revision \"{0}\" doesn't exist")]
     NoSuchRevision(String),
     #[error("Commit or change id prefix \"{0}\" is ambiguous")]
@@ -47,6 +48,13 @@ pub enum RevsetError {
     StoreError(#[source] BackendError),
 }
 
+/// Error occurred during revset evaluation.
+#[derive(Debug, Error)]
+pub enum RevsetEvaluationError {
+    #[error("Unexpected error from store: {0}")]
+    StoreError(#[source] BackendError),
+}
+
 #[derive(Parser)]
 #[grammar = "revset.pest"]
 pub struct RevsetParser;
@@ -402,7 +410,7 @@ impl RevsetExpression {
     pub fn evaluate<'index>(
         &self,
         repo: &'index dyn Repo,
-    ) -> Result<Box<dyn Revset<'index> + 'index>, RevsetError> {
+    ) -> Result<Box<dyn Revset<'index> + 'index>, RevsetEvaluationError> {
         repo.index().evaluate_revset(
             self,
             repo.store(),
@@ -1101,7 +1109,8 @@ fn transform_expression_bottom_up(
     try_transform_expression_bottom_up(expression, |expression| Ok(f(expression))).unwrap()
 }
 
-type TransformResult = Result<Option<Rc<RevsetExpression>>, RevsetError>;
+type TransformResult = Result<Option<Rc<RevsetExpression>>, RevsetResolutionError>;
+
 /// Walks `expression` tree and applies `f` recursively from leaf nodes.
 ///
 /// If `f` returns `None`, the original expression node is reused. If no nodes
@@ -1167,8 +1176,11 @@ fn try_transform_expression_bottom_up(
             RevsetExpression::Present(candidates) => match transform_rec(candidates, f) {
                 Ok(None) => None,
                 Ok(Some(expression)) => Some(RevsetExpression::Present(expression)),
-                Err(RevsetError::NoSuchRevision(_)) => Some(RevsetExpression::None),
-                r @ Err(RevsetError::AmbiguousIdPrefix(_) | RevsetError::StoreError(_)) => {
+                Err(RevsetResolutionError::NoSuchRevision(_)) => Some(RevsetExpression::None),
+                r @ Err(
+                    RevsetResolutionError::AmbiguousIdPrefix(_)
+                    | RevsetResolutionError::StoreError(_),
+                ) => {
                     return r;
                 }
             },
@@ -1202,7 +1214,7 @@ fn try_transform_expression_bottom_up(
     fn transform_rec_pair(
         (expression1, expression2): (&Rc<RevsetExpression>, &Rc<RevsetExpression>),
         f: &mut impl FnMut(&Rc<RevsetExpression>) -> TransformResult,
-    ) -> Result<Option<(Rc<RevsetExpression>, Rc<RevsetExpression>)>, RevsetError> {
+    ) -> Result<Option<(Rc<RevsetExpression>, Rc<RevsetExpression>)>, RevsetResolutionError> {
         match (
             transform_rec(expression1, f)?,
             transform_rec(expression2, f)?,
@@ -1475,7 +1487,7 @@ fn resolve_branch(repo: &dyn Repo, symbol: &str) -> Option<Vec<CommitId>> {
 fn resolve_full_commit_id(
     repo: &dyn Repo,
     symbol: &str,
-) -> Result<Option<Vec<CommitId>>, RevsetError> {
+) -> Result<Option<Vec<CommitId>>, RevsetResolutionError> {
     if let Ok(binary_commit_id) = hex::decode(symbol) {
         if repo.store().commit_id_length() != binary_commit_id.len() {
             return Ok(None);
@@ -1485,7 +1497,7 @@ fn resolve_full_commit_id(
             // Only recognize a commit if we have indexed it
             Ok(_) if repo.index().has_id(&commit_id) => Ok(Some(vec![commit_id])),
             Ok(_) | Err(BackendError::ObjectNotFound { .. }) => Ok(None),
-            Err(err) => Err(RevsetError::StoreError(err)),
+            Err(err) => Err(RevsetResolutionError::StoreError(err)),
         }
     } else {
         Ok(None)
@@ -1495,12 +1507,12 @@ fn resolve_full_commit_id(
 fn resolve_short_commit_id(
     repo: &dyn Repo,
     symbol: &str,
-) -> Result<Option<Vec<CommitId>>, RevsetError> {
+) -> Result<Option<Vec<CommitId>>, RevsetResolutionError> {
     if let Some(prefix) = HexPrefix::new(symbol) {
         match repo.index().resolve_prefix(&prefix) {
             PrefixResolution::NoMatch => Ok(None),
             PrefixResolution::AmbiguousMatch => {
-                Err(RevsetError::AmbiguousIdPrefix(symbol.to_owned()))
+                Err(RevsetResolutionError::AmbiguousIdPrefix(symbol.to_owned()))
             }
             PrefixResolution::SingleMatch(commit_id) => Ok(Some(vec![commit_id])),
         }
@@ -1509,12 +1521,15 @@ fn resolve_short_commit_id(
     }
 }
 
-fn resolve_change_id(repo: &dyn Repo, symbol: &str) -> Result<Option<Vec<CommitId>>, RevsetError> {
+fn resolve_change_id(
+    repo: &dyn Repo,
+    symbol: &str,
+) -> Result<Option<Vec<CommitId>>, RevsetResolutionError> {
     if let Some(prefix) = to_forward_hex(symbol).as_deref().and_then(HexPrefix::new) {
         match repo.resolve_change_id_prefix(&prefix) {
             PrefixResolution::NoMatch => Ok(None),
             PrefixResolution::AmbiguousMatch => {
-                Err(RevsetError::AmbiguousIdPrefix(symbol.to_owned()))
+                Err(RevsetResolutionError::AmbiguousIdPrefix(symbol.to_owned()))
             }
             PrefixResolution::SingleMatch(entries) => Ok(Some(entries)),
         }
@@ -1527,13 +1542,13 @@ pub fn resolve_symbol(
     repo: &dyn Repo,
     symbol: &str,
     workspace_id: Option<&WorkspaceId>,
-) -> Result<Vec<CommitId>, RevsetError> {
+) -> Result<Vec<CommitId>, RevsetResolutionError> {
     if symbol.ends_with('@') {
         let target_workspace = if symbol == "@" {
             if let Some(workspace_id) = workspace_id {
                 workspace_id.clone()
             } else {
-                return Err(RevsetError::NoSuchRevision(symbol.to_owned()));
+                return Err(RevsetResolutionError::NoSuchRevision(symbol.to_owned()));
             }
         } else {
             WorkspaceId::new(symbol.strip_suffix('@').unwrap().to_string())
@@ -1541,7 +1556,7 @@ pub fn resolve_symbol(
         if let Some(commit_id) = repo.view().get_wc_commit_id(&target_workspace) {
             Ok(vec![commit_id.clone()])
         } else {
-            Err(RevsetError::NoSuchRevision(symbol.to_owned()))
+            Err(RevsetResolutionError::NoSuchRevision(symbol.to_owned()))
         }
     } else if symbol == "root" {
         Ok(vec![repo.store().root_commit_id().clone()])
@@ -1577,7 +1592,7 @@ pub fn resolve_symbol(
             return Ok(ids);
         }
 
-        Err(RevsetError::NoSuchRevision(symbol.to_owned()))
+        Err(RevsetResolutionError::NoSuchRevision(symbol.to_owned()))
     }
 }
 
@@ -1588,7 +1603,7 @@ pub fn resolve_symbols(
     repo: &dyn Repo,
     expression: Rc<RevsetExpression>,
     workspace_ctx: Option<&RevsetWorkspaceContext>,
-) -> Result<Rc<RevsetExpression>, RevsetError> {
+) -> Result<Rc<RevsetExpression>, RevsetResolutionError> {
     Ok(
         try_transform_expression_bottom_up(&expression, |expression| {
             Ok(match expression.as_ref() {

lib/tests/test_revset.rs

@@ -25,8 +25,8 @@ use jujutsu_lib::repo::Repo;
 use jujutsu_lib::repo_path::RepoPath;
 use jujutsu_lib::revset::{
     optimize, parse, resolve_symbol, resolve_symbols, ReverseRevsetGraphIterator, Revset,
-    RevsetAliasesMap, RevsetError, RevsetExpression, RevsetFilterPredicate, RevsetGraphEdge,
-    RevsetWorkspaceContext,
+    RevsetAliasesMap, RevsetExpression, RevsetFilterPredicate, RevsetGraphEdge,
+    RevsetResolutionError, RevsetWorkspaceContext,
 };
 use jujutsu_lib::settings::GitSettings;
 use jujutsu_lib::workspace::Workspace;
@@ -141,7 +141,7 @@ fn test_resolve_symbol_commit_id() {
     // Test empty commit id
     assert_matches!(
         resolve_symbol(repo.as_ref(), "", None),
-        Err(RevsetError::AmbiguousIdPrefix(s)) if s.is_empty()
+        Err(RevsetResolutionError::AmbiguousIdPrefix(s)) if s.is_empty()
     );
 
     // Test commit id prefix
@@ -151,28 +151,28 @@ fn test_resolve_symbol_commit_id() {
     );
     assert_matches!(
         resolve_symbol(repo.as_ref(), "04", None),
-        Err(RevsetError::AmbiguousIdPrefix(s)) if s == "04"
+        Err(RevsetResolutionError::AmbiguousIdPrefix(s)) if s == "04"
     );
     assert_matches!(
         resolve_symbol(repo.as_ref(), "", None),
-        Err(RevsetError::AmbiguousIdPrefix(s)) if s.is_empty()
+        Err(RevsetResolutionError::AmbiguousIdPrefix(s)) if s.is_empty()
     );
     assert_matches!(
         resolve_symbol(repo.as_ref(), "040", None),
-        Err(RevsetError::NoSuchRevision(s)) if s == "040"
+        Err(RevsetResolutionError::NoSuchRevision(s)) if s == "040"
     );
 
     // Test non-hex string
     assert_matches!(
         resolve_symbol(repo.as_ref(), "foo", None),
-        Err(RevsetError::NoSuchRevision(s)) if s == "foo"
+        Err(RevsetResolutionError::NoSuchRevision(s)) if s == "foo"
     );
 
     // Test present() suppresses only NoSuchRevision error
     assert_eq!(resolve_commit_ids(repo.as_ref(), "present(foo)"), []);
     assert_matches!(
         resolve_symbols(repo.as_ref(), optimize(parse("present(04)", &RevsetAliasesMap::new(), None).unwrap()), None),
-        Err(RevsetError::AmbiguousIdPrefix(s)) if s == "04"
+        Err(RevsetResolutionError::AmbiguousIdPrefix(s)) if s == "04"
     );
     assert_eq!(
         resolve_commit_ids(repo.as_ref(), "present(046)"),
@@ -288,15 +288,15 @@ fn test_resolve_symbol_change_id(readonly: bool) {
     );
     assert_matches!(
         resolve_symbol(repo, "zvly", None),
-        Err(RevsetError::AmbiguousIdPrefix(s)) if s == "zvly"
+        Err(RevsetResolutionError::AmbiguousIdPrefix(s)) if s == "zvly"
     );
     assert_matches!(
         resolve_symbol(repo, "", None),
-        Err(RevsetError::AmbiguousIdPrefix(s)) if s.is_empty()
+        Err(RevsetResolutionError::AmbiguousIdPrefix(s)) if s.is_empty()
     );
     assert_matches!(
         resolve_symbol(repo, "zvlyw", None),
-        Err(RevsetError::NoSuchRevision(s)) if s == "zvlyw"
+        Err(RevsetResolutionError::NoSuchRevision(s)) if s == "zvlyw"
     );
 
     // Test that commit and changed id don't conflict ("040" and "zvz" are the
@@ -317,7 +317,7 @@ fn test_resolve_symbol_change_id(readonly: bool) {
     // Test non-hex string
     assert_matches!(
         resolve_symbol(repo, "foo", None),
-        Err(RevsetError::NoSuchRevision(s)) if s == "foo"
+        Err(RevsetResolutionError::NoSuchRevision(s)) if s == "foo"
     );
 }
 
@@ -340,15 +340,15 @@ fn test_resolve_symbol_checkout(use_git: bool) {
     // With no workspaces, no variation can be resolved
     assert_matches!(
         resolve_symbol(mut_repo, "@", None),
-        Err(RevsetError::NoSuchRevision(s)) if s == "@"
+        Err(RevsetResolutionError::NoSuchRevision(s)) if s == "@"
     );
     assert_matches!(
         resolve_symbol(mut_repo, "@", Some(&ws1)),
-        Err(RevsetError::NoSuchRevision(s)) if s == "@"
+        Err(RevsetResolutionError::NoSuchRevision(s)) if s == "@"
     );
     assert_matches!(
         resolve_symbol(mut_repo, "ws1@", Some(&ws1)),
-        Err(RevsetError::NoSuchRevision(s)) if s == "ws1@"
+        Err(RevsetResolutionError::NoSuchRevision(s)) if s == "ws1@"
     );
 
     // Add some workspaces
@@ -359,7 +359,7 @@ fn test_resolve_symbol_checkout(use_git: bool) {
     // @ cannot be resolved without a default workspace ID
     assert_matches!(
         resolve_symbol(mut_repo, "@", None),
-        Err(RevsetError::NoSuchRevision(s)) if s == "@"
+        Err(RevsetResolutionError::NoSuchRevision(s)) if s == "@"
     );
     // Can resolve "@" shorthand with a default workspace ID
     assert_eq!(
@@ -415,7 +415,7 @@ fn test_resolve_symbol_git_refs() {
     // Nonexistent ref
     assert_matches!(
         resolve_symbol(mut_repo, "nonexistent", None),
-        Err(RevsetError::NoSuchRevision(s)) if s == "nonexistent"
+        Err(RevsetResolutionError::NoSuchRevision(s)) if s == "nonexistent"
     );
 
     // Full ref

src/cli_util.rs

@@ -44,8 +44,9 @@ use jujutsu_lib::repo::{
 };
 use jujutsu_lib::repo_path::{FsPathParseError, RepoPath};
 use jujutsu_lib::revset::{
-    resolve_symbols, Revset, RevsetAliasesMap, RevsetError, RevsetExpression, RevsetIteratorExt,
-    RevsetParseError, RevsetParseErrorKind, RevsetWorkspaceContext,
+    resolve_symbols, Revset, RevsetAliasesMap, RevsetEvaluationError, RevsetExpression,
+    RevsetIteratorExt, RevsetParseError, RevsetParseErrorKind, RevsetResolutionError,
+    RevsetWorkspaceContext,
 };
 use jujutsu_lib::settings::UserSettings;
 use jujutsu_lib::transaction::Transaction;
@@ -249,6 +250,12 @@ impl From<GitExportError> for CommandError {
     }
 }
 
+impl From<RevsetEvaluationError> for CommandError {
+    fn from(err: RevsetEvaluationError) -> Self {
+        user_error(format!("{err}"))
+    }
+}
+
 impl From<RevsetParseError> for CommandError {
     fn from(err: RevsetParseError) -> Self {
         let message = iter::successors(Some(&err), |e| e.origin()).join("\n");
@@ -276,8 +283,8 @@ impl From<RevsetParseError> for CommandError {
     }
 }
 
-impl From<RevsetError> for CommandError {
-    fn from(err: RevsetError) -> Self {
+impl From<RevsetResolutionError> for CommandError {
+    fn from(err: RevsetResolutionError) -> Self {
         user_error(format!("{err}"))
     }
 }