From 079483ab9b60f068f60bd4e7e0f38072fd66344b Mon Sep 17 00:00:00 2001
From: Essien Ita Essien <34972+essiene@users.noreply.github.com>
Date: Thu, 15 Aug 2024 17:24:54 +0100
Subject: [PATCH] Update error message when no movement targets are found.

If movement commands don't find a target commit, they fail. However,
it's usually not intuitive why they fail because in non-edit mode the
start commit is the parent of the working commit.

Adding the start commit change hash to the error message makes it easier
for the user to figure out what is going on.

Also, specifying 'No **other** descendant|ancestor...' helps make it clear what
`jj` is really looking for.

Part of #3947
---
 cli/src/movement_util.rs             | 43 +++++++++++++++++-----------
 cli/tests/test_next_prev_commands.rs |  8 +++---
 2 files changed, 31 insertions(+), 20 deletions(-)

diff --git a/cli/src/movement_util.rs b/cli/src/movement_util.rs
index d699bfc371..34d4090d55 100644
--- a/cli/src/movement_util.rs
+++ b/cli/src/movement_util.rs
@@ -16,12 +16,12 @@ use std::io::Write;
 use std::rc::Rc;
 
 use itertools::Itertools;
-use jj_lib::backend::CommitId;
+use jj_lib::backend::{BackendError, CommitId};
 use jj_lib::commit::Commit;
 use jj_lib::repo::Repo;
 use jj_lib::revset::{RevsetExpression, RevsetFilterPredicate, RevsetIteratorExt};
 
-use crate::cli_util::WorkspaceCommandHelper;
+use crate::cli_util::{short_change_hash, WorkspaceCommandHelper};
 use crate::command_error::{user_error, CommandError};
 use crate::ui::{MovementEditMode, Ui};
 
@@ -53,20 +53,11 @@ impl Direction {
 
     fn get_target_revset(
         &self,
-        working_commit_id: &CommitId,
-        edit: bool,
+        working_revset: &Rc<RevsetExpression>,
+        start_revset: &Rc<RevsetExpression>,
         has_conflict: bool,
         change_offset: u64,
     ) -> Result<Rc<RevsetExpression>, CommandError> {
-        let wc_revset = RevsetExpression::commit(working_commit_id.clone());
-        // If we're editing, start at the working-copy commit. Otherwise, start from
-        // its direct parent(s).
-        let start_revset = if edit {
-            wc_revset.clone()
-        } else {
-            wc_revset.parents()
-        };
-
         let target_revset = match self {
             Direction::Next => if has_conflict {
                 start_revset
@@ -77,7 +68,7 @@ impl Direction {
             } else {
                 start_revset.descendants_at(change_offset)
             }
-            .minus(&wc_revset),
+            .minus(working_revset),
 
             Direction::Prev => {
                 if has_conflict {
@@ -115,8 +106,27 @@ pub fn get_target_commit(
     has_conflict: bool,
     change_offset: u64,
 ) -> Result<Commit, CommandError> {
+    let wc_revset = RevsetExpression::commit(working_commit_id.clone());
+    // If we're editing, start at the working-copy commit. Otherwise, start from
+    // its direct parent(s).
+    let start_revset = if edit {
+        wc_revset.clone()
+    } else {
+        wc_revset.parents()
+    };
+
+    let start_commit: Commit = start_revset
+        .clone()
+        .evaluate_programmatic(workspace_command.repo().as_ref())?
+        .iter()
+        .commits(workspace_command.repo().store())
+        .collect::<Result<Vec<_>, BackendError>>()?
+        .pop()
+        .unwrap();
+
     let target_revset =
-        direction.get_target_revset(working_commit_id, edit, has_conflict, change_offset)?;
+        direction.get_target_revset(&wc_revset, &start_revset, has_conflict, change_offset)?;
+
     let targets: Vec<Commit> = target_revset
         .evaluate_programmatic(workspace_command.repo().as_ref())?
         .iter()
@@ -128,11 +138,12 @@ pub fn get_target_commit(
         [] => {
             // We found no descendant.
             return Err(user_error(format!(
-                "No {} found {} commit{} {}",
+                "No other {} found {} commit{} {} from {}",
                 direction.next_node_type(),
                 change_offset,
                 if change_offset > 1 { "s" } else { "" },
                 direction.direction(),
+                short_change_hash(start_commit.change_id()),
             )));
         }
         commits => choose_commit(ui, workspace_command, &direction, commits)?,
diff --git a/cli/tests/test_next_prev_commands.rs b/cli/tests/test_next_prev_commands.rs
index 31ea91db86..b48c0b8bb9 100644
--- a/cli/tests/test_next_prev_commands.rs
+++ b/cli/tests/test_next_prev_commands.rs
@@ -201,7 +201,7 @@ fn test_next_exceeding_history() {
     let stderr = test_env.jj_cmd_failure(&repo_path, &["next", "3"]);
     // `jj next` beyond existing history fails.
     insta::assert_snapshot!(stderr, @r###"
-    Error: No descendant found 3 commits forward
+    Error: No descendant found 3 commits forward from rlvkpnrzqnoo
     "###);
 }
 
@@ -594,7 +594,7 @@ fn test_prev_beyond_root_fails() {
     // @- is at "fourth", and there is no parent 5 commits behind it.
     let stderr = test_env.jj_cmd_failure(&repo_path, &["prev", "5"]);
     insta::assert_snapshot!(stderr,@r###"
-    Error: No ancestor found 5 commits back
+    Error: No ancestor found 5 commits back from zsuskulnrvyr
     "###);
 }
 
@@ -860,12 +860,12 @@ fn test_next_conflict_head() {
     "###);
     let stderr = test_env.jj_cmd_failure(&repo_path, &["next", "--conflict"]);
     insta::assert_snapshot!(stderr, @r###"
-    Error: No descendant found 1 commit forward
+    Error: No descendant found 1 commit forward from zzzzzzzzzzzz
     "###);
 
     let stderr = test_env.jj_cmd_failure(&repo_path, &["next", "--conflict", "--edit"]);
     insta::assert_snapshot!(stderr, @r###"
-    Error: No descendant found 1 commit forward
+    Error: No descendant found 1 commit forward from rlvkpnrzqnoo
     "###);
 }