Rename conflict revset to conflicts.

See discussion thread in linked issue. With this PR, all revset functions in [BUILTIN_FUNCTION_MAP](https://github.com/martinvonz/jj/blob/8d166c764251042287626ab37a1148386cef8371/lib/src/revset.rs#L570) that return multiple values are either named in plural or named in a difficult-to-misused manner (e.g. `reachable`) Fixes: #4122
jj-vcs · Sep 24, 2024 · 1173f05 · 1173f05
1 parent e8deb08
commit 1173f05
Show file tree

Hide file tree

Showing 6 changed files with 66 additions and 42 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -96,6 +96,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
   commit to commit. It now either follows the flags `--edit|--no-edit` or it
   gets the mode from `ui.movement.edit`.
 
+* `conflict` revset has been renamed to `conflicts`. With this, all revsets that
+  could potentially return multiple results are named in the plural.
+
 ### Deprecations
 
 * `jj untrack` has been renamed to `jj file untrack`.

diff --git a/cli/tests/test_log_command.rs b/cli/tests/test_log_command.rs
@@ -873,15 +873,15 @@ fn test_log_filtered_by_path() {
     ~
     "###);
 
-    // file() revset doesn't filter the diff.
+    // files() revset doesn't filter the diff.
     let stdout = test_env.jj_cmd_success(
         &repo_path,
         &[
             "log",
             "-T",
             "description",
             "-s",
-            "-rfile(file2)",
+            "-rfiles(file2)",
             "--no-graph",
         ],
     );

diff --git a/cli/tests/test_revset_output.rs b/cli/tests/test_revset_output.rs
@@ -129,25 +129,25 @@ fn test_bad_function_call() {
       = Expected expression of type integer
     "###);
 
-    let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "file()"]);
+    let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "files()"]);
     insta::assert_snapshot!(stderr, @r###"
-    Error: Failed to parse revset: Function "file": Expected at least 1 arguments
-    Caused by:  --> 1:6
+    Error: Failed to parse revset: Function "files": Expected at least 1 arguments
+    Caused by:  --> 1:7
       |
-    1 | file()
-      |      ^
+    1 | files()
+      |       ^
       |
-      = Function "file": Expected at least 1 arguments
+      = Function "files": Expected at least 1 arguments
     "###);
 
-    let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "file(not::a-fileset)"]);
-    insta::assert_snapshot!(stderr, @r#"
+    let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "files(not::a-fileset)"]);
+    insta::assert_snapshot!(stderr, @r###"
     Error: Failed to parse revset: In fileset expression
     Caused by:
-    1:  --> 1:6
+    1:  --> 1:7
       |
-    1 | file(not::a-fileset)
-      |      ^------------^
+    1 | files(not::a-fileset)
+      |       ^------------^
       |
       = In fileset expression
     2:  --> 1:5
@@ -156,16 +156,16 @@ fn test_bad_function_call() {
       |     ^---
       |
       = 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#"
+    let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", r#"files(foo:"bar")"#]);
+    insta::assert_snapshot!(stderr, @r###"
     Error: Failed to parse revset: In fileset expression
     Caused by:
-    1:  --> 1:6
+    1:  --> 1:7
       |
-    1 | file(foo:"bar")
-      |      ^-------^
+    1 | files(foo:"bar")
+      |       ^-------^
       |
       = In fileset expression
     2:  --> 1:1
@@ -175,16 +175,16 @@ fn test_bad_function_call() {
       |
       = Invalid file pattern
     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#"
+    let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", r#"files(a, "../out")"#]);
+    insta::assert_snapshot!(stderr.replace('\\', "/"), @r###"
     Error: Failed to parse revset: In fileset expression
     Caused by:
-    1:  --> 1:9
+    1:  --> 1:10
       |
-    1 | file(a, "../out")
-      |         ^------^
+    1 | files(a, "../out")
+      |          ^------^
       |
       = In fileset expression
     2:  --> 1:1
@@ -195,7 +195,7 @@ fn test_bad_function_call() {
       = Invalid file pattern
     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", "bookmarks(bad:pattern)"]);
     insta::assert_snapshot!(stderr, @r###"
@@ -334,17 +334,17 @@ fn test_parse_warning() {
       = untracked_remote_branches() is deprecated; use untracked_remote_bookmarks() instead
     "#);
 
-    let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["log", "-r", "file(foo, bar)"]);
+    let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["log", "-r", "files(foo, bar)"]);
     insta::assert_snapshot!(stdout, @"");
-    insta::assert_snapshot!(stderr, @r#"
+    insta::assert_snapshot!(stderr, @r###"
     Warning: In revset expression
-     --> 1:6
+     --> 1:7
       |
-    1 | file(foo, bar)
-      |      ^------^
+    1 | files(foo, bar)
+      |       ^------^
       |
       = Multi-argument patterns syntax is deprecated; separate them with |
-    "#);
+    "###);
 }
 
 #[test]

diff --git a/docs/revsets.md b/docs/revsets.md
@@ -274,7 +274,7 @@ given [string pattern](#string-patterns).
 * `empty()`: Commits modifying no files. This also includes `merges()` without
   user modifications and `root()`.
 
-* `file(expression)`: Commits modifying paths matching the given [fileset
+* `files(expression)`: Commits modifying paths matching the given [fileset
   expression](filesets.md).
 
   Paths are relative to the directory `jj` was invoked from. A directory name
@@ -296,7 +296,7 @@ given [string pattern](#string-patterns).
   For example, `diff_contains("TODO", "src")` will search revisions where "TODO"
   is added to or removed from files under "src".
 
-* `conflict()`: Commits with conflicts.
+* `conflicts()`: Commits with conflicts.
 
 * `present(x)`: Same as `x`, but evaluated to `none()` if any of the commits
   in `x` doesn't exist (e.g. is an unknown bookmark name.)

diff --git a/lib/src/revset.rs b/lib/src/revset.rs
@@ -789,7 +789,14 @@ static BUILTIN_FUNCTION_MAP: Lazy<HashMap<&'static str, RevsetFunction>> = Lazy:
         function.expect_no_arguments()?;
         Ok(RevsetExpression::is_empty())
     });
-    map.insert("file", |diagnostics, function, context| {
+    map.insert("files", |diagnostics, function, context| {
+        // TODO: Remove in jj 0.28+
+        if function.name != "files" {
+            diagnostics.add_warning(RevsetParseError::expression(
+                "file() is deprecated; use files() instead",
+                function.name_span,
+            ));
+        }
         let ctx = context.workspace.as_ref().ok_or_else(|| {
             RevsetParseError::with_span(
                 RevsetParseErrorKind::FsPathWithoutWorkspace,
@@ -810,6 +817,8 @@ static BUILTIN_FUNCTION_MAP: Lazy<HashMap<&'static str, RevsetFunction>> = Lazy:
         let expr = FilesetExpression::union_all(file_expressions);
         Ok(RevsetExpression::filter(RevsetFilterPredicate::File(expr)))
     });
+    // TODO: Remove in jj 0.28+
+    map.insert("file", map["files"]);
     map.insert("diff_contains", |diagnostics, function, context| {
         let ([text_arg], [files_opt_arg]) = function.expect_arguments()?;
         let text = expect_string_pattern(diagnostics, text_arg)?;
@@ -830,10 +839,19 @@ static BUILTIN_FUNCTION_MAP: Lazy<HashMap<&'static str, RevsetFunction>> = Lazy:
             RevsetFilterPredicate::DiffContains { text, files },
         ))
     });
-    map.insert("conflict", |_diagnostics, function, _context| {
+    map.insert("conflicts", |diagnostics, function, _context| {
+        // TODO: Remove in jj 0.28+
+        if function.name != "conflicts" {
+            diagnostics.add_warning(RevsetParseError::expression(
+                "file() is deprecated; use files() instead",
+                function.name_span,
+            ));
+        }
         function.expect_no_arguments()?;
         Ok(RevsetExpression::filter(RevsetFilterPredicate::HasConflict))
     });
+    // TODO: Remove in jj 0.28+
+    map.insert("conflict", map["conflicts"]);
     map.insert("present", |diagnostics, function, context| {
         let [arg] = function.expect_exact_arguments()?;
         let expression = lower_expression(diagnostics, arg, context)?;

diff --git a/lib/tests/test_revset.rs b/lib/tests/test_revset.rs
@@ -3171,7 +3171,7 @@ fn test_evaluate_expression_file() {
     assert_eq!(
         resolve_commit_ids_in_workspace(
             mut_repo,
-            r#"file("repo/added_clean_clean")"#,
+            r#"files("repo/added_clean_clean")"#,
             &test_workspace.workspace,
             Some(test_workspace.workspace.workspace_root().parent().unwrap()),
         ),
@@ -3180,7 +3180,7 @@ fn test_evaluate_expression_file() {
     assert_eq!(
         resolve_commit_ids_in_workspace(
             mut_repo,
-            r#"file("added_clean_clean"|"added_modified_clean")"#,
+            r#"files("added_clean_clean"|"added_modified_clean")"#,
             &test_workspace.workspace,
             Some(test_workspace.workspace.workspace_root()),
         ),
@@ -3189,7 +3189,10 @@ fn test_evaluate_expression_file() {
     assert_eq!(
         resolve_commit_ids_in_workspace(
             mut_repo,
-            &format!(r#"{}:: & file("added_modified_clean")"#, commit2.id().hex()),
+            &format!(
+                r#"{}:: & files("added_modified_clean")"#,
+                commit2.id().hex()
+            ),
             &test_workspace.workspace,
             Some(test_workspace.workspace.workspace_root()),
         ),
@@ -3387,7 +3390,7 @@ fn test_evaluate_expression_file_merged_parents() {
     };
 
     assert_eq!(
-        query("file('file1')"),
+        query("files('file1')"),
         vec![
             commit4.id().clone(),
             commit3.id().clone(),
@@ -3396,7 +3399,7 @@ fn test_evaluate_expression_file_merged_parents() {
         ]
     );
     assert_eq!(
-        query("file('file2')"),
+        query("files('file2')"),
         vec![
             commit3.id().clone(),
             commit2.id().clone(),
@@ -3453,7 +3456,7 @@ fn test_evaluate_expression_conflict() {
 
     // Only commit4 has a conflict
     assert_eq!(
-        resolve_commit_ids(mut_repo, "conflict()"),
+        resolve_commit_ids(mut_repo, "conflicts()"),
         vec![commit4.id().clone()]
     );
 }