cli: migrate "chmod" to matcher API, warn unmatched "diff" paths

cli/src/cli_util.rs

@@ -1722,6 +1722,32 @@ Discard the conflicting changes with `jj restore --from {}`.",
     Ok(())
 }
 
+/// Prints warning about explicit paths that don't match any of the tree
+/// entries.
+pub fn print_unmatched_explicit_paths<'a>(
+    ui: &Ui,
+    workspace_command: &WorkspaceCommandHelper,
+    expression: &FilesetExpression,
+    trees: impl IntoIterator<Item = &'a MergedTree>,
+) -> io::Result<()> {
+    let mut explicit_paths = expression.explicit_paths().collect_vec();
+    for tree in trees {
+        explicit_paths.retain(|&path| tree.path_value(path).is_absent());
+        if explicit_paths.is_empty() {
+            return Ok(());
+        }
+    }
+    let ui_paths = explicit_paths
+        .iter()
+        .map(|&path| workspace_command.format_file_path(path))
+        .join(", ");
+    writeln!(
+        ui.warning_default(),
+        "No matching entries for paths: {ui_paths}"
+    )?;
+    Ok(())
+}
+
 pub fn print_trackable_remote_branches(ui: &Ui, view: &View) -> io::Result<()> {
     let remote_branch_names = view
         .branches()

cli/src/commands/chmod.rs

@@ -12,13 +12,12 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use itertools::Itertools;
 use jj_lib::backend::TreeValue;
 use jj_lib::merged_tree::MergedTreeBuilder;
 use jj_lib::object_id::ObjectId;
 use tracing::instrument;
 
-use crate::cli_util::{CommandHelper, RevisionArg};
+use crate::cli_util::{print_unmatched_explicit_paths, CommandHelper, RevisionArg};
 use crate::command_error::{user_error, CommandError};
 use crate::ui::Ui;
 
@@ -60,30 +59,25 @@ pub(crate) fn cmd_chmod(
     };
 
     let mut workspace_command = command.workspace_helper(ui)?;
-    // TODO: migrate to .parse_file_patterns()?.to_matcher()
-    let repo_paths: Vec<_> = args
-        .paths
-        .iter()
-        .map(|path| workspace_command.parse_file_path(path))
-        .try_collect()?;
     let commit = workspace_command.resolve_single_rev(&args.revision)?;
     workspace_command.check_rewritable([commit.id()])?;
+    let tree = commit.tree()?;
+    // TODO: No need to add special case for empty paths when switching to
+    // parse_union_filesets(). paths = [] should be "none()" if supported.
+    let fileset_expression = workspace_command.parse_file_patterns(&args.paths)?;
+    let matcher = fileset_expression.to_matcher();
+    print_unmatched_explicit_paths(ui, &workspace_command, &fileset_expression, [&tree])?;
 
     let mut tx = workspace_command.start_transaction();
-    let tree = commit.tree()?;
     let store = tree.store();
     let mut tree_builder = MergedTreeBuilder::new(commit.tree_id().clone());
-    for repo_path in repo_paths {
+    for (repo_path, tree_value) in tree.entries_matching(matcher.as_ref()) {
         let user_error_with_path = |msg: &str| {
             user_error(format!(
                 "{msg} at '{}'.",
                 tx.base_workspace_helper().format_file_path(&repo_path)
             ))
         };
-        let tree_value = tree.path_value(&repo_path);
-        if tree_value.is_absent() {
-            return Err(user_error_with_path("No such path"));
-        }
         let all_files = tree_value
             .adds()
             .flatten()

cli/src/commands/diff.rs

@@ -15,7 +15,7 @@
 use jj_lib::rewrite::merge_commit_trees;
 use tracing::instrument;
 
-use crate::cli_util::{CommandHelper, RevisionArg};
+use crate::cli_util::{print_unmatched_explicit_paths, CommandHelper, RevisionArg};
 use crate::command_error::CommandError;
 use crate::diff_util::{diff_formats_for, show_diff, DiffFormatArgs};
 use crate::ui::Ui;
@@ -76,9 +76,8 @@ pub(crate) fn cmd_diff(
         from_tree = merge_commit_trees(workspace_command.repo().as_ref(), &parents)?;
         to_tree = commit.tree()?
     }
-    let matcher = workspace_command
-        .parse_file_patterns(&args.paths)?
-        .to_matcher();
+    let fileset_expression = workspace_command.parse_file_patterns(&args.paths)?;
+    let matcher = fileset_expression.to_matcher();
     let diff_formats = diff_formats_for(command.settings(), &args.format)?;
     ui.request_pager();
     show_diff(
@@ -90,5 +89,11 @@ pub(crate) fn cmd_diff(
         matcher.as_ref(),
         &diff_formats,
     )?;
+    print_unmatched_explicit_paths(
+        ui,
+        &workspace_command,
+        &fileset_expression,
+        [&from_tree, &to_tree],
+    )?;
     Ok(())
 }

cli/tests/test_chmod_command.rs

@@ -116,27 +116,16 @@ fn test_chmod_regular_conflict() {
     >>>>>>>
     "###);
 
-    // An error prevents `chmod` from making any changes.
-    // In this case, the failure with `nonexistent` prevents any changes to `file`.
-    let stderr = test_env.jj_cmd_failure(&repo_path, &["chmod", "x", "nonexistent", "file"]);
+    // Unmatched paths should generate warnings
+    let (_stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["chmod", "x", "nonexistent", "file"]);
     insta::assert_snapshot!(stderr, @r###"
-    Error: No such path at 'nonexistent'.
-    "###);
-    let stdout = test_env.jj_cmd_success(&repo_path, &["debug", "tree"]);
-    insta::assert_snapshot!(stdout, 
-    @r###"
-    file: Conflicted([Some(File { id: FileId("587be6b4c3f93f93c489c0111bba5596147a26cb"), executable: false }), Some(File { id: FileId("df967b96a579e45a18b8251732d16804b2e56a55"), executable: false }), Some(File { id: FileId("8ba3a16384aacc37d01564b28401755ce8053f51"), executable: false })])
-    "###);
-    let stdout = test_env.jj_cmd_success(&repo_path, &["cat", "file"]);
-    insta::assert_snapshot!(stdout, 
-    @r###"
-    <<<<<<<
-    %%%%%%%
-    -base
-    +x
-    +++++++
-    n
-    >>>>>>>
+    Warning: No matching entries for paths: nonexistent
+    Working copy now at: yostqsxw cbc43289 conflict | (conflict) conflict
+    Parent commit      : royxmykx 427fbd2f x | x
+    Parent commit      : zsuskuln 3f83a26d n | n
+    Added 0 files, modified 1 files, removed 0 files
+    There are unresolved conflicts at these paths:
+    file    2-sided conflict including an executable
     "###);
 }
 

cli/tests/test_diff_command.rs

@@ -149,6 +149,33 @@ fn test_diff_basic() {
     file3 | 1 +
     3 files changed, 2 insertions(+), 1 deletion(-)
     "###);
+
+    // Unmatched paths should generate warnings
+    let (stdout, stderr) = test_env.jj_cmd_ok(
+        test_env.env_root(),
+        &[
+            "diff",
+            "-Rrepo",
+            "-s",
+            "repo",       // matches directory
+            "repo/file1", // deleted in to_tree, but exists in from_tree
+            "repo/x",
+            "repo/y/z",
+        ],
+    );
+    insta::assert_snapshot!(stdout.replace('\\', "/"), @r###"
+    D repo/file1
+    M repo/file2
+    A repo/file3
+    "###);
+    insta::assert_snapshot!(stderr.replace('\\', "/"), @r###"
+    Warning: No matching entries for paths: repo/x, repo/y/z
+    "###);
+
+    // Unmodified paths shouldn't generate warnings
+    let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["diff", "-s", "--from=@", "file2"]);
+    insta::assert_snapshot!(stdout, @"");
+    insta::assert_snapshot!(stderr, @"");
 }
 
 #[test]

lib/src/fileset.rs

@@ -16,7 +16,7 @@
 
 use std::collections::HashMap;
 use std::path::Path;
-use std::slice;
+use std::{iter, slice};
 
 use once_cell::sync::Lazy;
 use thiserror::Error;
@@ -30,7 +30,7 @@ use crate::matchers::{
     DifferenceMatcher, EverythingMatcher, FilesMatcher, IntersectionMatcher, Matcher,
     NothingMatcher, PrefixMatcher, UnionMatcher,
 };
-use crate::repo_path::{FsPathParseError, RelativePathParseError, RepoPathBuf};
+use crate::repo_path::{FsPathParseError, RelativePathParseError, RepoPath, RepoPathBuf};
 
 /// Error occurred during file pattern parsing.
 #[derive(Debug, Error)]
@@ -129,6 +129,15 @@ impl FilePattern {
         let path = RepoPathBuf::from_relative_path(input)?;
         Ok(FilePattern::PrefixPath(path))
     }
+
+    /// Returns path if this pattern represents a literal path in a workspace.
+    /// Returns `None` if this is a glob pattern for example.
+    pub fn as_path(&self) -> Option<&RepoPath> {
+        match self {
+            FilePattern::FilePath(path) => Some(path),
+            FilePattern::PrefixPath(path) => Some(path),
+        }
+    }
 }
 
 /// AST-level representation of the fileset expression.
@@ -217,6 +226,38 @@ impl FilesetExpression {
         }
     }
 
+    fn dfs_pre(&self) -> impl Iterator<Item = &Self> {
+        let mut stack: Vec<&Self> = vec![self];
+        iter::from_fn(move || {
+            let expr = stack.pop()?;
+            match expr {
+                FilesetExpression::None
+                | FilesetExpression::All
+                | FilesetExpression::Pattern(_) => {}
+                FilesetExpression::UnionAll(exprs) => stack.extend(exprs.iter().rev()),
+                FilesetExpression::Intersection(expr1, expr2)
+                | FilesetExpression::Difference(expr1, expr2) => {
+                    stack.push(expr2);
+                    stack.push(expr1);
+                }
+            }
+            Some(expr)
+        })
+    }
+
+    /// Iterates literal paths recursively from this expression.
+    ///
+    /// For example, `"a", "b", "c"` will be yielded in that order for
+    /// expression `"a" | all() & "b" | ~"c"`.
+    pub fn explicit_paths(&self) -> impl Iterator<Item = &RepoPath> {
+        // pre/post-ordering doesn't matter so long as children are visited from
+        // left to right.
+        self.dfs_pre().flat_map(|expr| match expr {
+            FilesetExpression::Pattern(pattern) => pattern.as_path(),
+            _ => None,
+        })
+    }
+
     /// Transforms the expression tree to `Matcher` object.
     pub fn to_matcher(&self) -> Box<dyn Matcher> {
         build_union_matcher(self.as_union_all())
@@ -531,6 +572,39 @@ mod tests {
         "###);
     }
 
+    #[test]
+    fn test_explicit_paths() {
+        let collect = |expr: &FilesetExpression| -> Vec<RepoPathBuf> {
+            expr.explicit_paths().map(|path| path.to_owned()).collect()
+        };
+        let file_expr = |path: &str| FilesetExpression::file_path(repo_path_buf(path));
+        assert!(collect(&FilesetExpression::none()).is_empty());
+        assert_eq!(collect(&file_expr("a")), ["a"].map(repo_path_buf));
+        assert_eq!(
+            collect(&FilesetExpression::union_all(vec![
+                file_expr("a"),
+                file_expr("b"),
+                file_expr("c"),
+            ])),
+            ["a", "b", "c"].map(repo_path_buf)
+        );
+        assert_eq!(
+            collect(&FilesetExpression::intersection(
+                FilesetExpression::union_all(vec![
+                    file_expr("a"),
+                    FilesetExpression::none(),
+                    file_expr("b"),
+                    file_expr("c"),
+                ]),
+                FilesetExpression::difference(
+                    file_expr("d"),
+                    FilesetExpression::union_all(vec![file_expr("e"), file_expr("f")])
+                )
+            )),
+            ["a", "b", "c", "d", "e", "f"].map(repo_path_buf)
+        );
+    }
+
     #[test]
     fn test_build_matcher_simple() {
         insta::assert_debug_snapshot!(FilesetExpression::none().to_matcher(), @"NothingMatcher");