revset: parse file() argument as fileset expression

CHANGELOG.md

@@ -49,6 +49,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
 
 ## [0.19.0] - 2024-07-03

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)]
@@ -700,20 +700,19 @@ static BUILTIN_FUNCTION_MAP: Lazy<HashMap<&'static str, RevsetFunction>> = Lazy:
         Ok(RevsetExpression::is_empty())
     });
     map.insert("file", |function, context| {
-        if let Some(ctx) = &context.workspace {
-            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)
-                .try_collect()?;
-            let expr = FilesetExpression::union_all(file_expressions);
-            Ok(RevsetExpression::filter(RevsetFilterPredicate::File(expr)))
-        } else {
-            Err(RevsetParseError::with_span(
+        let ctx = context.workspace.as_ref().ok_or_else(|| {
+            RevsetParseError::with_span(
                 RevsetParseErrorKind::FsPathWithoutWorkspace,
                 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_fileset_expression(arg, ctx.path_converter))
+            .try_collect()?;
+        let expr = FilesetExpression::union_all(file_expressions);
+        Ok(RevsetExpression::filter(RevsetFilterPredicate::File(expr)))
     });
     map.insert("conflict", |function, _context| {
         function.expect_no_arguments()?;
@@ -727,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> {
@@ -2568,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,