diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs index 40642a4d22..ecf59218ee 100644 --- a/cli/src/commands/rebase.rs +++ b/cli/src/commands/rebase.rs @@ -162,26 +162,24 @@ pub(crate) struct RebaseArgs { /// The revision(s) to insert after (can be repeated to create a merge /// commit) /// - /// Only works with `-r`. + /// Only works with `-r` and `-s`. #[arg( long, short = 'A', visible_alias = "after", conflicts_with = "destination", - 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`. + /// Only works with `-r` and `-s`. #[arg( long, short = 'B', visible_alias = "before", conflicts_with = "destination", - conflicts_with = "source", conflicts_with = "branch" )] insert_before: Vec, @@ -247,17 +245,14 @@ Please use `jj rebase -d 'all:x|y'` instead of `jj rebase --allow-large-revsets &rebase_options, )?; } else if !args.source.is_empty() { - let new_parents = workspace_command - .resolve_some_revsets_default_single(&args.destination)? - .into_iter() - .collect_vec(); - let source_commits = workspace_command.resolve_some_revsets_default_single(&args.source)?; - rebase_descendants_transaction( + rebase_source( ui, command.settings(), &mut workspace_command, - new_parents, - &source_commits, + &args.source, + &args.destination, + &args.insert_after, + &args.insert_before, &rebase_options, )?; } else { @@ -318,6 +313,47 @@ fn rebase_revisions( ) } +fn rebase_source( + ui: &mut Ui, + settings: &UserSettings, + workspace_command: &mut WorkspaceCommandHelper, + source: &[RevisionArg], + destination: &[RevisionArg], + insert_after: &[RevisionArg], + insert_before: &[RevisionArg], + rebase_options: &RebaseOptions, +) -> Result<(), CommandError> { + let source_commits = workspace_command + .resolve_some_revsets_default_single(source)? + .into_iter() + .collect_vec(); + workspace_command.check_rewritable(source_commits.iter().ids())?; + + let (new_parents, new_children) = compute_destination( + workspace_command, + &source_commits, + destination, + insert_after, + insert_before, + true, + )?; + if !new_children.is_empty() { + for commit in source_commits.iter() { + check_rebase_destinations(workspace_command.repo(), &new_parents, commit)?; + } + } + + rebase_descendants_transaction( + ui, + settings, + workspace_command, + &new_parents.iter().ids().cloned().collect_vec(), + &new_children, + &source_commits, + rebase_options, + ) +} + fn rebase_branch( ui: &mut Ui, settings: &UserSettings, @@ -337,17 +373,23 @@ fn rebase_branch( let roots_expression = RevsetExpression::commits(parent_ids) .range(&RevsetExpression::commits(branch_commit_ids)) .roots(); - let root_commits: IndexSet<_> = roots_expression + let root_commits: Vec<_> = roots_expression .evaluate_programmatic(workspace_command.repo().as_ref()) .unwrap() .iter() .commits(workspace_command.repo().store()) .try_collect()?; + workspace_command.check_rewritable(root_commits.iter().ids())?; + for commit in root_commits.iter() { + check_rebase_destinations(workspace_command.repo(), &new_parents, commit)?; + } + rebase_descendants_transaction( ui, settings, workspace_command, - new_parents, + &new_parents.iter().ids().cloned().collect_vec(), + &[], &root_commits, &rebase_options, ) @@ -357,19 +399,15 @@ fn rebase_descendants_transaction( ui: &mut Ui, settings: &UserSettings, workspace_command: &mut WorkspaceCommandHelper, - new_parents: Vec, - target_roots: &IndexSet, + new_parent_ids: &[CommitId], + new_children: &[Commit], + target_roots: &[Commit], rebase_options: &RebaseOptions, ) -> Result<(), CommandError> { if target_roots.is_empty() { return Ok(()); } - workspace_command.check_rewritable(target_roots.iter().ids())?; - for commit in target_roots.iter() { - check_rebase_destinations(workspace_command.repo(), &new_parents, commit)?; - } - let mut tx = workspace_command.start_transaction(); let tx_description = if target_roots.len() == 1 { format!( @@ -390,8 +428,6 @@ fn rebase_descendants_transaction( .iter() .commits(tx.repo().store()) .try_collect()?; - let new_parent_ids = new_parents.iter().ids().cloned().collect_vec(); - let new_children: [Commit; 0] = []; let target_roots = target_roots.iter().ids().cloned().collect_vec(); let MoveCommitsStats { diff --git a/cli/tests/test_rebase_command.rs b/cli/tests/test_rebase_command.rs index aead7ac8d4..0e34fb9205 100644 --- a/cli/tests/test_rebase_command.rs +++ b/cli/tests/test_rebase_command.rs @@ -96,16 +96,6 @@ fn test_rebase_invalid() { 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###" @@ -129,16 +119,6 @@ fn test_rebase_invalid() { For more information, try '--help'. "###); - // -s with --before - let stderr = test_env.jj_cmd_cli_error(&repo_path, &["rebase", "-s", "a", "--before", "b"]); - insta::assert_snapshot!(stderr, @r###" - error: the argument '--source ' cannot be used with '--insert-before ' - - Usage: jj rebase --source <--destination |--insert-after |--insert-before > - - For more information, try '--help'. - "###); - // -b with --before let stderr = test_env.jj_cmd_cli_error(&repo_path, &["rebase", "-b", "a", "--before", "b"]); insta::assert_snapshot!(stderr, @r###" @@ -1298,7 +1278,7 @@ fn test_rebase_with_child_and_descendant_bug_2600() { } #[test] -fn test_rebase_revisions_after() { +fn test_rebase_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"); @@ -1699,6 +1679,61 @@ fn test_rebase_revisions_after() { "###); test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]); + // `rebase -s` of commit "c" and its descendants after itself should be a no-op. + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-s", "c", "--after", "c"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Skipped rebase of 4 commits that were already in place + Nothing changed. + "###); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ f + ◉ e + │ ◉ d + ├─╯ + ◉ c + ├─╮ + │ ◉ b4 + │ ◉ b3 + ◉ │ b2 + ◉ │ b1 + ├─╯ + ◉ a + ◉ + "###); + test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]); + + // `rebase -s` of a commit and its descendants after multiple commits. + let (stdout, stderr) = test_env.jj_cmd_ok( + &repo_path, + &["rebase", "-s", "c", "--after", "b1", "--after", "b3"], + ); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Rebased 4 commits + Working copy now at: xznxytkn a4ace41c f | f + Parent commit : nkmrtpmo c7744d08 e | e + Added 0 files, modified 0 files, removed 2 files + "###); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + ◉ b4 + ├─╮ + │ │ ◉ b2 + ╭─┬─╯ + │ @ f + │ ◉ e + ◉ │ d + ├─╯ + ◉ c + ├─╮ + │ ◉ b3 + ◉ │ b1 + ├─╯ + ◉ a + ◉ + "###); + test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]); + // Should error if a loop will be created. let stderr = test_env.jj_cmd_failure( &repo_path, @@ -1710,7 +1745,7 @@ fn test_rebase_revisions_after() { } #[test] -fn test_rebase_revisions_before() { +fn test_rebase_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"); @@ -2120,6 +2155,62 @@ fn test_rebase_revisions_before() { "###); test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]); + // `rebase -s` of commit "c" and its descendants before itself should be a + // no-op. + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-s", "c", "--before", "c"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Skipped rebase of 4 commits that were already in place + Nothing changed. + "###); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ f + ◉ e + │ ◉ d + ├─╯ + ◉ c + ├─╮ + │ ◉ b4 + │ ◉ b3 + ◉ │ b2 + ◉ │ b1 + ├─╯ + ◉ a + ◉ + "###); + test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]); + + // `rebase -s` of a commit and its descendants before multiple commits. + let (stdout, stderr) = test_env.jj_cmd_ok( + &repo_path, + &["rebase", "-s", "c", "--before", "b2", "--before", "b4"], + ); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Rebased 4 commits + Working copy now at: xznxytkn 1ca449cc f | f + Parent commit : nkmrtpmo c75237e2 e | e + Added 0 files, modified 0 files, removed 2 files + "###); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + ◉ b4 + ├─╮ + │ │ ◉ b2 + ╭─┬─╯ + │ @ f + │ ◉ e + ◉ │ d + ├─╯ + ◉ c + ├─╮ + │ ◉ b3 + ◉ │ b1 + ├─╯ + ◉ a + ◉ + "###); + test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]); + // Should error if a loop will be created. let stderr = test_env.jj_cmd_failure( &repo_path, @@ -2131,7 +2222,7 @@ fn test_rebase_revisions_before() { } #[test] -fn test_rebase_revisions_after_before() { +fn test_rebase_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"); @@ -2286,6 +2377,33 @@ fn test_rebase_revisions_after_before() { "###); test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]); + // `rebase -s` of a commit and its descendants. + let (stdout, stderr) = test_env.jj_cmd_ok( + &repo_path, + &["rebase", "-s", "c", "--before", "b1", "--after", "b2"], + ); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Rebased 4 commits + Working copy now at: lylxulpl 108f0202 f | f + Parent commit : kmkuslsw 52245d71 e | e + Added 0 files, modified 0 files, removed 1 files + "###); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + ◉ b1 + ├─┬─╮ + │ │ @ f + │ │ ◉ e + │ ◉ │ d + │ ├─╯ + │ ◉ c + │ ◉ b2 + ├─╯ + ◉ a + ◉ + "###); + test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]); + // Should error if a loop will be created. let stderr = test_env.jj_cmd_failure( &repo_path,