From 8bf886cf7a45d4ca965f82568210bfa9186e0496 Mon Sep 17 00:00:00 2001 From: Benjamin Tan Date: Sat, 30 Mar 2024 04:57:06 +0800 Subject: [PATCH] rebase: allow both `--insert-after` and `--insert-before` to be used simultaneously --- cli/src/commands/rebase.rs | 71 +++++++++++++++++++++++++- cli/tests/test_rebase_command.rs | 87 ++++++++++++++++++++++++++++++++ 2 files changed, 156 insertions(+), 2 deletions(-) diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs index d4ab0cc561..4dfcb54bdb 100644 --- a/cli/src/commands/rebase.rs +++ b/cli/src/commands/rebase.rs @@ -124,7 +124,7 @@ use crate::ui::Ui; #[derive(clap::Args, Clone, Debug)] #[command(verbatim_doc_comment)] #[command(group(ArgGroup::new("to_rebase").args(&["branch", "source", "revision"])))] -#[command(group(ArgGroup::new("order").args(&["destination", "insert_after", "insert_before"]).required(true)))] +#[command(group(ArgGroup::new("order").args(&["destination", "insert_after", "insert_before"]).multiple(true).required(true)))] pub(crate) struct RebaseArgs { /// Rebase the whole branch relative to destination's ancestors (can be /// repeated) @@ -167,6 +167,7 @@ pub(crate) struct RebaseArgs { long, short = 'A', visible_alias = "after", + conflicts_with = "destination", conflicts_with = "source", conflicts_with = "branch" )] @@ -179,6 +180,7 @@ pub(crate) struct RebaseArgs { long, short = 'B', visible_alias = "before", + conflicts_with = "destination", conflicts_with = "source", conflicts_with = "branch" )] @@ -237,8 +239,29 @@ Please use `jj rebase -d 'all:x|y'` instead of `jj rebase --allow-large-revsets EmptyBehaviour::Keep, "clap should forbid `-r --skip-empty`" ); - if !args.insert_after.is_empty() { + if !args.insert_after.is_empty() && !args.insert_before.is_empty() { // TODO: Use `IndexSet`? + let after_commits = resolve_multiple_nonempty_revsets_default_single( + &workspace_command, + &args.insert_after, + )? + .into_iter() + .collect_vec(); + let before_commits = resolve_multiple_nonempty_revsets_default_single( + &workspace_command, + &args.insert_before, + )? + .into_iter() + .collect_vec(); + rebase_revision_after_before( + ui, + command.settings(), + &mut workspace_command, + after_commits, + before_commits, + rev_str, + )?; + } else if !args.insert_after.is_empty() { let after_commits = resolve_multiple_nonempty_revsets_default_single( &workspace_command, &args.insert_after, @@ -574,6 +597,50 @@ fn rebase_revision_before( ) } +fn rebase_revision_after_before( + ui: &mut Ui, + settings: &UserSettings, + workspace_command: &mut WorkspaceCommandHelper, + after_commits: Vec, + before_commits: Vec, + rev_str: &str, +) -> Result<(), CommandError> { + let old_commit = workspace_command.resolve_single_rev(rev_str)?; + workspace_command.check_rewritable([&old_commit])?; + workspace_command.check_rewritable(&before_commits)?; + + let after_commit_ids = after_commits.iter().map(|c| c.id().clone()).collect_vec(); + let before_commit_ids = before_commits.iter().map(|c| c.id().clone()).collect_vec(); + let new_children_expression = RevsetExpression::commits(before_commit_ids); + let new_parents_expression = RevsetExpression::commits(after_commit_ids); + + ensure_no_commit_loop( + workspace_command.repo().as_ref(), + &new_children_expression, + &new_parents_expression, + )?; + + let new_parents = after_commits; + let new_children = before_commits; + + let (new_parents, new_children_with_parents) = move_revision_compute_parent_children( + workspace_command, + &old_commit, + &new_parents_expression, + new_parents, + new_children, + )?; + + move_commit( + ui, + settings, + workspace_command, + &new_parents, + &new_children_with_parents, + old_commit, + ) +} + type NewParentsAndChildren = (Vec, Vec<(Commit, Vec)>); fn move_revision_compute_parent_children( diff --git a/cli/tests/test_rebase_command.rs b/cli/tests/test_rebase_command.rs index 2c3f01bba8..50816775e9 100644 --- a/cli/tests/test_rebase_command.rs +++ b/cli/tests/test_rebase_command.rs @@ -1321,6 +1321,93 @@ fn test_rebase_revision_before() { "###); } +#[test] +fn test_rebase_revision_after_before() { + 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"); + + create_commit(&test_env, &repo_path, "a", &[]); + create_commit(&test_env, &repo_path, "b1", &["a"]); + create_commit(&test_env, &repo_path, "b2", &["a"]); + create_commit(&test_env, &repo_path, "c", &["b1", "b2"]); + create_commit(&test_env, &repo_path, "d", &["c"]); + create_commit(&test_env, &repo_path, "e", &["c"]); + create_commit(&test_env, &repo_path, "f", &["e"]); + // Test the setup + insta::assert_snapshot!(get_long_log_output(&test_env, &repo_path), @r###" + @ f lylxulpl 88f778c5 + ◉ e kmkuslsw 48dd9e3f + │ ◉ d znkkpsqq 92438fc9 + ├─╯ + ◉ c vruxwmqv c41e416e + ├─╮ + │ ◉ b2 royxmykx 903ab0d6 + ◉ │ b1 zsuskuln 072d5ae1 + ├─╯ + ◉ a rlvkpnrz 2443ea76 + ◉ zzzzzzzz 00000000 + "###); + + let (stdout, stderr) = test_env.jj_cmd_ok( + &repo_path, + &["rebase", "-r", "c", "--before", "e", "--after", "d"], + ); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Rebased 6 commits + Working copy now at: lylxulpl cef45f4f f | f + Parent commit : kmkuslsw 0accb3c3 e | e + Added 1 files, modified 0 files, removed 0 files + "###); + insta::assert_snapshot!(get_long_log_output(&test_env, &repo_path), @r###" + @ f lylxulpl cef45f4f + ◉ e kmkuslsw 0accb3c3 + ◉ c vruxwmqv 80233f8e + ◉ d znkkpsqq f7326458 + ├─╮ + │ ◉ b1 zsuskuln 072d5ae1 + ◉ │ b2 royxmykx 903ab0d6 + ├─╯ + ◉ a rlvkpnrz 2443ea76 + ◉ zzzzzzzz 00000000 + "###); + + let (stdout, stderr) = test_env.jj_cmd_ok( + &repo_path, + &["rebase", "-r", "f", "--before", "e", "--after", "d"], + ); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Rebased 2 commits + Working copy now at: lylxulpl 715fd8c6 f | f + Parent commit : znkkpsqq f7326458 d | d + Added 0 files, modified 0 files, removed 2 files + "###); + insta::assert_snapshot!(get_long_log_output(&test_env, &repo_path), @r###" + ◉ e kmkuslsw 50b57734 + ├─╮ + │ @ f lylxulpl 715fd8c6 + ◉ │ c vruxwmqv 80233f8e + ├─╯ + ◉ d znkkpsqq f7326458 + ├─╮ + │ ◉ b1 zsuskuln 072d5ae1 + ◉ │ b2 royxmykx 903ab0d6 + ├─╯ + ◉ a rlvkpnrz 2443ea76 + ◉ zzzzzzzz 00000000 + "###); + + let stderr = test_env.jj_cmd_failure( + &repo_path, + &["rebase", "-r", "e", "--before", "b2", "--before", "c"], + ); + insta::assert_snapshot!(stderr, @r###" + Error: Refusing to create a loop: commit f7326458ae0e would be both an ancestor and a descendant of the rebased commit + "###); +} + #[test] fn test_rebase_skip_empty() { let test_env = TestEnvironment::default();