From a6ee51a998bea39883e96c2a3fcd43626bcc4ad5 Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Fri, 15 Mar 2024 17:49:23 +0900
Subject: [PATCH] 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.
---
 cli/src/cli_util.rs        |  6 +++---
 cli/src/commands/bench.rs  |  4 ++--
 cli/src/commands/branch.rs |  1 -
 cli/src/commands/log.rs    |  2 +-
 cli/src/revset_util.rs     | 12 ++----------
 5 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs
index 8c76d9238..92a71f101 100644
--- a/cli/src/cli_util.rs
+++ b/cli/src/cli_util.rs
@@ -67,7 +67,7 @@ use jj_lib::working_copy::{
 use jj_lib::workspace::{
     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 tracing::instrument;
 use tracing_chrome::ChromeLayerBuilder;
@@ -823,7 +823,7 @@ Set which revision the branch points to with `jj branch set {branch_name} -r <RE
         &self,
         revision_str: &str,
     ) -> 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>(
@@ -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| {
                     CommandError::ConfigError(format!("Invalid `revsets.short-prefixes`: {err}"))
                 })?;
-                context = context.disambiguate_within(disambiguation_revset);
+                context = context.disambiguate_within(revset::optimize(disambiguation_revset));
             }
             Ok(context)
         })
diff --git a/cli/src/commands/bench.rs b/cli/src/commands/bench.rs
index 53b9ffe60..5c0e805f4 100644
--- a/cli/src/commands/bench.rs
+++ b/cli/src/commands/bench.rs
@@ -23,7 +23,7 @@ use criterion::measurement::Measurement;
 use criterion::{BatchSize, BenchmarkGroup, BenchmarkId, Criterion};
 use jj_lib::object_id::HexPrefix;
 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::command_error::CommandError;
@@ -204,7 +204,7 @@ fn bench_revset<M: Measurement>(
     revset: &str,
 ) -> Result<(), CommandError> {
     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.
     let routine = |workspace_command: &WorkspaceCommandHelper, expression: Rc<RevsetExpression>| {
         // Evaluate the expression without parsing/evaluating short-prefixes.
diff --git a/cli/src/commands/branch.rs b/cli/src/commands/branch.rs
index 9f936ffed..c4321b569 100644
--- a/cli/src/commands/branch.rs
+++ b/cli/src/commands/branch.rs
@@ -625,7 +625,6 @@ fn cmd_branch_list(
             // Intersects with the set of local branch targets to minimize the lookup space.
             let revset_expression = RevsetExpression::branches(StringPattern::everything())
                 .intersection(&filter_expression);
-            let revset_expression = revset::optimize(revset_expression);
             let revset = workspace_command.evaluate_revset(revset_expression)?;
             let filtered_targets: HashSet<CommitId> = revset.iter().collect();
             branch_names.extend(
diff --git a/cli/src/commands/log.rs b/cli/src/commands/log.rs
index 44767eac7..d7d9e4f4c 100644
--- a/cli/src/commands/log.rs
+++ b/cli/src/commands/log.rs
@@ -96,7 +96,7 @@ pub(crate) fn cmd_log(
                 RevsetFilterPredicate::File(Some(repo_paths)),
             ));
         }
-        revset::optimize(expression)
+        expression
     };
     let repo = workspace_command.repo();
     let wc_commit_id = workspace_command.get_wc_commit_id();
diff --git a/cli/src/revset_util.rs b/cli/src/revset_util.rs
index 80958be1c..9f1faf927 100644
--- a/cli/src/revset_util.rs
+++ b/cli/src/revset_util.rs
@@ -80,20 +80,12 @@ pub fn load_revset_aliases(
     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>(
     repo: &'a dyn Repo,
     symbol_resolver: &DefaultSymbolResolver,
     expression: Rc<RevsetExpression>,
 ) -> Result<Box<dyn Revset + 'a>, UserRevsetEvaluationError> {
-    let resolved = expression
+    let resolved = revset::optimize(expression)
         .resolve_user_expression(repo, symbol_resolver)
         .map_err(UserRevsetEvaluationError::Resolution)?;
     resolved
@@ -128,7 +120,7 @@ pub fn parse_immutable_expression(
         params.is_empty(),
         "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
         .ancestors()
         .union(&RevsetExpression::root()))