From 158e6efe51751739e81bf9178d96b503e0e063c1 Mon Sep 17 00:00:00 2001 From: Evan Mesterhazy Date: Tue, 2 Apr 2024 17:27:02 -0400 Subject: [PATCH] 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 | 2 + cli/src/commands/prev.rs | 33 ++++------- cli/tests/test_next_prev_commands.rs | 84 ++++++++++++++++++++++++++-- 3 files changed, 93 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index edacc3cb46..f8e22d2405 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * File path arguments now support [file pattern syntax](docs/filesets.md#file-patterns). +* `jj prev` now works when the working copy revision is a merge. + * Operation objects in templates now have a `snapshot() -> Boolean` method that evaluates to true if the operation was a snapshot created by a non-mutating command (e.g. `jj log`). diff --git a/cli/src/commands/prev.rs b/cli/src/commands/prev.rs index 11c84efe98..703e947d8a 100644 --- a/cli/src/commands/prev.rs +++ b/cli/src/commands/prev.rs @@ -50,7 +50,6 @@ use crate::ui::Ui; /// ``` /// If the working copy revision 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 { @@ -68,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 +76,14 @@ pub(crate) fn cmd_prev( .view() .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); + // If we're editing, start at the working-copy commit. Otherwise, start from + // its direct parent(s). let target_revset = if edit { - ancestor_expression + RevsetExpression::commit(current_wc_id.clone()).ancestors_at(args.offset) } 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())) + RevsetExpression::commit(current_wc_id.clone()) + .parents() + .ancestors_at(args.offset) }; let targets: Vec<_> = target_revset .evaluate_programmatic(workspace_command.repo().as_ref())? @@ -107,14 +94,16 @@ 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 current_short = short_commit_hash(current_wc_id); let target_short = short_commit_hash(target.id()); // If we're editing, just move to the revision directly. if edit { diff --git a/cli/tests/test_next_prev_commands.rs b/cli/tests/test_next_prev_commands.rs index 91ee0dacc0..7dd88002de 100644 --- a/cli/tests/test_next_prev_commands.rs +++ b/cli/tests/test_next_prev_commands.rs @@ -228,7 +228,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,10 +248,86 @@ fn test_prev_fails_on_merge_commit() { ◉ zzzzzzzzzzzz "###); - // 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 + "###); + + 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 "###); }