From b07775c2732a110077b4522e423c9d5f67ca0a7e 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] next/prev: 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...' helps make it clear what `jj` is really looking for. Part of #3947 --- cli/src/movement_util.rs | 81 +++++++++++++++++++++------- cli/tests/test_next_prev_commands.rs | 48 ++++++++++------- 2 files changed, 91 insertions(+), 38 deletions(-) diff --git a/cli/src/movement_util.rs b/cli/src/movement_util.rs index 6cf6d8939b..bfb7c0a015 100644 --- a/cli/src/movement_util.rs +++ b/cli/src/movement_util.rs @@ -17,12 +17,14 @@ use std::rc::Rc; use config::Config; use itertools::Itertools; -use jj_lib::backend::CommitId; +use jj_lib::backend::{BackendError, ChangeId, CommitId}; use jj_lib::commit::Commit; use jj_lib::repo::Repo; use jj_lib::revset::{RevsetExpression, RevsetFilterPredicate, RevsetIteratorExt}; -use crate::cli_util::{short_commit_hash, CommandHelper, WorkspaceCommandHelper}; +use crate::cli_util::{ + short_change_hash, short_commit_hash, CommandHelper, WorkspaceCommandHelper, +}; use crate::command_error::{config_error_with_message, user_error, CommandError}; use crate::ui::Ui; @@ -55,27 +57,47 @@ impl Direction { } } - fn target_not_found_message(&self, change_offset: u64) -> String { + fn target_not_found_message( + &self, + edit: bool, + change_offset: u64, + change_id: &ChangeId, + ) -> String { + let change_hash = short_change_hash(change_id); match self { - Direction::Next => format!("No descendant found {} commit(s) forward", change_offset), - Direction::Prev => format!("No ancestor found {} commit(s) back", change_offset), + Direction::Next => { + if edit { + // in edit mode, start_revset is the WC, so + // we only look for direct descendants. + format!( + "No descendant found {} commit(s) forward from {}", + change_offset, change_hash + ) + } else { + // in non-edit mode, start_revset is the parent + // of WC, so we look for other descendants of + // start_revset. + format!( + "No other descendant found {} commit(s) forward from {}", + change_offset, change_hash + ) + } + } + // The WC can never be an ancestor of the start_revset since + // start_revset is either itself or it's parent. + Direction::Prev => format!( + "No ancestor found {} commit(s) back from {}", + change_offset, change_hash + ), } } fn get_target_revset( &self, - working_commit_id: &CommitId, + working_revset: &Rc, + start_revset: &Rc, args: &MovementArgsInternal, ) -> Result, 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 args.should_edit { - wc_revset.clone() - } else { - wc_revset.parents() - }; - let target_revset = match self { Direction::Next => if args.conflict { start_revset @@ -86,7 +108,7 @@ impl Direction { } else { start_revset.descendants_at(args.offset) } - .minus(&wc_revset), + .minus(working_revset), Direction::Prev => { if args.conflict { @@ -114,7 +136,26 @@ fn get_target_commit( working_commit_id: &CommitId, args: &MovementArgsInternal, ) -> Result { - let target_revset = direction.get_target_revset(working_commit_id, args)?; + 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 args.should_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::, BackendError>>()? + .pop() + .unwrap(); + + let target_revset = direction.get_target_revset(&wc_revset, &start_revset, args)?; + let targets: Vec = target_revset .evaluate_programmatic(workspace_command.repo().as_ref())? .iter() @@ -125,7 +166,11 @@ fn get_target_commit( [target] => target, [] => { // We found no ancestor/descendant. - return Err(user_error(direction.target_not_found_message(args.offset))); + return Err(user_error(direction.target_not_found_message( + args.should_edit, + args.offset, + 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 0e63eaa545..2cfedddafc 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 commit(s) forward + Error: No descendant found 3 commit(s) 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 commit(s) back + Error: No ancestor found 5 commit(s) back from zsuskulnrvyr "###); } @@ -858,14 +858,15 @@ fn test_next_conflict_head() { @ rlvkpnrzqnoo conflict ◆ zzzzzzzzzzzz "###); + let stderr = test_env.jj_cmd_failure(&repo_path, &["next", "--conflict"]); insta::assert_snapshot!(stderr, @r###" - Error: No descendant found 1 commit(s) forward + Error: No other descendant found 1 commit(s) 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(s) forward + Error: No descendant found 1 commit(s) forward from rlvkpnrzqnoo "###); } @@ -963,7 +964,7 @@ fn test_movement_edit_mode_auto() { // next --no-edit will fail on a linear tree. let stderr = test_env.jj_cmd_failure(&repo_path, &["next", "--no-edit"]); insta::assert_snapshot!(stderr, @r###" - Error: No descendant found 1 commit(s) forward + Error: No other descendant found 1 commit(s) forward from qpvuntsmwlqt "###); // to make it pass, first create a new commit @@ -1056,6 +1057,11 @@ fn test_movement_edit_mode_always() { ◆ zzzzzzzzzzzz "###); + let stderr = test_env.jj_cmd_failure(&repo_path, &["prev"]); + insta::assert_snapshot!(stderr, @r###" + Error: The root commit 000000000000 is immutable + "###); + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" @@ -1087,12 +1093,12 @@ fn test_movement_edit_mode_always() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["prev", "--no-edit"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" - Working copy now at: nkmrtpmo 67a001a6 (empty) (no description set) + Working copy now at: uyznsvlq 7ad57fb8 (empty) (no description set) Parent commit : qpvuntsm fa15625b (empty) first "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - @ nkmrtpmomlro + @ uyznsvlquzzm │ ○ kkmpptxzrspx third │ ○ rlvkpnrzqnoo second ├─╯ @@ -1103,18 +1109,23 @@ fn test_movement_edit_mode_always() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next", "--no-edit"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" - Working copy now at: xznxytkn 22287813 (empty) (no description set) + Working copy now at: xtnwkqum 7ac7a1c4 (empty) (no description set) Parent commit : rlvkpnrz 9ed53a4a (empty) second "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - @ xznxytknoqwo + @ xtnwkqumpolk │ ○ kkmpptxzrspx third ├─╯ ○ rlvkpnrzqnoo second ○ qpvuntsmwlqt first ◆ zzzzzzzzzzzz "###); + + let stderr = test_env.jj_cmd_failure(&repo_path, &["next"]); + insta::assert_snapshot!(stderr, @r###" + Error: No descendant found 1 commit(s) forward from xtnwkqumpolk + "###); } #[test] @@ -1163,15 +1174,20 @@ fn test_movement_edit_mode_never() { ◆ zzzzzzzzzzzz "###); + let stderr = test_env.jj_cmd_failure(&repo_path, &["prev", "3"]); + insta::assert_snapshot!(stderr, @r###" + Error: No ancestor found 3 commit(s) back from qpvuntsmwlqt + "###); + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" - Working copy now at: znkkpsqq a8419fd6 (empty) (no description set) + Working copy now at: kpqxywon d06750fb (empty) (no description set) Parent commit : rlvkpnrz 9ed53a4a (empty) second "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - @ znkkpsqqskkl + @ kpqxywonksrl │ ○ kkmpptxzrspx third ├─╯ ○ rlvkpnrzqnoo second @@ -1182,18 +1198,10 @@ fn test_movement_edit_mode_never() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next", "--no-edit"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" - Working copy now at: kmkuslsw 8c4d85ef (empty) (no description set) + Working copy now at: wqnwkozp 10fa181f (empty) (no description set) Parent commit : kkmpptxz 30056b0c (empty) third "###); - insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - @ kmkuslswpqwq - ○ kkmpptxzrspx third - ○ rlvkpnrzqnoo second - ○ qpvuntsmwlqt first - ◆ zzzzzzzzzzzz - "###); - let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["prev", "--edit", "2"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###"