diff --git a/CHANGELOG.md b/CHANGELOG.md index 4194964b74..0ee58eed7b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Fixed bugs +* Fix confusing/unclear error message, when next/prev can not find a target commit + by explicitly specifying the commit where the search for the target started from. + * Release binaries for Intel Macs have been restored. They were previously broken due to using a sunset version of GitHub's macOS runners (but nobody had previously complained.) diff --git a/cli/src/movement_util.rs b/cli/src/movement_util.rs index 8c18714199..b91fdddf9b 100644 --- a/cli/src/movement_util.rs +++ b/cli/src/movement_util.rs @@ -59,27 +59,72 @@ impl Direction { } } - fn target_not_found_message(&self, change_offset: u64) -> String { - match self { - Direction::Next => format!("No descendant found {} commit(s) forward", change_offset), - Direction::Prev => format!("No ancestor found {} commit(s) back", change_offset), - } + fn target_not_found_error( + &self, + workspace_command: &WorkspaceCommandHelper, + edit: bool, + change_offset: u64, + commits: &[Commit], + ) -> CommandError { + let err_msg = match self { + 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 the working copy", + change_offset + ) + } 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 the working copy \ + parent(s)", + change_offset + ) + } + } + // The WC can never be an ancestor of the start_revset since + // start_revset is either itself or it's parent. + Direction::Prev => { + if edit { + format!( + "No ancestor found {} commit(s) back from the working copy", + change_offset + ) + } else { + format!( + "No ancestor found {} commit(s) back from the working copy parents(s)", + change_offset + ) + } + } + }; + + let template = workspace_command.commit_summary_template(); + let mut cmd_err = user_error(err_msg); + commits.iter().for_each(|commit| { + cmd_err.add_formatted_hint_with(|formatter| { + if edit { + write!(formatter, "Working copy: ")?; + } else { + write!(formatter, "Working copy parent: ")?; + } + template.format(commit, formatter) + }) + }); + + cmd_err } 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 @@ -90,7 +135,7 @@ impl Direction { } else { start_revset.descendants_at(args.offset) } - .minus(&wc_revset), + .minus(working_revset), Direction::Prev => { if args.conflict { @@ -118,7 +163,24 @@ 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_commits: Vec = start_revset + .clone() + .evaluate_programmatic(workspace_command.repo().as_ref())? + .iter() + .commits(workspace_command.repo().store()) + .try_collect()?; + + 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() @@ -129,7 +191,12 @@ fn get_target_commit( [target] => target, [] => { // We found no ancestor/descendant. - return Err(user_error(direction.target_not_found_message(args.offset))); + return Err(direction.target_not_found_error( + workspace_command, + args.should_edit, + args.offset, + &start_commits, + )); } 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 5338bf3853..179c4cf5cf 100644 --- a/cli/tests/test_next_prev_commands.rs +++ b/cli/tests/test_next_prev_commands.rs @@ -203,7 +203,8 @@ 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 other descendant found 3 commit(s) forward from the working copy parent(s) + Hint: Working copy parent: qpvuntsm fa15625b (empty) first "###); } @@ -544,10 +545,12 @@ fn test_prev_prompts_on_multiple_parents() { // Create a merge commit, which has two parents. test_env.jj_cmd_ok(&repo_path, &["new", "all:@--+"]); test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "merge"]); + test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "merge+1"]); // Check that the graph looks the way we expect. insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - @ vruxwmqvtpmx + @ yostqsxwqrlt + ○ vruxwmqvtpmx merge+1 ○ yqosqzytrlsw merge ├─┬─╮ │ │ ○ qpvuntsmwlqt first @@ -559,7 +562,7 @@ fn test_prev_prompts_on_multiple_parents() { "###); // Move @ backwards. - let (stdout, stderr) = test_env.jj_cmd_stdin_ok(&repo_path, &["prev"], "3\n"); + let (stdout, stderr) = test_env.jj_cmd_stdin_ok(&repo_path, &["prev", "2"], "3\n"); insta::assert_snapshot!(stdout,@r###" ambiguous prev commit, choose one to target: 1: mzvwutvl bc4f4fe3 (empty) third @@ -569,9 +572,45 @@ fn test_prev_prompts_on_multiple_parents() { enter the index of the commit you want to target: "###); insta::assert_snapshot!(stderr,@r###" - Working copy now at: znkkpsqq 07b409e8 (empty) (no description set) + Working copy now at: kpqxywon ddac00b0 (empty) (no description set) Parent commit : qpvuntsm fa15625b (empty) first "###); + + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ kpqxywonksrl + │ ○ vruxwmqvtpmx merge+1 + │ ○ yqosqzytrlsw merge + ╭─┼─╮ + ○ │ │ qpvuntsmwlqt first + │ │ ○ kkmpptxzrspx second + ├───╯ + │ ○ mzvwutvlkqwt third + ├─╯ + ◆ zzzzzzzzzzzz + "###); + + _ = test_env.jj_cmd_ok(&repo_path, &["next"]); + _ = test_env.jj_cmd_ok(&repo_path, &["edit", "@-"]); + + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + ○ vruxwmqvtpmx merge+1 + @ yqosqzytrlsw merge + ├─┬─╮ + │ │ ○ qpvuntsmwlqt first + │ ○ │ kkmpptxzrspx second + │ ├─╯ + ○ │ mzvwutvlkqwt third + ├─╯ + ◆ zzzzzzzzzzzz + "###); + + let stderr = test_env.jj_cmd_failure(&repo_path, &["next", "--no-edit"]); + insta::assert_snapshot!(stderr,@r###" + Error: No other descendant found 1 commit(s) forward from the working copy parent(s) + Hint: Working copy parent: mzvwutvl bc4f4fe3 (empty) third + Hint: Working copy parent: kkmpptxz b0d21db3 (empty) second + Hint: Working copy parent: qpvuntsm fa15625b (empty) first + "###) } #[test] @@ -594,7 +633,8 @@ 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 the working copy parents(s) + Hint: Working copy parent: zsuskuln 9d7e5e99 (empty) fourth "###); } @@ -826,14 +866,17 @@ 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 the working copy parent(s) + Hint: Working copy parent: zzzzzzzz 00000000 (empty) (no description set) "###); 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 the working copy + Hint: Working copy: rlvkpnrz da992bf2 (conflict) (no description set) "###); } @@ -893,6 +936,11 @@ fn test_movement_edit_mode_true() { ◆ 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###" @@ -924,12 +972,12 @@ fn test_movement_edit_mode_true() { 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 ├─╯ @@ -940,18 +988,24 @@ fn test_movement_edit_mode_true() { 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 the working copy + Hint: Working copy: xtnwkqum 7ac7a1c4 (empty) (no description set) + "###); } #[test] @@ -1000,15 +1054,21 @@ fn test_movement_edit_mode_false() { ◆ 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 the working copy parents(s) + Hint: Working copy parent: qpvuntsm fa15625b (empty) first + "###); + 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 @@ -1019,18 +1079,10 @@ fn test_movement_edit_mode_false() { 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###"