cli: avoid bad hint "Prefix the expression with 'all'..."

There is no point of showing the hint for non-existent revision.

Also appends ':' to the 'all' to make the hint more precise.

#2451

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<Commit, CommandError> {
+        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<Vec<Commit>, 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<Vec<Commit>, 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<Commit, CommandError> {
         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 <REVISION>`."#,
                     )
                 } 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 <RE
         }
     }
 
-    /// Resolve a revset any number of revisions (including 0).
-    pub fn resolve_revset(&self, revision_str: &str) -> Result<Vec<Commit>, 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<Vec<Commit>, 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,

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 <REVISION>`.
-    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 <REVISION>`.
-    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
     "###);
 }
 

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"])
 }