From a757fddcf1726ad73c42654cbcb6cca243ad8abc Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sat, 6 Jul 2024 19:50:04 +0900 Subject: [PATCH] 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 | 2 ++ cli/tests/test_revset_output.rs | 41 ++++++++++++++++++++++-------- docs/revsets.md | 7 +++-- lib/src/fileset.rs | 10 ++++++++ lib/src/fileset_parser.rs | 1 - lib/src/revset.rs | 45 +++++++++++++++++++++++++-------- lib/src/revset_parser.rs | 2 +- lib/tests/test_revset.rs | 9 +++++++ 8 files changed, 91 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b26e7da4f5..35af57788e 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/cli/tests/test_revset_output.rs b/cli/tests/test_revset_output.rs index 766f84f6ef..0c564343e9 100644 --- a/cli/tests/test_revset_output.rs +++ b/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 , , or "###); 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)"]); diff --git a/docs/revsets.md b/docs/revsets.md index 023f645ac5..56b7559277 100644 --- a/docs/revsets.md +++ b/docs/revsets.md @@ -268,8 +268,8 @@ 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. @@ -277,6 +277,9 @@ given [string pattern](#string-patterns). 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 diff --git a/lib/src/fileset.rs b/lib/src/fileset.rs index acffdbf943..07b5225836 100644 --- a/lib/src/fileset.rs +++ b/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 { + 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 diff --git a/lib/src/fileset_parser.rs b/lib/src/fileset_parser.rs index 68e7bf74bf..884c6d687e 100644 --- a/lib/src/fileset_parser.rs +++ b/lib/src/fileset_parser.rs @@ -305,7 +305,6 @@ fn parse_expression_node(pair: Pair) -> FilesetParseResult } /// 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 { let mut pairs = FilesetParser::parse(Rule::program, text)?; let first = pairs.next().unwrap(); diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 262e4aa08d..db4ba985f7 100644 --- a/lib/src/revset.rs +++ b/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> = 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> = 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 { - 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 { + // 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 { @@ -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( diff --git a/lib/src/revset_parser.rs b/lib/src/revset_parser.rs index 742f4b1bbb..234aa0eda3 100644 --- a/lib/src/revset_parser.rs +++ b/lib/src/revset_parser.rs @@ -804,7 +804,7 @@ pub fn expect_literal( /// Applies the give function to the innermost `node` by unwrapping alias /// expansion nodes. -fn expect_expression_with( +pub(super) fn expect_expression_with( node: &ExpressionNode, f: impl FnOnce(&ExpressionNode) -> Result, ) -> Result { diff --git a/lib/tests/test_revset.rs b/lib/tests/test_revset.rs index 75716ed1e7..13f3d3e210 100644 --- a/lib/tests/test_revset.rs +++ b/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,