From 1222c78e08fb8bc61c0f579608f8dfce136fc729 Mon Sep 17 00:00:00 2001 From: Evan Mesterhazy Date: Sun, 7 Apr 2024 20:46:33 -0400 Subject: [PATCH] Make `jj next` work when the working copy is a merge commit --- CHANGELOG.md | 2 +- cli/src/commands/next.rs | 36 ++++---- cli/tests/test_next_prev_commands.rs | 127 ++++++++++++++++++++++++--- 3 files changed, 134 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f8e22d2405..a703a6dfe2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,7 +34,7 @@ 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. +* `jj prev` and `jj next` now work 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 diff --git a/cli/src/commands/next.rs b/cli/src/commands/next.rs index 19ad069ca6..e8687244a3 100644 --- a/cli/src/commands/next.rs +++ b/cli/src/commands/next.rs @@ -105,7 +105,6 @@ pub(crate) fn cmd_next( args: &NextArgs, ) -> 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"))?; @@ -115,24 +114,20 @@ pub(crate) fn cmd_next( .view() .heads() .contains(current_wc_id); - let current_wc = workspace_command.repo().store().get_commit(current_wc_id)?; - // If we're editing, start at the working-copy commit. - // Otherwise start from our direct parent. - let start_id = if edit { - current_wc_id + let wc_revset = RevsetExpression::commit(current_wc_id.clone()); + // If we're editing, start at the working-copy commit. Otherwise, start from + // its direct parent(s). + let target_revset = if edit { + wc_revset.descendants_at(args.offset) } else { - match current_wc.parent_ids() { - [parent_id] => parent_id, - _ => return Err(user_error("Cannot run `jj next` on a merge commit")), - } - }; - let descendant_expression = RevsetExpression::commit(start_id.clone()).descendants_at(offset); - let target_expression = if edit { - descendant_expression - } else { - descendant_expression.minus(&RevsetExpression::commit(current_wc_id.clone()).descendants()) + wc_revset + .parents() + .descendants_at(args.offset) + // In previous versions we subtracted `wc_revset.descendants()`. That's + // unnecessary now that --edit is implied if `@` has descendants. + .minus(&wc_revset) }; - let targets: Vec = target_expression + let targets: Vec = target_revset .evaluate_programmatic(workspace_command.repo().as_ref())? .iter() .commits(workspace_command.repo().store()) @@ -142,13 +137,14 @@ pub(crate) fn cmd_next( [] => { // We found no descendant. return Err(user_error(format!( - "No descendant found {offset} commit{} forward", - if offset > 1 { "s" } else { "" } + "No descendant found {} commit{} forward", + args.offset, + if args.offset > 1 { "s" } else { "" } ))); } commits => choose_commit(ui, &workspace_command, "next", commits)?, }; - 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()); // We're editing, just move to the target commit. if edit { diff --git a/cli/tests/test_next_prev_commands.rs b/cli/tests/test_next_prev_commands.rs index 7dd88002de..4463b3fed3 100644 --- a/cli/tests/test_next_prev_commands.rs +++ b/cli/tests/test_next_prev_commands.rs @@ -132,21 +132,128 @@ fn test_next_exceeding_history() { "###); } +// The working copy commit is a child of a "fork" with two children on each +// branch. #[test] -fn test_next_fails_on_merge_commit() { +fn test_next_parent_has_multiple_descendants() { 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, &["branch", "c", "left"]); - test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "first"]); - test_env.jj_cmd_ok(&repo_path, &["new", "@--"]); - test_env.jj_cmd_ok(&repo_path, &["branch", "c", "right"]); - test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "second"]); - test_env.jj_cmd_ok(&repo_path, &["new", "left", "right"]); - // Try to advance the working copy commit. - let stderr = test_env.jj_cmd_failure(&repo_path, &["next"]); + // Setup. + test_env.jj_cmd_ok(&repo_path, &["desc", "-m", "1"]); + test_env.jj_cmd_ok(&repo_path, &["new", "-m", "2"]); + test_env.jj_cmd_ok(&repo_path, &["new", "root()", "-m", "3"]); + test_env.jj_cmd_ok(&repo_path, &["new", "-m", "4"]); + test_env.jj_cmd_ok(&repo_path, &["edit", "description(3)"]); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + ◉ mzvwutvlkqwt 4 + @ zsuskulnrvyr 3 + │ ◉ kkmpptxzrspx 2 + │ ◉ qpvuntsmwlqt 1 + ├─╯ + ◉ zzzzzzzzzzzz + "###); + + // --edit is implied since the working copy isn't a leaf commit. + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next"]); + insta::assert_snapshot!(stdout,@r###""###); insta::assert_snapshot!(stderr,@r###" - Error: Cannot run `jj next` on a merge commit + Working copy now at: mzvwutvl 1b8531ce (empty) 4 + Parent commit : zsuskuln b1394455 (empty) 3 + "###); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ mzvwutvlkqwt 4 + ◉ zsuskulnrvyr 3 + │ ◉ kkmpptxzrspx 2 + │ ◉ qpvuntsmwlqt 1 + ├─╯ + ◉ zzzzzzzzzzzz + "###); +} + +#[test] +fn test_next_with_merge_commit_parent() { + 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"); + // Setup. + test_env.jj_cmd_ok(&repo_path, &["desc", "-m", "1"]); + test_env.jj_cmd_ok(&repo_path, &["new", "root()", "-m", "2"]); + test_env.jj_cmd_ok( + &repo_path, + &["new", "description(1)", "description(2)", "-m", "3"], + ); + test_env.jj_cmd_ok(&repo_path, &["new", "-m", "4"]); + test_env.jj_cmd_ok(&repo_path, &["prev", "0"]); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ royxmykxtrkr + │ ◉ mzvwutvlkqwt 4 + ├─╯ + ◉ zsuskulnrvyr 3 + ├─╮ + │ ◉ kkmpptxzrspx 2 + ◉ │ qpvuntsmwlqt 1 + ├─╯ + ◉ zzzzzzzzzzzz + "###); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next"]); + insta::assert_snapshot!(stdout,@r###""###); + insta::assert_snapshot!(stderr,@r###" + Working copy now at: vruxwmqv 718bbcd9 (empty) (no description set) + Parent commit : mzvwutvl cb5881ec (empty) 4 + "###); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ vruxwmqvtpmx + ◉ mzvwutvlkqwt 4 + ◉ zsuskulnrvyr 3 + ├─╮ + │ ◉ kkmpptxzrspx 2 + ◉ │ qpvuntsmwlqt 1 + ├─╯ + ◉ zzzzzzzzzzzz + "###); +} + +#[test] +fn test_next_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"); + // Setup. + test_env.jj_cmd_ok(&repo_path, &["desc", "-m", "1"]); + test_env.jj_cmd_ok(&repo_path, &["new", "root()", "-m", "2"]); + test_env.jj_cmd_ok( + &repo_path, + &["new", "description(1)", "description(2)", "-m", "3"], + ); + test_env.jj_cmd_ok(&repo_path, &["new", "-m", "4"]); + test_env.jj_cmd_ok(&repo_path, &["edit", "description(3)"]); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + ◉ mzvwutvlkqwt 4 + @ zsuskulnrvyr 3 + ├─╮ + │ ◉ kkmpptxzrspx 2 + ◉ │ qpvuntsmwlqt 1 + ├─╯ + ◉ zzzzzzzzzzzz + "###); + + // --edit is implied since the working copy is not a leaf commit. + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next"]); + insta::assert_snapshot!(stdout,@r###""###); + insta::assert_snapshot!(stderr,@r###" + Working copy now at: mzvwutvl cb5881ec (empty) 4 + Parent commit : zsuskuln 038acb86 (empty) 3 + "###); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ mzvwutvlkqwt 4 + ◉ zsuskulnrvyr 3 + ├─╮ + │ ◉ kkmpptxzrspx 2 + ◉ │ qpvuntsmwlqt 1 + ├─╯ + ◉ zzzzzzzzzzzz "###); }