cli: move revset::optimize() invocation from parse() to evaluate()

AST substitution is technically closer to parsing, but the parsed expression
can be modified further by caller. So I think it's better to do optimize() in
later pass.

revset_util::parse() is inlined.
This commit is contained in:
Yuya Nishihara 2024-03-15 17:49:23 +09:00
parent 70a2a44f42
commit a6ee51a998
5 changed files with 8 additions and 17 deletions

View File

@ -67,7 +67,7 @@ use jj_lib::working_copy::{
use jj_lib::workspace::{ use jj_lib::workspace::{
default_working_copy_factories, LockedWorkspace, Workspace, WorkspaceLoadError, WorkspaceLoader, default_working_copy_factories, LockedWorkspace, Workspace, WorkspaceLoadError, WorkspaceLoader,
}; };
use jj_lib::{dag_walk, file_util, git, op_heads_store, op_walk}; use jj_lib::{dag_walk, file_util, git, op_heads_store, op_walk, revset};
use once_cell::unsync::OnceCell; use once_cell::unsync::OnceCell;
use tracing::instrument; use tracing::instrument;
use tracing_chrome::ChromeLayerBuilder; use tracing_chrome::ChromeLayerBuilder;
@ -823,7 +823,7 @@ Set which revision the branch points to with `jj branch set {branch_name} -r <RE
&self, &self,
revision_str: &str, revision_str: &str,
) -> Result<Rc<RevsetExpression>, RevsetParseError> { ) -> Result<Rc<RevsetExpression>, RevsetParseError> {
revset_util::parse(revision_str, &self.revset_parse_context()) revset::parse(revision_str, &self.revset_parse_context())
} }
pub fn evaluate_revset<'repo>( pub fn evaluate_revset<'repo>(
@ -860,7 +860,7 @@ Set which revision the branch points to with `jj branch set {branch_name} -r <RE
let disambiguation_revset = self.parse_revset(&revset_string).map_err(|err| { let disambiguation_revset = self.parse_revset(&revset_string).map_err(|err| {
CommandError::ConfigError(format!("Invalid `revsets.short-prefixes`: {err}")) CommandError::ConfigError(format!("Invalid `revsets.short-prefixes`: {err}"))
})?; })?;
context = context.disambiguate_within(disambiguation_revset); context = context.disambiguate_within(revset::optimize(disambiguation_revset));
} }
Ok(context) Ok(context)
}) })

View File

@ -23,7 +23,7 @@ use criterion::measurement::Measurement;
use criterion::{BatchSize, BenchmarkGroup, BenchmarkId, Criterion}; use criterion::{BatchSize, BenchmarkGroup, BenchmarkId, Criterion};
use jj_lib::object_id::HexPrefix; use jj_lib::object_id::HexPrefix;
use jj_lib::repo::Repo; use jj_lib::repo::Repo;
use jj_lib::revset::{DefaultSymbolResolver, RevsetExpression}; use jj_lib::revset::{self, DefaultSymbolResolver, RevsetExpression};
use crate::cli_util::{CommandHelper, WorkspaceCommandHelper}; use crate::cli_util::{CommandHelper, WorkspaceCommandHelper};
use crate::command_error::CommandError; use crate::command_error::CommandError;
@ -204,7 +204,7 @@ fn bench_revset<M: Measurement>(
revset: &str, revset: &str,
) -> Result<(), CommandError> { ) -> Result<(), CommandError> {
writeln!(ui.stderr(), "----------Testing revset: {revset}----------")?; writeln!(ui.stderr(), "----------Testing revset: {revset}----------")?;
let expression = workspace_command.parse_revset(revset)?; let expression = revset::optimize(workspace_command.parse_revset(revset)?);
// Time both evaluation and iteration. // Time both evaluation and iteration.
let routine = |workspace_command: &WorkspaceCommandHelper, expression: Rc<RevsetExpression>| { let routine = |workspace_command: &WorkspaceCommandHelper, expression: Rc<RevsetExpression>| {
// Evaluate the expression without parsing/evaluating short-prefixes. // Evaluate the expression without parsing/evaluating short-prefixes.

View File

@ -625,7 +625,6 @@ fn cmd_branch_list(
// Intersects with the set of local branch targets to minimize the lookup space. // Intersects with the set of local branch targets to minimize the lookup space.
let revset_expression = RevsetExpression::branches(StringPattern::everything()) let revset_expression = RevsetExpression::branches(StringPattern::everything())
.intersection(&filter_expression); .intersection(&filter_expression);
let revset_expression = revset::optimize(revset_expression);
let revset = workspace_command.evaluate_revset(revset_expression)?; let revset = workspace_command.evaluate_revset(revset_expression)?;
let filtered_targets: HashSet<CommitId> = revset.iter().collect(); let filtered_targets: HashSet<CommitId> = revset.iter().collect();
branch_names.extend( branch_names.extend(

View File

@ -96,7 +96,7 @@ pub(crate) fn cmd_log(
RevsetFilterPredicate::File(Some(repo_paths)), RevsetFilterPredicate::File(Some(repo_paths)),
)); ));
} }
revset::optimize(expression) expression
}; };
let repo = workspace_command.repo(); let repo = workspace_command.repo();
let wc_commit_id = workspace_command.get_wc_commit_id(); let wc_commit_id = workspace_command.get_wc_commit_id();

View File

@ -80,20 +80,12 @@ pub fn load_revset_aliases(
Ok(aliases_map) Ok(aliases_map)
} }
pub fn parse(
revision_str: &str,
context: &RevsetParseContext,
) -> Result<Rc<RevsetExpression>, RevsetParseError> {
let expression = revset::parse(revision_str, context)?;
Ok(revset::optimize(expression))
}
pub fn evaluate<'a>( pub fn evaluate<'a>(
repo: &'a dyn Repo, repo: &'a dyn Repo,
symbol_resolver: &DefaultSymbolResolver, symbol_resolver: &DefaultSymbolResolver,
expression: Rc<RevsetExpression>, expression: Rc<RevsetExpression>,
) -> Result<Box<dyn Revset + 'a>, UserRevsetEvaluationError> { ) -> Result<Box<dyn Revset + 'a>, UserRevsetEvaluationError> {
let resolved = expression let resolved = revset::optimize(expression)
.resolve_user_expression(repo, symbol_resolver) .resolve_user_expression(repo, symbol_resolver)
.map_err(UserRevsetEvaluationError::Resolution)?; .map_err(UserRevsetEvaluationError::Resolution)?;
resolved resolved
@ -128,7 +120,7 @@ pub fn parse_immutable_expression(
params.is_empty(), params.is_empty(),
"invalid declaration should have been rejected by load_revset_aliases()" "invalid declaration should have been rejected by load_revset_aliases()"
); );
let immutable_heads_revset = parse(immutable_heads_str, context)?; let immutable_heads_revset = revset::parse(immutable_heads_str, context)?;
Ok(immutable_heads_revset Ok(immutable_heads_revset
.ancestors() .ancestors()
.union(&RevsetExpression::root())) .union(&RevsetExpression::root()))