diff --git a/CHANGELOG.md b/CHANGELOG.md index 5097749db..c80ff6d15 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -64,6 +64,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * `jj git` subcommands now support credential helpers. +* `jj log` will warn if it appears that the provided path was meant to be a + revset. + ### Fixed bugs * (#463) A bug in the export of branches to Git caused spurious conflicted diff --git a/lib/src/revset.rs b/lib/src/revset.rs index be88029c8..8e6791293 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -1049,6 +1049,10 @@ pub fn optimize(expression: Rc) -> Rc { pub trait Revset<'repo> { // All revsets currently iterate in order of descending index position fn iter<'revset>(&'revset self) -> RevsetIterator<'revset, 'repo>; + + fn is_empty(&self) -> bool { + self.iter().next().is_none() + } } pub struct RevsetIterator<'revset, 'repo: 'revset> { diff --git a/src/cli_util.rs b/src/cli_util.rs index 325d5f699..e590fb62b 100644 --- a/src/cli_util.rs +++ b/src/cli_util.rs @@ -53,6 +53,7 @@ use crate::formatter::Formatter; use crate::templater::TemplateFormatter; use crate::ui::{ColorChoice, Ui}; +#[derive(Debug)] pub enum CommandError { UserError { message: String, diff --git a/src/commands.rs b/src/commands.rs index adc949c9b..bf4488e33 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -2023,10 +2023,12 @@ fn cmd_log(ui: &mut Ui, command: &CommandHelper, args: &LogArgs) -> Result<(), C let workspace_id = workspace_command.workspace_id(); let checkout_id = repo.view().get_wc_commit_id(&workspace_id); let matcher = workspace_command.matcher_from_values(&args.paths)?; - let mut revset = workspace_command.evaluate_revset(&revset_expression)?; - if !args.paths.is_empty() { - revset = revset::filter_by_diff(repo.as_repo_ref(), matcher.as_ref(), revset); - } + let revset = workspace_command.evaluate_revset(&revset_expression)?; + let revset = if !args.paths.is_empty() { + revset::filter_by_diff(repo.as_repo_ref(), matcher.as_ref(), revset) + } else { + revset + }; let store = repo.store(); let diff_format = (args.patch || args.diff_format.git || args.diff_format.summary) @@ -2042,94 +2044,115 @@ fn cmd_log(ui: &mut Ui, command: &CommandHelper, args: &LogArgs) -> Result<(), C &template_string, ); - let mut formatter = ui.stdout_formatter(); - let mut formatter = formatter.as_mut(); - formatter.add_label("log")?; + { + let mut formatter = ui.stdout_formatter(); + let mut formatter = formatter.as_mut(); + formatter.add_label("log")?; - if !args.no_graph { - let mut graph = AsciiGraphDrawer::new(&mut formatter); - let iter: Box)>> = if args.reversed { - Box::new(revset.iter().graph().reversed()) - } else { - Box::new(revset.iter().graph()) - }; - for (index_entry, edges) in iter { - let mut graphlog_edges = vec![]; - // TODO: Should we update RevsetGraphIterator to yield this flag instead of all - // the missing edges since we don't care about where they point here - // anyway? - let mut has_missing = false; - for edge in edges { - match edge.edge_type { - RevsetGraphEdgeType::Missing => { - has_missing = true; - } - RevsetGraphEdgeType::Direct => graphlog_edges.push(Edge::Present { - direct: true, - target: edge.target, - }), - RevsetGraphEdgeType::Indirect => graphlog_edges.push(Edge::Present { - direct: false, - target: edge.target, - }), - } - } - if has_missing { - graphlog_edges.push(Edge::Missing); - } - let mut buffer = vec![]; - let commit_id = index_entry.commit_id(); - let commit = store.get_commit(&commit_id)?; - let is_checkout = Some(&commit_id) == checkout_id; - { - let mut formatter = ui.new_formatter(&mut buffer); - if is_checkout { - formatter.with_label("working_copy", |formatter| { - template.format(&commit, formatter) - })?; + if !args.no_graph { + let mut graph = AsciiGraphDrawer::new(&mut formatter); + let iter: Box)>> = + if args.reversed { + Box::new(revset.iter().graph().reversed()) } else { - template.format(&commit, formatter.as_mut())?; + Box::new(revset.iter().graph()) + }; + for (index_entry, edges) in iter { + let mut graphlog_edges = vec![]; + // TODO: Should we update RevsetGraphIterator to yield this flag instead of all + // the missing edges since we don't care about where they point here + // anyway? + let mut has_missing = false; + for edge in edges { + match edge.edge_type { + RevsetGraphEdgeType::Missing => { + has_missing = true; + } + RevsetGraphEdgeType::Direct => graphlog_edges.push(Edge::Present { + direct: true, + target: edge.target, + }), + RevsetGraphEdgeType::Indirect => graphlog_edges.push(Edge::Present { + direct: false, + target: edge.target, + }), + } + } + if has_missing { + graphlog_edges.push(Edge::Missing); + } + let mut buffer = vec![]; + let commit_id = index_entry.commit_id(); + let commit = store.get_commit(&commit_id)?; + let is_checkout = Some(&commit_id) == checkout_id; + { + let mut formatter = ui.new_formatter(&mut buffer); + if is_checkout { + formatter.with_label("working_copy", |formatter| { + template.format(&commit, formatter) + })?; + } else { + template.format(&commit, formatter.as_mut())?; + } + } + if !buffer.ends_with(b"\n") { + buffer.push(b'\n'); + } + if let Some(diff_format) = diff_format { + let mut formatter = ui.new_formatter(&mut buffer); + show_patch( + formatter.as_mut(), + &workspace_command, + &commit, + matcher.as_ref(), + diff_format, + )?; + } + let node_symbol = if is_checkout { b"@" } else { b"o" }; + graph.add_node( + &index_entry.position(), + &graphlog_edges, + node_symbol, + &buffer, + )?; + } + } else { + let iter: Box> = if args.reversed { + Box::new(revset.iter().reversed()) + } else { + Box::new(revset.iter()) + }; + for index_entry in iter { + let commit = store.get_commit(&index_entry.commit_id())?; + template.format(&commit, formatter)?; + if let Some(diff_format) = diff_format { + show_patch( + formatter, + &workspace_command, + &commit, + matcher.as_ref(), + diff_format, + )?; } } - if !buffer.ends_with(b"\n") { - buffer.push(b'\n'); - } - if let Some(diff_format) = diff_format { - let mut formatter = ui.new_formatter(&mut buffer); - show_patch( - formatter.as_mut(), - &workspace_command, - &commit, - matcher.as_ref(), - diff_format, - )?; - } - let node_symbol = if is_checkout { b"@" } else { b"o" }; - graph.add_node( - &index_entry.position(), - &graphlog_edges, - node_symbol, - &buffer, - )?; } - } else { - let iter: Box> = if args.reversed { - Box::new(revset.iter().reversed()) - } else { - Box::new(revset.iter()) - }; - for index_entry in iter { - let commit = store.get_commit(&index_entry.commit_id())?; - template.format(&commit, formatter)?; - if let Some(diff_format) = diff_format { - show_patch( - formatter, - &workspace_command, - &commit, - matcher.as_ref(), - diff_format, - )?; - } + } + + // Check to see if the user might have specified a path when they intended + // to specify a revset. + if let (None, [only_path]) = (&args.revisions, args.paths.as_slice()) { + if only_path == "." && workspace_command.parse_file_path(only_path)?.is_root() { + // For users of e.g. Mercurial, where `.` indicates the current commit. + ui.write_warn(&format!( + "warning: The argument {only_path:?} is being interpreted as a path, but this is \ + often not useful because all non-empty commits touch '.'. If you meant to show \ + the working copy commit, pass -r '@' instead.\n" + ))?; + } else if revset.is_empty() && revset::parse(only_path, None).is_ok() { + ui.write_warn(&format!( + "warning: The argument {only_path:?} is being interpreted as a path. To specify a \ + revset, pass -r {only_path:?} instead.\n" + ))?; } } diff --git a/tests/test_log_command.rs b/tests/test_log_command.rs index 90ff0ba06..dfb16608d 100644 --- a/tests/test_log_command.rs +++ b/tests/test_log_command.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use common::TestEnvironment; +use common::{get_stderr_string, get_stdout_string, TestEnvironment}; pub mod common; @@ -217,6 +217,73 @@ fn test_log_filtered_by_path() { "###); } +#[test] +fn test_log_warn_path_might_be_revset() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]); + let repo_path = test_env.env_root().join("repo"); + + std::fs::write(repo_path.join("file1"), "foo\n").unwrap(); + + // Don't warn if the file actually exists. + let assert = test_env + .jj_cmd(&repo_path, &["log", "file1", "-T", "description"]) + .assert() + .success(); + insta::assert_snapshot!(get_stdout_string(&assert), @r###" + @ (no description set) + ~ + "###); + insta::assert_snapshot!(get_stderr_string(&assert), @""); + + // Warn for `jj log .` specifically, for former Mercurial users. + let assert = test_env + .jj_cmd(&repo_path, &["log", ".", "-T", "description"]) + .assert() + .success(); + insta::assert_snapshot!(get_stdout_string(&assert), @r###" + @ (no description set) + ~ + "###); + insta::assert_snapshot!(get_stderr_string(&assert), @r###"warning: The argument "." is being interpreted as a path, but this is often not useful because all non-empty commits touch '.'. If you meant to show the working copy commit, pass -r '@' instead."###); + + // ...but checking `jj log .` makes sense in a subdirectory. + let subdir = repo_path.join("dir"); + std::fs::create_dir_all(&subdir).unwrap(); + let assert = test_env.jj_cmd(&subdir, &["log", "."]).assert().success(); + insta::assert_snapshot!(get_stdout_string(&assert), @""); + insta::assert_snapshot!(get_stderr_string(&assert), @""); + + // Warn for `jj log @` instead of `jj log -r @`. + let assert = test_env + .jj_cmd(&repo_path, &["log", "@", "-T", "description"]) + .assert() + .success(); + insta::assert_snapshot!(get_stdout_string(&assert), @""); + insta::assert_snapshot!(get_stderr_string(&assert), @r###" + warning: The argument "@" is being interpreted as a path. To specify a revset, pass -r "@" instead. + "###); + + // Warn when there's no path with the provided name. + let assert = test_env + .jj_cmd(&repo_path, &["log", "file2", "-T", "description"]) + .assert() + .success(); + insta::assert_snapshot!(get_stdout_string(&assert), @""); + insta::assert_snapshot!(get_stderr_string(&assert), @r###" + warning: The argument "file2" is being interpreted as a path. To specify a revset, pass -r "file2" instead. + "###); + + // If an explicit revision is provided, then suppress the warning. + let assert = test_env + .jj_cmd(&repo_path, &["log", "@", "-r", "@", "-T", "description"]) + .assert() + .success(); + insta::assert_snapshot!(get_stdout_string(&assert), @""); + insta::assert_snapshot!(get_stderr_string(&assert), @r###" + "###); +} + #[test] fn test_default_revset() { let test_env = TestEnvironment::default();