diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index d6580acd8e..37e27398cb 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -748,6 +748,37 @@ impl WorkspaceCommandHelper { /// Resolve a revset to a single revision. Return an error if the revset is /// empty or has multiple revisions. pub fn resolve_single_rev(&self, revision_str: &str) -> Result { + self.resolve_single_rev_with_hint_about_all_prefix(revision_str, false) + } + + /// Resolve a revset any number of revisions (including 0). + pub fn resolve_revset(&self, revision_str: &str) -> Result, CommandError> { + let revset_expression = self.parse_revset(revision_str)?; + let revset = self.evaluate_revset(revset_expression)?; + Ok(revset.iter().commits(self.repo().store()).try_collect()?) + } + + /// Resolve a revset any number of revisions (including 0), but require the + /// user to indicate if they allow multiple revisions by prefixing the + /// expression with `all:`. + pub fn resolve_revset_default_single( + &self, + revision_str: &str, + ) -> Result, CommandError> { + // TODO: Let pest parse the prefix too once we've dropped support for `:` + if let Some(revision_str) = revision_str.strip_prefix("all:") { + self.resolve_revset(revision_str) + } else { + self.resolve_single_rev_with_hint_about_all_prefix(revision_str, true) + .map(|commit| vec![commit]) + } + } + + fn resolve_single_rev_with_hint_about_all_prefix( + &self, + revision_str: &str, + should_hint_about_all_prefix: bool, + ) -> Result { let revset_expression = self.parse_revset(revision_str)?; let revset = self.evaluate_revset(revset_expression.clone())?; let mut iter = revset.iter().commits(self.repo().store()).fuse(); @@ -783,10 +814,17 @@ It resolved to these revisions: Set which revision the branch points to with `jj branch set {branch_name} -r `."#, ) } else { - format!( + let mut hint = format!( r#"The revset "{revision_str}" resolved to these revisions: {commits_summary}"#, - ) + ); + if should_hint_about_all_prefix { + hint.push_str(&format!( + "\nPrefix the expression with 'all:' to allow any number of revisions \ + (i.e. 'all:{revision_str}')." + )); + } + hint }; Err(user_error_with_hint( format!(r#"Revset "{revision_str}" resolved to more than one revision"#), @@ -796,41 +834,6 @@ Set which revision the branch points to with `jj branch set {branch_name} -r Result, CommandError> { - let revset_expression = self.parse_revset(revision_str)?; - let revset = self.evaluate_revset(revset_expression)?; - Ok(revset.iter().commits(self.repo().store()).try_collect()?) - } - - /// Resolve a revset any number of revisions (including 0), but require the - /// user to indicate if they allow multiple revisions by prefixing the - /// expression with `all:`. - pub fn resolve_revset_default_single( - &self, - revision_str: &str, - ) -> Result, CommandError> { - // TODO: Let pest parse the prefix too once we've dropped support for `:` - if let Some(revision_str) = revision_str.strip_prefix("all:") { - self.resolve_revset(revision_str) - } else { - self.resolve_single_rev(revision_str) - .map_err(|err| match err { - CommandError::UserError { err, hint } => CommandError::UserError { - err, - hint: Some(format!( - "{old_hint}Prefix the expression with 'all' to allow any number of \ - revisions (i.e. 'all:{}').", - revision_str, - old_hint = hint.map(|hint| format!("{hint}\n")).unwrap_or_default() - )), - }, - err => err, - }) - .map(|commit| vec![commit]) - } - } - pub fn parse_revset( &self, revision_str: &str, diff --git a/cli/tests/test_new_command.rs b/cli/tests/test_new_command.rs index 635ed6ff88..a67752af32 100644 --- a/cli/tests/test_new_command.rs +++ b/cli/tests/test_new_command.rs @@ -498,7 +498,6 @@ fn test_new_conflicting_branches() { kkmpptxz 66c6502d foo?? | (empty) two qpvuntsm a9330854 foo?? | (empty) one Set which revision the branch points to with `jj branch set foo -r `. - Prefix the expression with 'all' to allow any number of revisions (i.e. 'all:foo'). "###); } @@ -521,7 +520,21 @@ fn test_new_conflicting_change_ids() { qpvuntsm?? d2ae6806 (empty) two qpvuntsm?? a9330854 (empty) one Some of these commits have the same change id. Abandon one of them with `jj abandon -r `. - Prefix the expression with 'all' to allow any number of revisions (i.e. 'all:qpvuntsm'). + "###); +} + +#[test] +fn test_new_error_revision_does_not_exist() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]); + let repo_path = test_env.env_root().join("repo"); + + test_env.jj_cmd_ok(&repo_path, &["describe", "-m", "one"]); + test_env.jj_cmd_ok(&repo_path, &["new", "-m", "two"]); + + let stderr = test_env.jj_cmd_failure(&repo_path, &["new", "this"]); + insta::assert_snapshot!(stderr, @r###" + Error: Revision "this" doesn't exist "###); } diff --git a/cli/tests/test_rebase_command.rs b/cli/tests/test_rebase_command.rs index b820139c80..a773307af4 100644 --- a/cli/tests/test_rebase_command.rs +++ b/cli/tests/test_rebase_command.rs @@ -169,7 +169,7 @@ fn test_rebase_branch() { Hint: The revset "e|d" resolved to these revisions: znkkpsqq e52756c8 e | e vruxwmqv 514fa6b2 d | d - Prefix the expression with 'all' to allow any number of revisions (i.e. 'all:e|d'). + Prefix the expression with 'all:' to allow any number of revisions (i.e. 'all:e|d'). "###); let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-b=all:e|d", "-d=b"]); insta::assert_snapshot!(stdout, @""); @@ -480,7 +480,7 @@ fn test_rebase_multiple_destinations() { Hint: The revset "b|c" resolved to these revisions: royxmykx fe2e8e8b c | c zsuskuln d370aee1 b | b - Prefix the expression with 'all' to allow any number of revisions (i.e. 'all:b|c'). + Prefix the expression with 'all:' to allow any number of revisions (i.e. 'all:b|c'). "###); let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-r", "a", "-d", "all:b|c"]); insta::assert_snapshot!(stdout, @""); @@ -616,7 +616,7 @@ fn test_rebase_with_descendants() { Hint: The revset "b|d" resolved to these revisions: vruxwmqv df54a9fd d | d zsuskuln d370aee1 b | b - Prefix the expression with 'all' to allow any number of revisions (i.e. 'all:b|d'). + Prefix the expression with 'all:' to allow any number of revisions (i.e. 'all:b|d'). "###); let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-s=all:b|d", "-d=a"]); insta::assert_snapshot!(stdout, @""); @@ -638,6 +638,27 @@ fn test_rebase_with_descendants() { "###); } +#[test] +fn test_rebase_error_revision_does_not_exist() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]); + let repo_path = test_env.env_root().join("repo"); + + test_env.jj_cmd_ok(&repo_path, &["describe", "-m", "one"]); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "b-one"]); + test_env.jj_cmd_ok(&repo_path, &["new", "-r", "@-", "-m", "two"]); + + let stderr = test_env.jj_cmd_failure(&repo_path, &["rebase", "-b", "b-one", "-d", "this"]); + insta::assert_snapshot!(stderr, @r###" + Error: Revision "this" doesn't exist + "###); + + let stderr = test_env.jj_cmd_failure(&repo_path, &["rebase", "-b", "this", "-d", "b-one"]); + insta::assert_snapshot!(stderr, @r###" + Error: Revision "this" doesn't exist + "###); +} + fn get_log_output(test_env: &TestEnvironment, repo_path: &Path) -> String { test_env.jj_cmd_success(repo_path, &["log", "-T", "branches"]) }