From 3307696ba855163a7a20cfb03d7b5bb00823ea05 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 10 May 2024 09:25:33 +0900 Subject: [PATCH] cli: parse out redundant "all:" revset modifier in arguments and templates This also means "all:" is allowed in default revsets (such as "revsets.log"), but that seems okay. In revset aliases, "all:" isn't allowed because aliases may be expanded to sub-expression position. Closes #3654 --- cli/src/cli_util.rs | 24 ++++--- cli/src/commit_templater.rs | 10 +-- cli/tests/test_log_command.rs | 2 +- cli/tests/test_revset_output.rs | 117 ++++++++++++++++++++++++++++---- 4 files changed, 126 insertions(+), 27 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index d32750ce9..133a889ea 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -921,12 +921,13 @@ impl WorkspaceCommandHelper { &self, revision_arg: &RevisionArg, ) -> Result, CommandError> { - let expression = revset::parse(revision_arg.as_ref(), &self.revset_parse_context())?; - self.attach_revset_evaluator(expression) + let (expression, modifier) = self.parse_revset_with_modifier(revision_arg)?; + // Whether the caller accepts multiple revisions or not, "all:" should + // be valid. For example, "all:@" is a valid single-rev expression. + let (None | Some(RevsetModifier::All)) = modifier; + Ok(expression) } - // TODO: maybe better to parse all: prefix even if it is the default? It - // shouldn't be allowed in aliases, though. fn parse_revset_with_modifier( &self, revision_arg: &RevisionArg, @@ -944,7 +945,8 @@ impl WorkspaceCommandHelper { let context = self.revset_parse_context(); let expressions: Vec<_> = revision_args .iter() - .map(|arg| revset::parse(arg.as_ref(), &context)) + .map(|arg| revset::parse_with_modifier(arg.as_ref(), &context)) + .map_ok(|(expression, None | Some(RevsetModifier::All))| expression) .try_collect()?; let expression = RevsetExpression::union_all(&expressions); self.attach_revset_evaluator(expression) @@ -985,11 +987,13 @@ impl WorkspaceCommandHelper { .get_string("revsets.short-prefixes") .unwrap_or_else(|_| self.settings.default_revset()); if !revset_string.is_empty() { - let disambiguation_revset = - revset::parse(&revset_string, &self.revset_parse_context()).map_err(|err| { - config_error_with_message("Invalid `revsets.short-prefixes`", err) - })?; - context = context.disambiguate_within(revset::optimize(disambiguation_revset)); + let (expression, modifier) = + revset::parse_with_modifier(&revset_string, &self.revset_parse_context()) + .map_err(|err| { + config_error_with_message("Invalid `revsets.short-prefixes`", err) + })?; + let (None | Some(RevsetModifier::All)) = modifier; + context = context.disambiguate_within(revset::optimize(expression)); } Ok(context) }) diff --git a/cli/src/commit_templater.rs b/cli/src/commit_templater.rs index 117d4a614..a6bd3ef0e 100644 --- a/cli/src/commit_templater.rs +++ b/cli/src/commit_templater.rs @@ -28,7 +28,7 @@ use jj_lib::id_prefix::IdPrefixContext; use jj_lib::object_id::ObjectId as _; use jj_lib::op_store::{RefTarget, RemoteRef, WorkspaceId}; use jj_lib::repo::Repo; -use jj_lib::revset::{self, Revset, RevsetExpression, RevsetParseContext}; +use jj_lib::revset::{self, Revset, RevsetExpression, RevsetModifier, RevsetParseContext}; use once_cell::unsync::OnceCell; use crate::template_builder::{ @@ -713,9 +713,11 @@ fn evaluate_user_revset<'repo>( span: pest::Span<'_>, revset: &str, ) -> Result, TemplateParseError> { - let expression = revset::parse(revset, &language.revset_parse_context).map_err(|err| { - TemplateParseError::expression("Failed to parse revset", span).with_source(err) - })?; + let (expression, modifier) = + revset::parse_with_modifier(revset, &language.revset_parse_context).map_err(|err| { + TemplateParseError::expression("Failed to parse revset", span).with_source(err) + })?; + let (None | Some(RevsetModifier::All)) = modifier; evaluate_revset_expression(language, span, expression) } diff --git a/cli/tests/test_log_command.rs b/cli/tests/test_log_command.rs index 176e7e43b..ae96eec6e 100644 --- a/cli/tests/test_log_command.rs +++ b/cli/tests/test_log_command.rs @@ -454,7 +454,7 @@ fn test_log_bad_short_prefixes() { 1 | !nval!d | ^--- | - = expected + = expected or For help, see https://github.com/martinvonz/jj/blob/main/docs/config.md. "###); } diff --git a/cli/tests/test_revset_output.rs b/cli/tests/test_revset_output.rs index 7bf42054d..29a4f8811 100644 --- a/cli/tests/test_revset_output.rs +++ b/cli/tests/test_revset_output.rs @@ -66,18 +66,6 @@ fn test_syntax_error() { = '^' is not a postfix operator Hint: Did you mean '-' for parents? "###); - - // "jj new" supports "all:" prefix - let stderr = test_env.jj_cmd_failure(&repo_path, &["new", "ale:x"]); - insta::assert_snapshot!(stderr, @r###" - Error: Failed to parse revset: Modifier "ale" doesn't exist - Caused by: --> 1:1 - | - 1 | ale:x - | ^-^ - | - = Modifier "ale" doesn't exist - "###); } #[test] @@ -496,3 +484,108 @@ fn test_bad_alias_decl() { = Redefinition of function parameter "###); } + +#[test] +fn test_all_modifier() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + let repo_path = test_env.env_root().join("repo"); + + // Command that accepts single revision by default + let stderr = test_env.jj_cmd_failure(&repo_path, &["new", "all()"]); + insta::assert_snapshot!(stderr, @r###" + Error: Revset "all()" resolved to more than one revision + Hint: The revset "all()" resolved to these revisions: + qpvuntsm 230dd059 (empty) (no description set) + zzzzzzzz 00000000 (empty) (no description set) + Hint: Prefix the expression with 'all:' to allow any number of revisions (i.e. 'all:all()'). + "###); + let stderr = test_env.jj_cmd_failure(&repo_path, &["new", "all:all()"]); + insta::assert_snapshot!(stderr, @r###" + Error: The Git backend does not support creating merge commits with the root commit as one of the parents. + "###); + + // Command that accepts multiple revisions by default + let stdout = test_env.jj_cmd_success(&repo_path, &["log", "-rall:all()"]); + insta::assert_snapshot!(stdout, @r###" + @ qpvuntsm test.user@example.com 2001-02-03 08:05:07 230dd059 + │ (empty) (no description set) + ◉ zzzzzzzz root() 00000000 + "###); + + // Command that accepts only single revision + let (_stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["branch", "create", "-rall:@", "x"]); + insta::assert_snapshot!(stderr, @""); + let stderr = test_env.jj_cmd_failure(&repo_path, &["branch", "set", "-rall:all()", "x"]); + insta::assert_snapshot!(stderr, @r###" + Error: Revset "all:all()" resolved to more than one revision + Hint: The revset "all:all()" resolved to these revisions: + qpvuntsm 230dd059 x | (empty) (no description set) + zzzzzzzz 00000000 (empty) (no description set) + "###); + + // Template expression that accepts multiple revisions by default + let stdout = test_env.jj_cmd_success(&repo_path, &["log", "-Tself.contained_in('all:all()')"]); + insta::assert_snapshot!(stdout, @r###" + @ true + ◉ true + "###); + + // Typo + let stderr = test_env.jj_cmd_failure(&repo_path, &["new", "ale:x"]); + insta::assert_snapshot!(stderr, @r###" + Error: Failed to parse revset: Modifier "ale" doesn't exist + Caused by: --> 1:1 + | + 1 | ale:x + | ^-^ + | + = Modifier "ale" doesn't exist + "###); + + // Modifier shouldn't be allowed in aliases (This restriction might be + // lifted later. "all:" in alias could be allowed if it is expanded to the + // top-level expression.) + let stderr = test_env.jj_cmd_failure( + &repo_path, + &["new", "x", "--config-toml=revset-aliases.x='all:@'"], + ); + insta::assert_snapshot!(stderr, @r###" + Error: Failed to parse revset: Alias "x" cannot be expanded + Caused by: + 1: --> 1:1 + | + 1 | x + | ^ + | + = Alias "x" cannot be expanded + 2: --> 1:4 + | + 1 | all:@ + | ^ + | + = ':' is not an infix operator + Hint: Did you mean '::' for DAG range? + "###); + + // immutable_heads() alias may be parsed as a top-level expression, but + // still, modifier shouldn't be allowed there. + let stderr = test_env.jj_cmd_failure( + &repo_path, + &[ + "new", + "--config-toml=revset-aliases.'immutable_heads()'='all:@'", + "--config-toml=revsets.short-prefixes='none()'", + ], + ); + insta::assert_snapshot!(stderr, @r###" + Config error: Invalid `revset-aliases.immutable_heads()` + Caused by: --> 1:4 + | + 1 | all:@ + | ^ + | + = ':' is not an infix operator + For help, see https://github.com/martinvonz/jj/blob/main/docs/config.md. + "###); +}