diff --git a/lib/src/default_revset_engine.rs b/lib/src/default_revset_engine.rs index d61792c09..4026ab074 100644 --- a/lib/src/default_revset_engine.rs +++ b/lib/src/default_revset_engine.rs @@ -536,6 +536,8 @@ pub fn evaluate<'index>( Ok(Box::new(revset_impl)) } +// TODO: delete unused workspace_ctx argument +#[allow(clippy::only_used_in_recursion)] fn evaluate_impl<'index>( repo: &'index dyn Repo, expression: &RevsetExpression, @@ -559,8 +561,7 @@ fn evaluate_impl<'index>( } RevsetExpression::Commits(commit_ids) => Ok(revset_for_commit_ids(repo, commit_ids)), RevsetExpression::Symbol(symbol) => { - let commit_ids = resolve_symbol(repo, symbol, workspace_ctx.map(|c| c.workspace_id))?; - evaluate_impl(repo, &RevsetExpression::Commits(commit_ids), workspace_ctx) + panic!("Symbol '{}' should have been resolved by caller", symbol); } RevsetExpression::Children(roots) => { let root_set = evaluate_impl(repo, roots, workspace_ctx)?; diff --git a/lib/src/revset.rs b/lib/src/revset.rs index f16aeaa84..18ddc3410 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -30,6 +30,7 @@ use thiserror::Error; use crate::backend::{BackendError, BackendResult, CommitId}; use crate::commit::Commit; use crate::default_index_store::{IndexEntry, IndexPosition}; +use crate::default_revset_engine::resolve_symbol; use crate::op_store::WorkspaceId; use crate::repo::Repo; use crate::repo_path::{FsPathParseError, RepoPath}; @@ -1112,9 +1113,14 @@ fn try_transform_expression_bottom_up( RevsetExpression::AsFilter(candidates) => { transform_rec(candidates, f)?.map(RevsetExpression::AsFilter) } - RevsetExpression::Present(candidates) => { - transform_rec(candidates, f)?.map(RevsetExpression::Present) - } + 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(_)) => { + return r; + } + }, RevsetExpression::NotIn(complement) => { transform_rec(complement, f)?.map(RevsetExpression::NotIn) } @@ -1385,6 +1391,29 @@ pub fn optimize(expression: Rc) -> Rc { fold_difference(&expression).unwrap_or(expression) } +// TODO: Maybe return a new type (RevsetParameters?) instead of +// RevsetExpression. Then pass that to evaluate(), so it's clear which variants +// are allowed. +pub fn resolve_symbols( + repo: &dyn Repo, + expression: Rc, + workspace_ctx: Option<&RevsetWorkspaceContext>, +) -> Result, RevsetError> { + Ok( + try_transform_expression_bottom_up(&expression, |expression| { + Ok(match expression.as_ref() { + RevsetExpression::Symbol(symbol) => { + let commit_ids = + resolve_symbol(repo, symbol, workspace_ctx.map(|ctx| ctx.workspace_id))?; + Some(RevsetExpression::commits(commit_ids)) + } + _ => None, + }) + })? + .unwrap_or(expression), + ) +} + pub trait Revset<'index> { // All revsets currently iterate in order of descending index position fn iter(&self) -> Box> + '_>; diff --git a/lib/tests/test_revset.rs b/lib/tests/test_revset.rs index ad9862dc6..b2f5a6766 100644 --- a/lib/tests/test_revset.rs +++ b/lib/tests/test_revset.rs @@ -23,8 +23,9 @@ use jujutsu_lib::op_store::{RefTarget, WorkspaceId}; use jujutsu_lib::repo::Repo; use jujutsu_lib::repo_path::RepoPath; use jujutsu_lib::revset::{ - optimize, parse, ReverseRevsetGraphIterator, RevsetAliasesMap, RevsetError, RevsetExpression, - RevsetFilterPredicate, RevsetGraphEdge, RevsetIteratorExt, RevsetWorkspaceContext, + optimize, parse, resolve_symbols, ReverseRevsetGraphIterator, RevsetAliasesMap, RevsetError, + RevsetExpression, RevsetFilterPredicate, RevsetGraphEdge, RevsetIteratorExt, + RevsetWorkspaceContext, }; use jujutsu_lib::settings::GitSettings; use jujutsu_lib::workspace::Workspace; @@ -146,9 +147,7 @@ fn test_resolve_symbol_commit_id() { // Test present() suppresses only NoSuchRevision error assert_eq!(resolve_commit_ids(&repo, "present(foo)"), []); assert_matches!( - optimize(parse("present(04)", &RevsetAliasesMap::new(), None).unwrap()) - .evaluate(&repo, None) - .map(|_| ()), + resolve_symbols(&repo, optimize(parse("present(04)", &RevsetAliasesMap::new(), None).unwrap()), None), Err(RevsetError::AmbiguousIdPrefix(s)) if s == "04" ); assert_eq!( @@ -478,6 +477,7 @@ fn test_resolve_symbol_git_refs() { fn resolve_commit_ids(repo: &dyn Repo, revset_str: &str) -> Vec { let expression = optimize(parse(revset_str, &RevsetAliasesMap::new(), None).unwrap()); + let expression = resolve_symbols(repo, expression, None).unwrap(); expression .evaluate(repo, None) .unwrap() @@ -499,6 +499,7 @@ fn resolve_commit_ids_in_workspace( }; let expression = optimize(parse(revset_str, &RevsetAliasesMap::new(), Some(&workspace_ctx)).unwrap()); + let expression = resolve_symbols(repo, expression, Some(&workspace_ctx)).unwrap(); expression .evaluate(repo, Some(&workspace_ctx)) .unwrap() diff --git a/src/cli_util.rs b/src/cli_util.rs index a5e7abbb4..fd5fab77e 100644 --- a/src/cli_util.rs +++ b/src/cli_util.rs @@ -44,8 +44,8 @@ use jujutsu_lib::repo::{ }; use jujutsu_lib::repo_path::{FsPathParseError, RepoPath}; use jujutsu_lib::revset::{ - Revset, RevsetAliasesMap, RevsetError, RevsetExpression, RevsetIteratorExt, RevsetParseError, - RevsetWorkspaceContext, + resolve_symbols, Revset, RevsetAliasesMap, RevsetError, RevsetExpression, RevsetIteratorExt, + RevsetParseError, RevsetWorkspaceContext, }; use jujutsu_lib::settings::UserSettings; use jujutsu_lib::transaction::Transaction; @@ -827,6 +827,8 @@ impl WorkspaceCommandHelper { &'repo self, revset_expression: Rc, ) -> Result + 'repo>, RevsetError> { + let revset_expression = + resolve_symbols(&self.repo, revset_expression, Some(&self.revset_context()))?; revset_expression.evaluate(&self.repo, Some(&self.revset_context())) }