diff --git a/cli/src/movement_util.rs b/cli/src/movement_util.rs index e75ee7e33f..63b7eb904c 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::{short_commit_hash, WorkspaceCommandHelper}; +use crate::cli_util::{short_change_hash, short_commit_hash, WorkspaceCommandHelper}; use crate::command_error::{user_error, CommandError}; use crate::ui::{MovementEditMode, Ui}; @@ -38,7 +38,12 @@ impl Direction { } } - fn target_not_found_message(&self, change_offset: u64) -> String { + fn target_not_found_message( + &self, + edit: bool, + change_offset: u64, + change_hash: &str, + ) -> String { let commit_text = if change_offset > 1 { "commits" } else { @@ -46,30 +51,42 @@ impl Direction { }; match self { - Direction::Next => format!( - "No descendant found {} {} forward", - change_offset, commit_text + Direction::Next => { + if edit { + // in edit mode, start_revset is the WC, so + // we only look for direct descendants. + + format!( + "No descendant found {} {} forward from {}", + change_offset, commit_text, change_hash + ) + } else { + // in none edit mode, start_revset is the parent + // of WC, so we look for other descendants of + // start_revset. + + format!( + "No other descendant found {} {} forward from {}", + change_offset, commit_text, 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 {} {} back from {}", + change_offset, commit_text, change_hash ), - Direction::Prev => format!("No ancestor found {} {} back", change_offset, commit_text), } } fn get_target_revset( &self, - working_commit_id: &CommitId, - edit: bool, + working_revset: &Rc, + start_revset: &Rc, has_conflict: bool, change_offset: u64, ) -> 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 edit { - wc_revset.clone() - } else { - wc_revset.parents() - }; - let target_revset = match self { Direction::Next => if has_conflict { start_revset @@ -80,7 +97,7 @@ impl Direction { } else { start_revset.descendants_at(change_offset) } - .minus(&wc_revset), + .minus(working_revset), Direction::Prev => { if has_conflict { @@ -110,8 +127,27 @@ fn get_target_commit( has_conflict: bool, change_offset: u64, ) -> Result { + 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::, 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 = target_revset .evaluate_programmatic(workspace_command.repo().as_ref())? .iter() @@ -122,9 +158,11 @@ fn get_target_commit( [target] => target, [] => { // We found no ancestor/descendant. - return Err(user_error( - direction.target_not_found_message(change_offset), - )); + return Err(user_error(direction.target_not_found_message( + edit, + change_offset, + &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..a0d5194f55 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 other 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 "###); } @@ -1003,6 +1003,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###" @@ -1030,6 +1035,11 @@ fn test_movement_edit_mode_always() { ○ qpvuntsmwlqt first ◆ zzzzzzzzzzzz "###); + + let stderr = test_env.jj_cmd_failure(&repo_path, &["next"]); + insta::assert_snapshot!(stderr, @r###" + Error: No descendant found 1 commit forward from kkmpptxzrspx + "###); } #[test] @@ -1078,15 +1088,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 commits 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 @@ -1094,19 +1109,9 @@ fn test_movement_edit_mode_never() { ◆ zzzzzzzzzzzz "###); - let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next"]); - insta::assert_snapshot!(stdout, @""); + let stderr = test_env.jj_cmd_failure(&repo_path, &["next", "5"]); insta::assert_snapshot!(stderr, @r###" - Working copy now at: kmkuslsw 8c4d85ef (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 + Error: No other descendant found 5 commits forward from rlvkpnrzqnoo "###); }