From 932e59d2e0f30b8ee0e57639d70724b5b7af0dc9 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 | 89 +++++++++++++++++++++++++++++++- 2 files changed, 141 insertions(+), 3 deletions(-) diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs index 6c1c9cf8ec..b52c5223e5 100644 --- a/cli/src/commands/rebase.rs +++ b/cli/src/commands/rebase.rs @@ -126,7 +126,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("dest").args(&["destination", "insert_after", "insert_before"]).required(true)))] +#[command(group(ArgGroup::new("dest").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) @@ -169,6 +169,7 @@ pub(crate) struct RebaseArgs { long, short = 'A', visible_alias = "after", + conflicts_with = "destination", conflicts_with = "source", conflicts_with = "branch" )] @@ -181,6 +182,7 @@ pub(crate) struct RebaseArgs { long, short = 'B', visible_alias = "before", + conflicts_with = "destination", conflicts_with = "source", conflicts_with = "branch" )] @@ -243,7 +245,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( @@ -564,6 +579,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])?; + workspace_command.check_rewritable(before_commits.iter())?; + + 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; + + move_commit( + ui, + settings, + workspace_command, + new_parents, + new_children, + old_commit, + ) +} + /// Extracts `commit` from the graph by rebasing its children on its parents. /// This assumes that it can be rewritten. fn extract_commit( diff --git a/cli/tests/test_rebase_command.rs b/cli/tests/test_rebase_command.rs index 7116cc4874..2c37c4776d 100644 --- a/cli/tests/test_rebase_command.rs +++ b/cli/tests/test_rebase_command.rs @@ -1283,7 +1283,7 @@ fn test_rebase_revision_before() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-r", "b2", "--before", "a"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" - Rebased 11 commits + Rebased 7 commits Working copy now at: lylxulpl de47a5a1 f | f Parent commit : kmkuslsw 838e25e4 e | e "###); @@ -1355,6 +1355,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 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();