From 7e04af17296c542dbed798932cd1fbfa35f844e6 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Thu, 19 Sep 2024 16:52:57 +0900 Subject: [PATCH] cli: cache immutable heads revset expression This will help simplify warning handling in future patches. I'm going to add deprecation warnings to revset, so Ui will be required in order to parse a user revset expression. revset_util::parse_immutable_expression() is inlined as it's a thin wrapper around parse_immutable_heads_expression(). --- cli/src/cli_util.rs | 27 ++++++++++++++++++++++----- cli/src/commands/git/push.rs | 5 +---- cli/src/commands/status.rs | 5 +---- cli/src/commit_templater.rs | 24 ++++++++---------------- cli/src/revset_util.rs | 9 --------- cli/tests/test_commit_template.rs | 16 +++++----------- 6 files changed, 37 insertions(+), 49 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index ad3338ac0..930913116 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -595,6 +595,7 @@ pub struct WorkspaceCommandEnvironment { template_aliases_map: TemplateAliasesMap, path_converter: RepoPathUiConverter, workspace_id: WorkspaceId, + immutable_heads_expression: Rc, short_prefixes_expression: Option>, } @@ -614,8 +615,10 @@ impl WorkspaceCommandEnvironment { template_aliases_map, path_converter, workspace_id: workspace.workspace_id().to_owned(), + immutable_heads_expression: RevsetExpression::root(), short_prefixes_expression: None, }; + env.immutable_heads_expression = env.load_immutable_heads_expression()?; env.short_prefixes_expression = env.load_short_prefixes_expression()?; Ok(env) } @@ -663,6 +666,23 @@ impl WorkspaceCommandEnvironment { } } + /// User-configured expression defining the immutable set. + pub fn immutable_expression(&self) -> Rc { + // Negated ancestors expression `~::( | root())` is slightly + // easier to optimize than negated union `~(:: | root())`. + self.immutable_heads_expression.ancestors() + } + + /// User-configured expression defining the heads of the immutable set. + pub fn immutable_heads_expression(&self) -> &Rc { + &self.immutable_heads_expression + } + + fn load_immutable_heads_expression(&self) -> Result, CommandError> { + revset_util::parse_immutable_heads_expression(&self.revset_parse_context()) + .map_err(|e| config_error_with_message("Invalid `revset-aliases.immutable_heads()`", e)) + } + fn load_short_prefixes_expression(&self) -> Result>, CommandError> { let revset_string = self .settings() @@ -704,15 +724,11 @@ impl WorkspaceCommandEnvironment { let id_prefix_context = IdPrefixContext::new(self.command.revset_extensions().clone()); let to_rewrite_revset = RevsetExpression::commits(commits.into_iter().cloned().collect_vec()); - let immutable = revset_util::parse_immutable_expression(&self.revset_parse_context()) - .map_err(|e| { - config_error_with_message("Invalid `revset-aliases.immutable_heads()`", e) - })?; let mut expression = RevsetExpressionEvaluator::new( repo, self.command.revset_extensions().clone(), &id_prefix_context, - immutable, + self.immutable_expression(), ); expression.intersect_with(&to_rewrite_revset); @@ -766,6 +782,7 @@ impl WorkspaceCommandEnvironment { &self.workspace_id, self.revset_parse_context(), id_prefix_context, + self.immutable_expression(), &self.command.data.commit_template_extensions, ) } diff --git a/cli/src/commands/git/push.rs b/cli/src/commands/git/push.rs index b1523cb82..19aaa92f9 100644 --- a/cli/src/commands/git/push.rs +++ b/cli/src/commands/git/push.rs @@ -51,7 +51,6 @@ use crate::formatter::Formatter; use crate::git_util::get_git_repo; use crate::git_util::with_remote_git_callbacks; use crate::git_util::GitSidebandProgressMessageWriter; -use crate::revset_util; use crate::ui::Ui; /// Push to a Git remote @@ -326,9 +325,7 @@ fn validate_commits_ready_to_push( .cloned() .collect_vec(); let commits_to_push = RevsetExpression::commits(old_heads) - .union(&revset_util::parse_immutable_heads_expression( - &tx.base_workspace_helper().revset_parse_context(), - )?) + .union(workspace_helper.env().immutable_heads_expression()) .range(&RevsetExpression::commits(new_heads)); let config = command.settings().config(); diff --git a/cli/src/commands/status.rs b/cli/src/commands/status.rs index a7c3ac47c..f413ce5d6 100644 --- a/cli/src/commands/status.rs +++ b/cli/src/commands/status.rs @@ -24,7 +24,6 @@ use crate::cli_util::CommandHelper; use crate::command_error::CommandError; use crate::diff_util::get_copy_records; use crate::diff_util::DiffFormat; -use crate::revset_util; use crate::ui::Ui; /// Show high-level repo status @@ -118,9 +117,7 @@ pub(crate) fn cmd_status( .parents() .ancestors() .filtered(RevsetFilterPredicate::HasConflict) - .minus(&revset_util::parse_immutable_expression( - &workspace_command.revset_parse_context(), - )?), + .minus(&workspace_command.env().immutable_expression()), ) .evaluate_to_commit_ids()? .collect(); diff --git a/cli/src/commit_templater.rs b/cli/src/commit_templater.rs index 4b4a7f86c..fea472dc2 100644 --- a/cli/src/commit_templater.rs +++ b/cli/src/commit_templater.rs @@ -91,6 +91,7 @@ pub struct CommitTemplateLanguage<'repo> { // are contained in RevsetParseContext for example. revset_parse_context: RevsetParseContext<'repo>, id_prefix_context: &'repo IdPrefixContext, + immutable_expression: Rc, build_fn_table: CommitTemplateBuildFnTable<'repo>, keyword_cache: CommitKeywordCache<'repo>, cache_extensions: ExtensionsMap, @@ -105,6 +106,7 @@ impl<'repo> CommitTemplateLanguage<'repo> { workspace_id: &WorkspaceId, revset_parse_context: RevsetParseContext<'repo>, id_prefix_context: &'repo IdPrefixContext, + immutable_expression: Rc, extensions: &[impl AsRef], ) -> Self { let mut build_fn_table = CommitTemplateBuildFnTable::builtin(); @@ -123,6 +125,7 @@ impl<'repo> CommitTemplateLanguage<'repo> { workspace_id: workspace_id.clone(), revset_parse_context, id_prefix_context, + immutable_expression, build_fn_table, keyword_cache: CommitKeywordCache::default(), cache_extensions, @@ -477,8 +480,12 @@ impl<'repo> CommitKeywordCache<'repo> { language: &CommitTemplateLanguage<'repo>, span: pest::Span<'_>, ) -> TemplateParseResult<&Rc>> { + // Alternatively, a negated (i.e. visible mutable) set could be computed. + // It's usually smaller than the immutable set. The revset engine can also + // optimize "::" query to use bitset-based implementation. self.is_immutable_fn.get_or_try_init(|| { - let revset = evaluate_immutable_revset(language, span)?; + let expression = language.immutable_expression.clone(); + let revset = evaluate_revset_expression(language, span, expression)?; Ok(revset.containing_fn().into()) }) } @@ -785,21 +792,6 @@ fn evaluate_revset_expression<'repo>( Ok(revset) } -fn evaluate_immutable_revset<'repo>( - language: &CommitTemplateLanguage<'repo>, - span: pest::Span<'_>, -) -> Result, TemplateParseError> { - // Alternatively, a negated (i.e. visible mutable) set could be computed. - // It's usually smaller than the immutable set. The revset engine can also - // optimize "::" query to use bitset-based implementation. - let expression = revset_util::parse_immutable_expression(&language.revset_parse_context) - .map_err(|err| { - TemplateParseError::expression("Failed to parse revset", span).with_source(err) - })?; - - evaluate_revset_expression(language, span, expression) -} - fn evaluate_user_revset<'repo>( language: &CommitTemplateLanguage<'repo>, span: pest::Span<'_>, diff --git a/cli/src/revset_util.rs b/cli/src/revset_util.rs index 0809a8808..fed2d3858 100644 --- a/cli/src/revset_util.rs +++ b/cli/src/revset_util.rs @@ -211,15 +211,6 @@ pub fn parse_immutable_heads_expression( Ok(heads.union(&RevsetExpression::root())) } -/// Parses user-configured expression defining the immutable set. -pub fn parse_immutable_expression( - context: &RevsetParseContext, -) -> Result, RevsetParseError> { - // Negated ancestors expression `~::( | root())` is slightly easier - // to optimize than negated union `~(:: | root())`. - Ok(parse_immutable_heads_expression(context)?.ancestors()) -} - pub(super) fn evaluate_revset_to_single_commit<'a>( revision_str: &str, expression: &RevsetExpressionEvaluator<'_>, diff --git a/cli/tests/test_commit_template.rs b/cli/tests/test_commit_template.rs index 852f975d2..987cb6e61 100644 --- a/cli/tests/test_commit_template.rs +++ b/cli/tests/test_commit_template.rs @@ -771,22 +771,16 @@ fn test_log_immutable() { test_env.add_config("revset-aliases.'immutable_heads()' = 'unknown_fn()'"); let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r::", "-T", template]); - insta::assert_snapshot!(stderr, @r###" - Error: Failed to parse template: Failed to parse revset - Caused by: - 1: --> 5:10 - | - 5 | if(immutable, "[immutable]"), - | ^-------^ - | - = Failed to parse revset - 2: --> 1:1 + insta::assert_snapshot!(stderr, @r#" + Config error: Invalid `revset-aliases.immutable_heads()` + Caused by: --> 1:1 | 1 | unknown_fn() | ^--------^ | = Function "unknown_fn" doesn't exist - "###); + For help, see https://martinvonz.github.io/jj/latest/config/. + "#); test_env.add_config("revset-aliases.'immutable_heads()' = 'unknown_symbol'"); let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r::", "-T", template]);