From 10cac54d0cc72b16c40dcf416e1b291256ba68b8 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Tue, 9 Apr 2024 16:27:04 +0900 Subject: [PATCH 1/3] fileset: add recursive iterator over explicit paths The primary use case is to warn unmatched paths. I originally thought paths in negated expressions shouldn't be checked, but doing that seems rather inconsistent than useful. For example, "~x" in "jj split '~x'" should match at least one file to split to non-empty revisions. --- lib/src/fileset.rs | 78 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 76 insertions(+), 2 deletions(-) diff --git a/lib/src/fileset.rs b/lib/src/fileset.rs index 31368ae28d..aada568f25 100644 --- a/lib/src/fileset.rs +++ b/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 { + 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 { + // 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 { build_union_matcher(self.as_union_all()) @@ -531,6 +572,39 @@ mod tests { "###); } + #[test] + fn test_explicit_paths() { + let collect = |expr: &FilesetExpression| -> Vec { + 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"); From 82d2d9f24d60cd07bd693326f27dd58165e4cae6 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Tue, 9 Apr 2024 16:35:23 +0900 Subject: [PATCH 2/3] cli: warn explicit paths not exist in either of diff trees Maybe we can optimize it to check paths during diffing, but I think it's okay to add extra lookup cost at the end. The size of the path arguments is usually small. Closes #505 --- cli/src/cli_util.rs | 26 ++++++++++++++++++++++++++ cli/src/commands/diff.rs | 13 +++++++++---- cli/tests/test_diff_command.rs | 27 +++++++++++++++++++++++++++ 3 files changed, 62 insertions(+), 4 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 62b23d86fc..924413d189 100644 --- a/cli/src/cli_util.rs +++ b/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, +) -> 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() diff --git a/cli/src/commands/diff.rs b/cli/src/commands/diff.rs index 99ae066139..6a44f67b61 100644 --- a/cli/src/commands/diff.rs +++ b/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(()) } diff --git a/cli/tests/test_diff_command.rs b/cli/tests/test_diff_command.rs index dac18276a3..e0a74b7bb0 100644 --- a/cli/tests/test_diff_command.rs +++ b/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] From 445044e5fdfff352c84588d663b4fc9aba6b31b0 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Tue, 9 Apr 2024 12:33:32 +0900 Subject: [PATCH 3/3] cli: migrate "chmod" to matcher API, warn unmatched paths --- cli/src/commands/chmod.rs | 22 ++++++++-------------- cli/tests/test_chmod_command.rs | 29 +++++++++-------------------- 2 files changed, 17 insertions(+), 34 deletions(-) diff --git a/cli/src/commands/chmod.rs b/cli/src/commands/chmod.rs index a63fdb9627..4d8078d4ea 100644 --- a/cli/src/commands/chmod.rs +++ b/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() diff --git a/cli/tests/test_chmod_command.rs b/cli/tests/test_chmod_command.rs index a43fa15fc8..5081957819 100644 --- a/cli/tests/test_chmod_command.rs +++ b/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 "###); }