From 9fbf09ca0286d22425bce28163d7df03ebad1954 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 | 55 +++++++++++++++++++- cli/tests/test_rebase_command.rs | 87 ++++++++++++++++++++++++++++++++ 2 files changed, 140 insertions(+), 2 deletions(-) diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs index 92f2868738..bdbb7290d8 100644 --- a/cli/src/commands/rebase.rs +++ b/cli/src/commands/rebase.rs @@ -128,7 +128,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("target").args(&["destination", "insert_after", "insert_before"]).required(true)))] +#[command(group(ArgGroup::new("target").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) @@ -171,6 +171,7 @@ pub(crate) struct RebaseArgs { long, short = 'A', visible_alias = "after", + conflicts_with = "destination", conflicts_with = "source", conflicts_with = "branch" )] @@ -183,6 +184,7 @@ pub(crate) struct RebaseArgs { long, short = 'B', visible_alias = "before", + conflicts_with = "destination", conflicts_with = "source", conflicts_with = "branch" )] @@ -237,7 +239,20 @@ 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() { + let after_commits = + workspace_command.resolve_some_revsets_default_single(&args.insert_after)?; + let before_commits = + workspace_command.resolve_some_revsets_default_single(&args.insert_before)?; + rebase_revision_after_before( + ui, + command.settings(), + &mut workspace_command, + &after_commits, + &before_commits, + rev_arg, + )?; + } else if !args.insert_after.is_empty() { let after_commits = workspace_command.resolve_some_revsets_default_single(&args.insert_after)?; rebase_revision_after( @@ -553,6 +568,42 @@ fn rebase_revision_before( ) } +fn rebase_revision_after_before( + ui: &mut Ui, + settings: &UserSettings, + workspace_command: &mut WorkspaceCommandHelper, + after_commits: &IndexSet, + before_commits: &IndexSet, + rev_arg: &RevisionArg, +) -> Result<(), CommandError> { + let old_commit = workspace_command.resolve_single_rev(rev_arg)?; + workspace_command.check_rewritable([old_commit.id()])?; + let before_commit_ids = before_commits.iter().ids().cloned().collect_vec(); + workspace_command.check_rewritable(&before_commit_ids)?; + + let after_commit_ids = after_commits.iter().ids().cloned().collect_vec(); + let new_children_expression = RevsetExpression::commits(before_commit_ids); + let new_parents_expression = RevsetExpression::commits(after_commit_ids.clone()); + + ensure_no_commit_loop( + workspace_command.repo().as_ref(), + &new_children_expression, + &new_parents_expression, + )?; + + let new_parent_ids = after_commit_ids; + let new_children = before_commits.iter().cloned().collect_vec(); + + move_commit( + ui, + settings, + workspace_command, + &new_parent_ids, + &new_children, + old_commit, + ) +} + /// Extracts `commit` from the graph by rebasing its children onto its parents. /// This assumes that `commit` can be rewritten. fn extract_commit( diff --git a/cli/tests/test_rebase_command.rs b/cli/tests/test_rebase_command.rs index 0c8e610c27..4561b5ea2c 100644 --- a/cli/tests/test_rebase_command.rs +++ b/cli/tests/test_rebase_command.rs @@ -1350,6 +1350,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 4 commits + Working copy now at: lylxulpl 4aa9c561 f | f + Parent commit : kmkuslsw 288ca227 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 4aa9c561 + ◉ e kmkuslsw 288ca227 + ◉ c vruxwmqv 583a5164 + ◉ d znkkpsqq 1ca86c56 + ├─╮ + │ ◉ b2 royxmykx 903ab0d6 + ◉ │ b1 zsuskuln 072d5ae1 + ├─╯ + ◉ 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 044de9f6 f | f + Parent commit : znkkpsqq 1ca86c56 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 c3183134 + ├─╮ + │ @ f lylxulpl 044de9f6 + ◉ │ c vruxwmqv 583a5164 + ├─╯ + ◉ d znkkpsqq 1ca86c56 + ├─╮ + │ ◉ b2 royxmykx 903ab0d6 + ◉ │ b1 zsuskuln 072d5ae1 + ├─╯ + ◉ 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 1ca86c56c601 would be both an ancestor and a descendant of the rebased commit + "###); +} + #[test] fn test_rebase_skip_empty() { let test_env = TestEnvironment::default();