cli: parse out redundant "all:" revset modifier

cli/src/cli_util.rs

@@ -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)
         })

cli/src/commands/log.rs

@@ -14,7 +14,7 @@
 
 use jj_lib::backend::CommitId;
 use jj_lib::repo::Repo;
-use jj_lib::revset::{self, RevsetExpression, RevsetFilterPredicate, RevsetIteratorExt};
+use jj_lib::revset::{RevsetExpression, RevsetFilterPredicate, RevsetIteratorExt};
 use jj_lib::revset_graph::{
     ReverseRevsetGraphIterator, RevsetGraphEdgeType, TopoGroupedRevsetGraphIterator,
 };
@@ -262,7 +262,9 @@ pub(crate) fn cmd_log(
                  working copy commit, pass -r '@' instead."
             )?;
         } else if revset.is_empty()
-            && revset::parse(only_path, &workspace_command.revset_parse_context()).is_ok()
+            && workspace_command
+                .parse_revset(&RevisionArg::from(only_path.to_owned()))
+                .is_ok()
         {
             writeln!(
                 ui.warning_default(),

cli/src/commands/squash.rs

@@ -18,7 +18,6 @@ use jj_lib::matchers::Matcher;
 use jj_lib::merged_tree::MergedTree;
 use jj_lib::object_id::ObjectId;
 use jj_lib::repo::Repo;
-use jj_lib::revset;
 use jj_lib::settings::UserSettings;
 use tracing::instrument;
 
@@ -234,11 +233,10 @@ from the source will be moved into the destination.
 
         if let [only_path] = path_arg {
             if no_rev_arg
-                && revset::parse(
-                    only_path,
-                    &tx.base_workspace_helper().revset_parse_context(),
-                )
-                .is_ok()
+                && tx
+                    .base_workspace_helper()
+                    .parse_revset(&RevisionArg::from(only_path.to_owned()))
+                    .is_ok()
             {
                 writeln!(
                     ui.warning_default(),

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<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)
 }

cli/tests/test_log_command.rs

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

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 [email protected] 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.
+    "###);
+}