Make jj prev work when the working copy is a merge commit

Before this commit `jj prev` fails if the current working copy commit is a
merge commit. After this commit it will prompt the user to choose the ancestor
they want to select.

#2126

CHANGELOG.md

@@ -24,6 +24,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 * New command `jj parallelize` that rebases a set of revisions into siblings.
 
+* `jj prev` now works when the working copy revision is a merge.
+
 ### Fixed bugs
 
 ## [0.16.0] - 2024-04-03

cli/src/commands/prev.rs

@@ -49,7 +49,6 @@ use crate::ui::Ui;
 /// If your working-copy commit already has visible children, then `--edit` is
 /// implied.
 /// ```
-// TODO(#2126): Handle multiple parents, e.g merges.
 #[derive(clap::Args, Clone, Debug)]
 #[command(verbatim_doc_comment)]
 pub(crate) struct PrevArgs {
@@ -67,7 +66,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 +76,12 @@ 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
-    } 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()))
-    };
+    // This is the distance of the target ancestor from the current working copy
+    // revision (@). The offset provided by the user is the distance of the
+    // target ancestor from the parent(s) of the working copy revision (@-).
+    let target_ancestor_distance = if edit { args.offset } else { args.offset + 1 };
+    let target_revset =
+        RevsetExpression::commit(current_wc_id.clone()).ancestors_at(target_ancestor_distance);
     let targets: Vec<_> = target_revset
         .evaluate_programmatic(workspace_command.repo().as_ref())?
         .iter()
@@ -106,12 +91,14 @@ pub(crate) fn cmd_prev(
         [target] => target,
         [] => {
             return Err(user_error(format!(
-                "No ancestor found {offset} commit{} back",
-                if offset > 1 { "s" } else { "" }
+                "No ancestor found {} commit{} back",
+                args.offset,
+                if args.offset > 1 { "s" } else { "" }
             )))
         }
         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());

cli/tests/test_next_prev_commands.rs

@@ -227,7 +227,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");
@@ -248,9 +248,87 @@ 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_ok(&repo_path, &["prev"]);
+    insta::assert_snapshot!(stdout, @"");
+    insta::assert_snapshot!(stderr,@r###"
+    Working copy now at: vruxwmqv 41658cf4 (empty) (no description set)
+    Parent commit      : zzzzzzzz 00000000 (empty) (no description set)
+    "###);
+
+    test_env.jj_cmd_ok(&repo_path, &["undo"]);
+    let (stdout, stderr) = test_env.jj_cmd_stdin_ok(&repo_path, &["prev", "--edit"], "2\n");
+    insta::assert_snapshot!(stdout, @r###"
+    ambiguous prev commit, choose one to target:
+    1: zsuskuln b0d21db3 right | (empty) second
+    2: qpvuntsm 69542c19 left | (empty) first
+    q: quit the prompt
+    enter the index of the commit you want to target: 
+    "###);
+    insta::assert_snapshot!(stderr,@r###"
+    Working copy now at: qpvuntsm 69542c19 left | (empty) first
+    Parent commit      : zzzzzzzz 00000000 (empty) (no description set)
+    "###);
+}
+
+#[test]
+fn test_prev_on_merge_commit_with_parent_merge() {
+    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");
+    test_env.jj_cmd_ok(&repo_path, &["desc", "-m", "x"]);
+    test_env.jj_cmd_ok(&repo_path, &["new", "root()", "-m", "y"]);
+    test_env.jj_cmd_ok(
+        &repo_path,
+        &["new", "description(x)", "description(y)", "-m", "z"],
+    );
+    test_env.jj_cmd_ok(&repo_path, &["new", "root()", "-m", "1"]);
+    test_env.jj_cmd_ok(
+        &repo_path,
+        &["new", "description(z)", "description(1)", "-m", "M"],
+    );
+
+    // Check that the graph looks the way we expect.
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    @    royxmykxtrkr M
+    ├─╮
+    │ ◉  mzvwutvlkqwt 1
+    ◉ │    zsuskulnrvyr z
+    ├───╮
+    │ │ ◉  kkmpptxzrspx y
+    │ ├─╯
+    ◉ │  qpvuntsmwlqt x
+    ├─╯
+    ◉  zzzzzzzzzzzz
+    "###);
+
+    // Try to advance the working copy commit.
+    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: kkmpptxz 146d5c67 (empty) y
+    2: qpvuntsm c56e5035 (empty) x
+    3: zzzzzzzz 00000000 (empty) (no description set)
+    q: quit the prompt
+    enter the index of the commit you want to target: 
+    "###);
+    insta::assert_snapshot!(stderr,@r###"
+    Working copy now at: vruxwmqv e8ff4fa0 (empty) (no description set)
+    Parent commit      : qpvuntsm c56e5035 (empty) x
+    "###);
+
+    test_env.jj_cmd_ok(&repo_path, &["undo"]);
+    let (stdout, stderr) = test_env.jj_cmd_stdin_ok(&repo_path, &["prev", "--edit"], "2\n");
+    insta::assert_snapshot!(stdout, @r###"
+    ambiguous prev commit, choose one to target:
+    1: mzvwutvl 89b8a355 (empty) 1
+    2: zsuskuln 1ef71474 (empty) z
+    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: zsuskuln 1ef71474 (empty) z
+    Parent commit      : qpvuntsm c56e5035 (empty) x
+    Parent commit      : kkmpptxz 146d5c67 (empty) y
     "###);
 }