diff --git a/cli/src/commands/prev.rs b/cli/src/commands/prev.rs index 7e756e7ae2f..ed5f44d3056 100644 --- a/cli/src/commands/prev.rs +++ b/cli/src/commands/prev.rs @@ -67,7 +67,6 @@ pub(crate) fn cmd_prev( args: &PrevArgs, ) -> Result<(), CommandError> { let mut workspace_command = command.workspace_helper(ui)?; - let offset = args.offset; let current_wc_id = workspace_command .get_wc_commit_id() .ok_or_else(|| user_error("This command requires a working copy"))?; @@ -78,25 +77,17 @@ pub(crate) fn cmd_prev( .heads() .contains(current_wc_id); let current_wc = workspace_command.repo().store().get_commit(current_wc_id)?; - let start_id = if edit { - current_wc_id + let offset = if current_wc.is_discardable() && !args.edit { + // If the current working copy is discardable and the user didn't + // specify --edit, then we move one commit further back. This is so that + // the new empty working commit is created on the parent of the current + // on (or further back depending on the value of args.amount). If we + // didn't do this, then we wouldn't move far enough. + args.offset + 1 } else { - match current_wc.parent_ids() { - [parent_id] => parent_id, - _ => return Err(user_error("Cannot run `jj prev` on a merge commit")), - } - }; - let ancestor_expression = RevsetExpression::commit(start_id.clone()).ancestors_at(offset); - let target_revset = if edit { - ancestor_expression - } else { - // Jujutsu will always create a new commit for prev, even where Mercurial cannot - // and fails. The decision and all discussion around it are available - // here: https://github.com/martinvonz/jj/pull/1200#discussion_r1298623933 - // - // If users ever request erroring out, add `.ancestors()` to the revset below. - ancestor_expression.minus(&RevsetExpression::commit(current_wc_id.clone())) + args.offset }; + let target_revset = RevsetExpression::commit(current_wc_id.clone()).ancestors_at(offset); let targets: Vec<_> = target_revset .evaluate_programmatic(workspace_command.repo().as_ref())? .iter() @@ -105,13 +96,11 @@ pub(crate) fn cmd_prev( let target = match targets.as_slice() { [target] => target, [] => { - return Err(user_error(format!( - "No ancestor found {offset} commit{} back", - if offset > 1 { "s" } else { "" } - ))) + return Err(user_error("No ancestor found at requested offset.")); } commits => choose_commit(ui, &workspace_command, "prev", commits)?, }; + // Generate a short commit hash, to make it readable in the op log. let current_short = short_commit_hash(current_wc.id()); let target_short = short_commit_hash(target.id()); diff --git a/cli/tests/test_next_prev_commands.rs b/cli/tests/test_next_prev_commands.rs index 136d37d11b9..9bcb24baafd 100644 --- a/cli/tests/test_next_prev_commands.rs +++ b/cli/tests/test_next_prev_commands.rs @@ -112,14 +112,25 @@ fn test_prev_non_discardable_working_copy() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["prev"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" - Working copy now at: royxmykx f04d8595 (empty) (no description set) - Parent commit : qpvuntsm 69542c19 (empty) first + Working copy now at: royxmykx 5647d685 (empty) (no description set) + Parent commit : rlvkpnrz 5c52832c (empty) second + "###); + + // Since the user didn't specify --edit, @ is now sitting at a new empty + // child of second. + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ royxmykxtrkr + │ ◉ kkmpptxzrspx third + ├─╯ + ◉ rlvkpnrzqnoo second + ◉ qpvuntsmwlqt first + ◉ zzzzzzzzzzzz "###); } #[test] fn test_prev_multiple_without_root() { - // Move from fourth => first. + // Move from fourth => second. 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"); @@ -139,8 +150,18 @@ fn test_prev_multiple_without_root() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["prev", "2"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" - Working copy now at: vruxwmqv bcea672c (empty) (no description set) - Parent commit : qpvuntsm 69542c19 (empty) first + Working copy now at: vruxwmqv 6c3e8d2a (empty) (no description set) + Parent commit : rlvkpnrz 5c52832c (empty) second + "###); + + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ vruxwmqvtpmx + │ ◉ zsuskulnrvyr fourth + │ ◉ kkmpptxzrspx third + ├─╯ + ◉ rlvkpnrzqnoo second + ◉ qpvuntsmwlqt first + ◉ zzzzzzzzzzzz "###); } @@ -257,7 +278,7 @@ fn test_next_choose_branching_child() { } #[test] -fn test_prev_fails_on_merge_commit() { +fn test_prev_on_merge_commit() { 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"); @@ -279,9 +300,17 @@ fn test_prev_fails_on_merge_commit() { "###); // Try to advance the working copy commit. - let stderr = test_env.jj_cmd_failure(&repo_path, &["prev"]); + let (stdout, stderr) = test_env.jj_cmd_stdin_ok(&repo_path, &["prev"], "2\n"); + insta::assert_snapshot!(stdout,@r###" + ambiguous prev commit, choose one to target: + 1: zsuskuln edad76e9 right | (empty) second + 2: qpvuntsm 5ae1a6a5 left | (empty) first + q: quit the prompt + enter the index of the commit you want to target: + "###); insta::assert_snapshot!(stderr,@r###" - Error: Cannot run `jj prev` on a merge commit + Working copy now at: yostqsxw a9de0711 (empty) (no description set) + Parent commit : qpvuntsm 5ae1a6a5 left | (empty) first "###); } @@ -348,7 +377,7 @@ fn test_prev_onto_root_fails() { // The root commit is before "first", which is 5 commits back. 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 at requested offset. "###); }