Skip to content

Commit

Permalink
cli: warn explicit paths not exist in either of diff trees
Browse files Browse the repository at this point in the history
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
  • Loading branch information
yuja committed Apr 10, 2024
1 parent 33beb8d commit ae70db8
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 4 deletions.
26 changes: 26 additions & 0 deletions cli/src/cli_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
13 changes: 9 additions & 4 deletions cli/src/commands/diff.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(
Expand All @@ -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(())
}
27 changes: 27 additions & 0 deletions cli/tests/test_diff_command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down

0 comments on commit ae70db8

Please sign in to comment.