From c28d2d7784e42e87deed7b241598677abae80780 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sun, 2 Apr 2023 10:59:22 +0900 Subject: [PATCH] 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 | 6 ++-- lib/src/default_revset_engine.rs | 14 ++++----- lib/src/index.rs | 4 +-- lib/src/revset.rs | 49 +++++++++++++++++++++----------- lib/tests/test_revset.rs | 34 +++++++++++----------- src/cli_util.rs | 15 +++++++--- 6 files changed, 70 insertions(+), 52 deletions(-) diff --git a/lib/src/default_index_store.rs b/lib/src/default_index_store.rs index 090111ca0..883843f8a 100644 --- a/lib/src/default_index_store.rs +++ b/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, visible_heads: &[CommitId], - ) -> Result + 'index>, RevsetError> { + ) -> Result + 'index>, RevsetEvaluationError> { let revset_impl = default_revset_engine::evaluate( expression, store, @@ -1820,7 +1820,7 @@ impl Index for ReadonlyIndexImpl { expression: &RevsetExpression, store: &Arc, visible_heads: &[CommitId], - ) -> Result + 'index>, RevsetError> { + ) -> Result + 'index>, RevsetEvaluationError> { let revset_impl = default_revset_engine::evaluate( expression, store, diff --git a/lib/src/default_revset_engine.rs b/lib/src/default_revset_engine.rs index 39da2f398..d6ae312ab 100644 --- a/lib/src/default_revset_engine.rs +++ b/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, RevsetError> { +) -> Result, RevsetEvaluationError> { let context = EvaluationContext { store: store.clone(), index, @@ -529,7 +529,7 @@ impl<'index, 'heads> EvaluationContext<'index, 'heads> { fn evaluate( &self, expression: &RevsetExpression, - ) -> Result + 'index>, RevsetError> { + ) -> Result + '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)?; diff --git a/lib/src/index.rs b/lib/src/index.rs index 796e72492..6b1cfac50 100644 --- a/lib/src/index.rs +++ b/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, visible_heads: &[CommitId], - ) -> Result + 'index>, RevsetError>; + ) -> Result + 'index>, RevsetEvaluationError>; } pub trait ReadonlyIndex: Send + Sync { diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 677f74cf4..8805e49d5 100644 --- a/lib/src/revset.rs +++ b/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 + 'index>, RevsetError> { + ) -> Result + '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>, RevsetError>; +type TransformResult = Result>, 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, &Rc), f: &mut impl FnMut(&Rc) -> TransformResult, - ) -> Result, Rc)>, RevsetError> { + ) -> Result, Rc)>, RevsetResolutionError> { match ( transform_rec(expression1, f)?, transform_rec(expression2, f)?, @@ -1475,7 +1487,7 @@ fn resolve_branch(repo: &dyn Repo, symbol: &str) -> Option> { fn resolve_full_commit_id( repo: &dyn Repo, symbol: &str, -) -> Result>, RevsetError> { +) -> Result>, 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>, RevsetError> { +) -> Result>, 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>, RevsetError> { +fn resolve_change_id( + repo: &dyn Repo, + symbol: &str, +) -> Result>, 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, RevsetError> { +) -> Result, 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, workspace_ctx: Option<&RevsetWorkspaceContext>, -) -> Result, RevsetError> { +) -> Result, RevsetResolutionError> { Ok( try_transform_expression_bottom_up(&expression, |expression| { Ok(match expression.as_ref() { diff --git a/lib/tests/test_revset.rs b/lib/tests/test_revset.rs index fa54bb3ff..eeb786f3a 100644 --- a/lib/tests/test_revset.rs +++ b/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 diff --git a/src/cli_util.rs b/src/cli_util.rs index e8dc1a457..82f537c99 100644 --- a/src/cli_util.rs +++ b/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 for CommandError { } } +impl From for CommandError { + fn from(err: RevsetEvaluationError) -> Self { + user_error(format!("{err}")) + } +} + impl From for CommandError { fn from(err: RevsetParseError) -> Self { let message = iter::successors(Some(&err), |e| e.origin()).join("\n"); @@ -276,8 +283,8 @@ impl From for CommandError { } } -impl From for CommandError { - fn from(err: RevsetError) -> Self { +impl From for CommandError { + fn from(err: RevsetResolutionError) -> Self { user_error(format!("{err}")) } }