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
This commit is contained in:
Yuya Nishihara 2024-05-10 09:25:33 +09:00
parent ba73accac6
commit 3307696ba8
4 changed files with 126 additions and 27 deletions

View file

@ -921,12 +921,13 @@ impl WorkspaceCommandHelper {
&self,
revision_arg: &RevisionArg,
) -> Result<RevsetExpressionEvaluator<'_>, 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)
})

View file

@ -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<Box<dyn Revset + 'repo>, 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)
}

View file

@ -454,7 +454,7 @@ fn test_log_bad_short_prefixes() {
1 | !nval!d
| ^---
|
= expected <expression>
= expected <identifier> or <expression>
For help, see https://github.com/martinvonz/jj/blob/main/docs/config.md.
"###);
}

View file

@ -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.
"###);
}