revset: parse file() argument as fileset expression

Since fileset and revset languages are syntactically close, we can reparse
revset expression as a fileset. This might sound a bit scary, but helps
eliminate nested quoting like file("~glob:'*.rs'"). One oddity exists in alias
substitution, though. Another possible problem is that we'll need to add fake
operator parsing rules if we introduce incompatibility in fileset, or want to
embed revset expressions in a fileset.

Since "file(x, y)" is equivalent to "file(x|y)", the former will be deprecated.
I'll probably add a mechanism to collect warnings during parsing.

CHANGELOG.md

@@ -54,6 +54,8 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 * New `tracked_remote_branches()` and `untracked_remote_branches()` revset
   functions can be used to select tracked/untracked remote branches.
 
+* The `file()` revset function now accepts fileset as argument.
+
 ### Fixed bugs
 
 * `jj diff --git` no longer shows the contents of binary files.

cli/tests/test_revset_output.rs

@@ -140,42 +140,61 @@ fn test_bad_function_call() {
       = Function "file": Expected at least 1 arguments
     "###);
 
-    let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "file(a, not@a-string)"]);
+    let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "file(not::a-fileset)"]);
     insta::assert_snapshot!(stderr, @r###"
-    Error: Failed to parse revset: Expected expression of file pattern
-    Caused by:  --> 1:9
+    Error: Failed to parse revset: Invalid fileset expression
+    Caused by:
+    1:  --> 1:6
       |
-    1 | file(a, not@a-string)
-      |         ^----------^
+    1 | file(not::a-fileset)
+      |      ^------------^
       |
-      = Expected expression of file pattern
+      = Invalid fileset expression
+    2:  --> 1:5
+      |
+    1 | not::a-fileset
+      |     ^---
+      |
+      = expected <identifier>, <string_literal>, or <raw_string_literal>
     "###);
 
     let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", r#"file(foo:"bar")"#]);
     insta::assert_snapshot!(stderr, @r###"
-    Error: Failed to parse revset: Invalid file pattern
+    Error: Failed to parse revset: Invalid fileset expression
     Caused by:
     1:  --> 1:6
       |
     1 | file(foo:"bar")
       |      ^-------^
       |
+      = Invalid fileset expression
+    2:  --> 1:1
+      |
+    1 | foo:"bar"
+      | ^-------^
+      |
       = Invalid file pattern
-    2: Invalid file pattern kind "foo:"
+    3: Invalid file pattern kind "foo:"
     "###);
 
     let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", r#"file(a, "../out")"#]);
     insta::assert_snapshot!(stderr.replace('\\', "/"), @r###"
-    Error: Failed to parse revset: Invalid file pattern
+    Error: Failed to parse revset: Invalid fileset expression
     Caused by:
     1:  --> 1:9
       |
     1 | file(a, "../out")
       |         ^------^
       |
+      = Invalid fileset expression
+    2:  --> 1:1
+      |
+    1 | "../out"
+      | ^------^
+      |
       = Invalid file pattern
-    2: Path "../out" is not in the repo "."
-    3: Invalid component ".." in repo-relative path "../out"
+    3: Path "../out" is not in the repo "."
+    4: Invalid component ".." in repo-relative path "../out"
     "###);
 
     let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "branches(bad:pattern)"]);

docs/revsets.md

@@ -268,15 +268,18 @@ given [string pattern](#string-patterns).
 * `empty()`: Commits modifying no files. This also includes `merges()` without
   user modifications and `root()`.
 
-* `file(pattern[, pattern]...)`: Commits modifying paths matching one of the
-  given [file patterns](filesets.md#file-patterns).
+* `file(expression)`: Commits modifying paths matching the given [fileset
+  expression](filesets.md).
 
   Paths are relative to the directory `jj` was invoked from. A directory name
   will match all files in that directory and its subdirectories.
 
   For example, `file(foo)` will match files `foo`, `foo/bar`, `foo/bar/baz`.
   It will *not* match `foobar` or `bar/foo`.
 
+  Some file patterns might need quoting because the `expression` must also be
+  parsable as a revset. For example, `.` has to be quoted in `file(".")`.
+
 * `conflict()`: Commits with conflicts.
 
 * `present(x)`: Same as `x`, but evaluated to `none()` if any of the commits

lib/src/fileset.rs

@@ -446,6 +446,16 @@ fn resolve_expression(
     }
 }
 
+/// Parses text into `FilesetExpression` without bare string fallback.
+pub fn parse(
+    text: &str,
+    path_converter: &RepoPathUiConverter,
+) -> FilesetParseResult<FilesetExpression> {
+    let node = fileset_parser::parse_program(text)?;
+    // TODO: add basic tree substitution pass to eliminate redundant expressions
+    resolve_expression(path_converter, &node)
+}
+
 /// Parses text into `FilesetExpression` with bare string fallback.
 ///
 /// If the text can't be parsed as a fileset expression, and if it doesn't

lib/src/fileset_parser.rs

@@ -305,7 +305,6 @@ fn parse_expression_node(pair: Pair<Rule>) -> FilesetParseResult<ExpressionNode>
 }
 
 /// Parses text into expression tree. No name resolution is made at this stage.
-#[cfg(test)] // TODO: alias will be parsed with no bare_string fallback
 pub fn parse_program(text: &str) -> FilesetParseResult<ExpressionNode> {
     let mut pairs = FilesetParser::parse(Rule::program, text)?;
     let first = pairs.next().unwrap();

lib/src/revset.rs

@@ -29,7 +29,7 @@ use thiserror::Error;
 use crate::backend::{BackendError, BackendResult, ChangeId, CommitId};
 use crate::commit::Commit;
 use crate::dsl_util::{collect_similar, AliasExpandError as _};
-use crate::fileset::{FilePattern, FilesetExpression};
+use crate::fileset::FilesetExpression;
 use crate::graph::GraphEdge;
 use crate::hex_util::to_forward_hex;
 use crate::id_prefix::IdPrefixContext;
@@ -43,7 +43,7 @@ pub use crate::revset_parser::{
 };
 use crate::store::Store;
 use crate::str_util::StringPattern;
-use crate::{dsl_util, revset_parser};
+use crate::{dsl_util, fileset, revset_parser};
 
 /// Error occurred during symbol resolution.
 #[derive(Debug, Error)]
@@ -706,10 +706,10 @@ static BUILTIN_FUNCTION_MAP: Lazy<HashMap<&'static str, RevsetFunction>> = Lazy:
                 function.args_span, // TODO: better to use name_span?
             )
         })?;
+        // TODO: emit deprecation warning if multiple arguments are passed
         let ([arg], args) = function.expect_some_arguments()?;
         let file_expressions = itertools::chain([arg], args)
-            .map(|arg| expect_file_pattern(arg, ctx.path_converter))
-            .map_ok(FilesetExpression::pattern)
+            .map(|arg| expect_fileset_expression(arg, ctx.path_converter))
             .try_collect()?;
         let expr = FilesetExpression::union_all(file_expressions);
         Ok(RevsetExpression::filter(RevsetFilterPredicate::File(expr)))
@@ -726,15 +726,19 @@ static BUILTIN_FUNCTION_MAP: Lazy<HashMap<&'static str, RevsetFunction>> = Lazy:
     map
 });
 
-pub fn expect_file_pattern(
+/// Parses the given `node` as a fileset expression.
+pub fn expect_fileset_expression(
     node: &ExpressionNode,
     path_converter: &RepoPathUiConverter,
-) -> Result<FilePattern, RevsetParseError> {
-    let parse_pattern = |value: &str, kind: Option<&str>| match kind {
-        Some(kind) => FilePattern::from_str_kind(path_converter, value, kind),
-        None => FilePattern::cwd_prefix_path(path_converter, value),
-    };
-    revset_parser::expect_pattern_with("file pattern", node, parse_pattern)
+) -> Result<FilesetExpression, RevsetParseError> {
+    // Alias handling is a bit tricky. The outermost expression `alias` is
+    // substituted, but inner expressions `x & alias` aren't. If this seemed
+    // weird, we can either transform AST or turn off revset aliases completely.
+    revset_parser::expect_expression_with(node, |node| {
+        fileset::parse(node.span.as_str(), path_converter).map_err(|err| {
+            RevsetParseError::expression("Invalid fileset expression", node.span).with_source(err)
+        })
+    })
 }
 
 pub fn expect_string_pattern(node: &ExpressionNode) -> Result<StringPattern, RevsetParseError> {
@@ -2567,9 +2571,28 @@ mod tests {
         insta::assert_debug_snapshot!(
             parse_with_workspace("file(foo)", &WorkspaceId::default()).unwrap(),
             @r###"Filter(File(Pattern(PrefixPath("foo"))))"###);
+        insta::assert_debug_snapshot!(
+            parse_with_workspace("file(all())", &WorkspaceId::default()).unwrap(),
+            @"Filter(File(All))");
         insta::assert_debug_snapshot!(
             parse_with_workspace(r#"file(file:"foo")"#, &WorkspaceId::default()).unwrap(),
             @r###"Filter(File(Pattern(FilePath("foo"))))"###);
+        insta::assert_debug_snapshot!(
+            parse_with_workspace("file(foo|bar&baz)", &WorkspaceId::default()).unwrap(), @r###"
+        Filter(
+            File(
+                UnionAll(
+                    [
+                        Pattern(PrefixPath("foo")),
+                        Intersection(
+                            Pattern(PrefixPath("bar")),
+                            Pattern(PrefixPath("baz")),
+                        ),
+                    ],
+                ),
+            ),
+        )
+        "###);
         insta::assert_debug_snapshot!(
             parse_with_workspace("file(foo, bar, baz)", &WorkspaceId::default()).unwrap(), @r###"
         Filter(

lib/src/revset_parser.rs

@@ -804,7 +804,7 @@ pub fn expect_literal<T: FromStr>(
 
 /// Applies the give function to the innermost `node` by unwrapping alias
 /// expansion nodes.
-fn expect_expression_with<T>(
+pub(super) fn expect_expression_with<T>(
     node: &ExpressionNode,
     f: impl FnOnce(&ExpressionNode) -> Result<T, RevsetParseError>,
 ) -> Result<T, RevsetParseError> {

lib/tests/test_revset.rs

@@ -2986,6 +2986,15 @@ fn test_evaluate_expression_file() {
         ),
         vec![commit1.id().clone()]
     );
+    assert_eq!(
+        resolve_commit_ids_in_workspace(
+            mut_repo,
+            r#"file("added_clean_clean"|"added_modified_clean")"#,
+            &test_workspace.workspace,
+            Some(test_workspace.workspace.workspace_root()),
+        ),
+        vec![commit2.id().clone(), commit1.id().clone()]
+    );
     assert_eq!(
         resolve_commit_ids_in_workspace(
             mut_repo,