diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs index 1a66aa9bde..a6310f1b35 100644 --- a/cli/src/commands/rebase.rs +++ b/cli/src/commands/rebase.rs @@ -14,6 +14,7 @@ use std::collections::HashMap; use std::io::Write; +use std::rc::Rc; use std::sync::Arc; use clap::ArgGroup; @@ -123,6 +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)))] pub(crate) struct RebaseArgs { /// Rebase the whole branch relative to destination's ancestors (can be /// repeated) @@ -155,8 +157,32 @@ pub(crate) struct RebaseArgs { revision: Option, /// The revision(s) to rebase onto (can be repeated to create a merge /// commit) - #[arg(long, short, required = true)] + #[arg(long, short)] destination: Vec, + /// The revision(s) to insert after (can be repeated to create a merge + /// commit) + /// + /// Only works with `-r`. + #[arg( + long, + short = 'A', + visible_alias = "after", + conflicts_with = "source", + conflicts_with = "branch" + )] + insert_after: Vec, + /// The revision(s) to insert before (can be repeated to create a merge + /// commit) + /// + /// Only works with `-r`. + #[arg( + long, + short = 'B', + visible_alias = "before", + conflicts_with = "source", + conflicts_with = "branch" + )] + insert_before: Vec, /// If true, when rebasing would produce an empty commit, the commit is /// abandoned. It will not be abandoned if it was already empty before the @@ -210,13 +236,38 @@ Please use `jj rebase -d 'all:x|y'` instead of `jj rebase --allow-large-revsets EmptyBehaviour::Keep, "clap should forbid `-r --skip-empty`" ); - rebase_revision( - ui, - command.settings(), - &mut workspace_command, - &new_parents, - rev_str, - )?; + if !args.insert_after.is_empty() { + let after_commits = cli_util::resolve_all_revs(&workspace_command, &args.insert_after)? + .into_iter() + .collect_vec(); + rebase_revision_after( + ui, + command.settings(), + &mut workspace_command, + after_commits, + rev_str, + )?; + } else if !args.insert_before.is_empty() { + let before_commits = + cli_util::resolve_all_revs(&workspace_command, &args.insert_before)? + .into_iter() + .collect_vec(); + rebase_revision_before( + ui, + command.settings(), + &mut workspace_command, + before_commits, + rev_str, + )?; + } else { + rebase_revision( + ui, + command.settings(), + &mut workspace_command, + &new_parents, + rev_str, + )?; + } } else if !args.source.is_empty() { let source_commits = resolve_multiple_nonempty_revsets_default_single(&workspace_command, &args.source)?; @@ -416,6 +467,239 @@ fn rebase_revision( } } +fn rebase_revision_after( + ui: &mut Ui, + settings: &UserSettings, + workspace_command: &mut WorkspaceCommandHelper, + after_commits: Vec, + rev_str: &str, +) -> Result<(), CommandError> { + let old_commit = workspace_command.resolve_single_rev(rev_str)?; + workspace_command.check_rewritable([&old_commit])?; + + let after_commit_ids = after_commits + .iter() + .map(|commit| commit.id().clone()) + .collect(); + let new_parents_expression = RevsetExpression::commits(after_commit_ids); + let new_children_expression = new_parents_expression.children(); + + ensure_no_commit_loop( + workspace_command.repo().as_ref(), + &new_children_expression, + &new_parents_expression, + )?; + + let new_parents = after_commits; + + let new_children: Vec = new_children_expression + .evaluate_programmatic(workspace_command.repo().as_ref())? + .iter() + .commits(workspace_command.repo().store()) + .try_collect()?; + workspace_command.check_rewritable(&new_children)?; + + 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, + ) +} + +fn rebase_revision_before( + ui: &mut Ui, + settings: &UserSettings, + workspace_command: &mut WorkspaceCommandHelper, + 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 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 = new_children_expression.parents(); + + ensure_no_commit_loop( + workspace_command.repo().as_ref(), + &new_children_expression, + &new_parents_expression, + )?; + + let new_parents: Vec = new_parents_expression + // Exclude parents that are ancestors of each other. + .minus(&new_parents_expression.parents().ancestors()) + .clone() + .evaluate_programmatic(workspace_command.repo().as_ref())? + .iter() + .commits(workspace_command.repo().store()) + .try_collect()?; + 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, + ) +} + +fn move_revision_compute_parent_children( + workspace_command: &mut WorkspaceCommandHelper, + old_commit: &Commit, + new_parents_expression: &Rc, + new_parents: Vec, + new_children: Vec, +) -> Result<(Vec, Vec<(Commit, Vec)>), CommandError> { + let new_parents: Vec<_> = new_parents + .into_iter() + .flat_map(|c| { + if &c == old_commit { + old_commit.parents().clone() + } else { + [c].to_vec() + } + }) + .collect(); + + let old_commit_expression = RevsetExpression::commit(old_commit.id().clone()); + let old_commit_children: Vec = old_commit_expression + .children() + .evaluate_programmatic(workspace_command.repo().as_ref())? + .iter() + .commits(workspace_command.repo().store()) + .try_collect()?; + + let new_children: Vec<_> = new_children + .into_iter() + .flat_map(|c| { + if &c == old_commit { + old_commit_children.clone() + } else { + [c].to_vec() + } + }) + .collect(); + + let new_children_to_rebase: Vec<_> = new_children + .into_iter() + .map(|child_commit| { + let new_child_parents_expression = + RevsetExpression::commits(child_commit.parent_ids().to_owned()) + .minus(&new_parents_expression) + .minus(&old_commit_expression); + + let new_child_parents: Vec = new_child_parents_expression + .evaluate_programmatic(workspace_command.repo().as_ref()) + .unwrap() + .iter() + .commits(workspace_command.repo().store()) + .try_collect()?; + + Ok::<_, BackendError>((child_commit, new_child_parents)) + }) + .try_collect()?; + + Ok((new_parents, new_children_to_rebase)) +} + +/// Moves `commit` from its current location to a new location, given by the set +/// of `new_parents` and `new_children_with_parents`. +fn move_commit( + ui: &mut Ui, + settings: &UserSettings, + workspace_command: &mut WorkspaceCommandHelper, + new_parents: &[Commit], + new_children_with_parents: &[(Commit, Vec)], + commit: Commit, +) -> Result<(), CommandError> { + let mut tx = workspace_command.start_transaction(); + // Extract `commit` from its previous location by rebasing its children + // onto its parents. + let (mut rebased_commit_ids, num_rebased_descendants) = + extract_commit(&mut tx, settings, &commit)?; + + // We now update `new_parents` to account for the rebase of all of + // `commit`'s descendants. Even if some of the original `new_parents` + // were descendants of `old_commit`, this will no longer be the case after + // the update. + // + // See comment in `rebase_revision` for the bug in this code. + let new_parents: Vec<_> = new_parents + .iter() + .map(|new_parent| get_possibly_rewritten_commit(&mut tx, &rebased_commit_ids, new_parent)) + .try_collect()?; + + // Finally, it's safe to rebase `commit`. We can skip rebasing if it is + // already a child of `new_parents`. Otherwise, at this point, it should no + // longer have any children; they have all been rebased and the originals + // have been abandoned. + let new_commit = if commit.parents() == new_parents { + write!(ui.stderr(), "Skipping rebase of commit ")?; + tx.write_commit_summary(ui.stderr_formatter().as_mut(), &commit)?; + writeln!(ui.stderr())?; + commit.clone() + } else { + let new_commit = rebase_commit(settings, tx.mut_repo(), &commit, &new_parents)?; + rebased_commit_ids.insert(commit.id().clone(), new_commit.id().clone()); + new_commit + }; + + // Now, rebase all the new children onto the newly rebased commit. + let new_children_with_parents: Vec<_> = new_children_with_parents + .iter() + .map(|(commit, parents)| { + // The child commit itself could have been rewritten. + let commit = get_possibly_rewritten_commit(&mut tx, &rebased_commit_ids, commit)?; + + let mut new_parents: Vec<_> = parents + .iter() + .map(|parent| get_possibly_rewritten_commit(&mut tx, &rebased_commit_ids, parent)) + .try_collect()?; + new_parents.push(new_commit.clone()); + + Ok::<_, BackendError>((commit, new_parents)) + }) + .try_collect()?; + + let (_, new_num_rebased_descendants) = + rebase_onto_new_parents(&mut tx, settings, &new_children_with_parents)?; + + // TODO: the number of descendants isn't quite right. Do we care if counts are + // repeated? + let num_rebased_descendants = num_rebased_descendants + new_num_rebased_descendants; + if num_rebased_descendants > 0 { + writeln!(ui.stderr(), "Rebased {num_rebased_descendants} commits")?; + } + if tx.mut_repo().has_changes() { + tx.finish(ui, format!("move commit {}", commit.id().hex())) + } else { + Ok(()) // Do not print "Nothing changed." + } +} + /// Extracts `commit` from the graph by rebasing its children on its parents. /// This assumes that it can be rewritten. fn extract_commit( @@ -517,6 +801,28 @@ fn get_possibly_rewritten_commit( }) } +/// Ensure that there is no possible cycle between the potential children and +/// parents of a rebased commit. +fn ensure_no_commit_loop( + repo: &ReadonlyRepo, + children_expression: &Rc, + parents_expression: &Rc, +) -> Result<(), CommandError> { + if let Some(commit_id) = children_expression + .dag_range_to(parents_expression) + .evaluate_programmatic(repo)? + .iter() + .next() + { + return Err(user_error(format!( + "Refusing to create a loop: commit {} would be both an ancestor and a descendant of \ + the rebased commit", + short_commit_hash(&commit_id), + ))); + } + Ok(()) +} + fn check_rebase_destinations( repo: &Arc, new_parents: &[Commit], diff --git a/cli/tests/cli-reference@.md.snap b/cli/tests/cli-reference@.md.snap index dc7d797e9f..08beef4938 100644 --- a/cli/tests/cli-reference@.md.snap +++ b/cli/tests/cli-reference@.md.snap @@ -1477,7 +1477,7 @@ J J If a working-copy commit gets abandoned, it will be given a new, empty commit. This is true in general; it is not specific to this command. -**Usage:** `jj rebase [OPTIONS] --destination ` +**Usage:** `jj rebase [OPTIONS] <--destination |--insert-after |--insert-before >` ###### **Options:** @@ -1485,6 +1485,8 @@ commit. This is true in general; it is not specific to this command. * `-s`, `--source ` — Rebase specified revision(s) together their tree of descendants (can be repeated) * `-r`, `--revision ` — Rebase only this revision, rebasing descendants onto this revision's parent(s) * `-d`, `--destination ` — The revision(s) to rebase onto (can be repeated to create a merge commit) +* `-A`, `--insert-after ` — The revision(s) to insert after (can be repeated to create a merge commit) +* `-B`, `--insert-before ` — The revision(s) to insert before (can be repeated to create a merge commit) * `--skip-empty` — If true, when rebasing would produce an empty commit, the commit is abandoned. It will not be abandoned if it was already empty before the rebase. Will never skip merge commits with multiple non-empty parents Possible values: `true`, `false` diff --git a/cli/tests/test_rebase_command.rs b/cli/tests/test_rebase_command.rs index 4f2d3b3078..cd0a67789e 100644 --- a/cli/tests/test_rebase_command.rs +++ b/cli/tests/test_rebase_command.rs @@ -41,9 +41,9 @@ fn test_rebase_invalid() { let stderr = test_env.jj_cmd_cli_error(&repo_path, &["rebase"]); insta::assert_snapshot!(stderr, @r###" error: the following required arguments were not provided: - --destination + <--destination |--insert-after |--insert-before > - Usage: jj rebase --destination + Usage: jj rebase <--destination |--insert-after |--insert-before > For more information, try '--help'. "###); @@ -54,7 +54,7 @@ fn test_rebase_invalid() { insta::assert_snapshot!(stderr, @r###" error: the argument '--revision ' cannot be used with '--source ' - Usage: jj rebase --destination --revision + Usage: jj rebase --revision <--destination |--insert-after |--insert-before > For more information, try '--help'. "###); @@ -65,7 +65,7 @@ fn test_rebase_invalid() { insta::assert_snapshot!(stderr, @r###" error: the argument '--branch ' cannot be used with '--source ' - Usage: jj rebase --destination --branch + Usage: jj rebase --branch <--destination |--insert-after |--insert-before > For more information, try '--help'. "###); @@ -78,7 +78,40 @@ fn test_rebase_invalid() { insta::assert_snapshot!(stderr, @r###" error: the argument '--revision ' cannot be used with '--skip-empty' - Usage: jj rebase --destination --revision + Usage: jj rebase --revision <--destination |--insert-after |--insert-before > + + For more information, try '--help'. + "###); + + // Both -d and --after + let stderr = test_env.jj_cmd_cli_error( + &repo_path, + &["rebase", "-r", "a", "-d", "b", "--after", "b"], + ); + insta::assert_snapshot!(stderr, @r###" + error: the argument '--destination ' cannot be used with '--insert-after ' + + Usage: jj rebase --revision <--destination |--insert-after |--insert-before > + + For more information, try '--help'. + "###); + + // -s with --after + let stderr = test_env.jj_cmd_cli_error(&repo_path, &["rebase", "-s", "a", "--after", "b"]); + insta::assert_snapshot!(stderr, @r###" + error: the argument '--source ' cannot be used with '--insert-after ' + + Usage: jj rebase --source <--destination |--insert-after |--insert-before > + + For more information, try '--help'. + "###); + + // -b with --after + let stderr = test_env.jj_cmd_cli_error(&repo_path, &["rebase", "-b", "a", "--after", "b"]); + insta::assert_snapshot!(stderr, @r###" + error: the argument '--branch ' cannot be used with '--insert-after ' + + Usage: jj rebase --branch <--destination |--insert-after |--insert-before > For more information, try '--help'. "###); @@ -1055,6 +1088,215 @@ fn test_rebase_with_child_and_descendant_bug_2600() { "###); } +#[test] +fn test_rebase_revision_after() { + 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", "--after", "e"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Rebased 4 commits + Working copy now at: lylxulpl 41d72a5b f | f + Parent commit : vruxwmqv fe35083c c | c + "###); + insta::assert_snapshot!(get_long_log_output(&test_env, &repo_path), @r###" + @ f lylxulpl 41d72a5b + ◉ c vruxwmqv fe35083c + ◉ e kmkuslsw 1f75714f + ├─╮ + │ │ ◉ 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", "--after", "e", "--after", "d"], + ); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Rebased 1 commits + Working copy now at: lylxulpl 60b88d56 f | f + Parent commit : kmkuslsw 1f75714f e | e + Parent commit : znkkpsqq f7326458 d | d + Added 1 files, modified 0 files, removed 1 files + "###); + insta::assert_snapshot!(get_long_log_output(&test_env, &repo_path), @r###" + ◉ c vruxwmqv dc516a33 + @ f lylxulpl 60b88d56 + ├─╮ + │ ◉ d znkkpsqq f7326458 + │ ├─╮ + ◉ │ │ e kmkuslsw 1f75714f + ╰─┬─╮ + │ ◉ b1 zsuskuln 072d5ae1 + ◉ │ b2 royxmykx 903ab0d6 + ├─╯ + ◉ a rlvkpnrz 2443ea76 + ◉ zzzzzzzz 00000000 + "###); + + let (stdout, stderr) = test_env.jj_cmd_ok( + &repo_path, + &["rebase", "-r", "b1", "--after", "d", "--after", "e"], + ); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Rebased 6 commits + Working copy now at: lylxulpl bef37a3d f | f + Parent commit : zsuskuln 697fcb31 b1 | b1 + "###); + insta::assert_snapshot!(get_long_log_output(&test_env, &repo_path), @r###" + ◉ c vruxwmqv 997ed500 + @ f lylxulpl bef37a3d + ◉ b1 zsuskuln 697fcb31 + ├─╮ + │ ◉ e kmkuslsw 362b37d6 + ◉ │ d znkkpsqq b9b77883 + ├─╯ + ◉ b2 royxmykx 903ab0d6 + ◉ a rlvkpnrz 2443ea76 + ◉ zzzzzzzz 00000000 + "###); + + let stderr = test_env.jj_cmd_failure( + &repo_path, + &["rebase", "-r", "e", "--after", "a", "--after", "b2"], + ); + insta::assert_snapshot!(stderr, @r###" + Error: Refusing to create a loop: commit 903ab0d6ef88 would be both an ancestor and a descendant of the rebased commit + "###); +} + +#[test] +fn test_rebase_revision_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", "b2", "--before", "a"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Rebased 10 commits + Working copy now at: lylxulpl c22409f0 f | f + Parent commit : kmkuslsw cc9972db e | e + "###); + insta::assert_snapshot!(get_long_log_output(&test_env, &repo_path), @r###" + @ f lylxulpl c22409f0 + ◉ e kmkuslsw cc9972db + │ ◉ d znkkpsqq 7569d81e + ├─╯ + ◉ c vruxwmqv 683a5e00 + ◉ b1 zsuskuln 29a5d465 + ◉ a rlvkpnrz d983e4de + ◉ b2 royxmykx 2033fe8e + ◉ zzzzzzzz 00000000 + "###); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-r", "f", "--before", "d"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Rebased 1 commits + Working copy now at: lylxulpl d82e36dc f | f + Parent commit : vruxwmqv 683a5e00 c | c + Added 0 files, modified 0 files, removed 1 files + "###); + insta::assert_snapshot!(get_long_log_output(&test_env, &repo_path), @r###" + ◉ d znkkpsqq a770c8f2 + @ f lylxulpl d82e36dc + │ ◉ e kmkuslsw cc9972db + ├─╯ + ◉ c vruxwmqv 683a5e00 + ◉ b1 zsuskuln 29a5d465 + ◉ a rlvkpnrz d983e4de + ◉ b2 royxmykx 2033fe8e + ◉ zzzzzzzz 00000000 + "###); + + let (stdout, stderr) = test_env.jj_cmd_ok( + &repo_path, + &["rebase", "-r", "b1", "--before", "d", "--before", "e"], + ); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Rebased 6 commits + Working copy now at: lylxulpl 99b469fd f | f + Parent commit : vruxwmqv 407c9dc0 c | c + Added 0 files, modified 0 files, removed 1 files + "###); + // Note that B1 only has a parent of F instead of F (D's parent) and C (E's + // parent) because C is an ancestor of F. + insta::assert_snapshot!(get_long_log_output(&test_env, &repo_path), @r###" + ◉ e kmkuslsw fe4d4c91 + │ ◉ d znkkpsqq acb09767 + ├─╯ + ◉ b1 zsuskuln 1dfc844c + @ f lylxulpl 99b469fd + ◉ c vruxwmqv 407c9dc0 + ◉ a rlvkpnrz d983e4de + ◉ b2 royxmykx 2033fe8e + ◉ 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 d983e4dea283 would be both an ancestor and a descendant of the rebased commit + "###); +} + #[test] fn test_rebase_skip_empty() { let test_env = TestEnvironment::default();