From b3095271a2bc31c83a4ed3b778975280c01be9c7 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Sat, 18 Feb 2023 19:31:48 -0800 Subject: [PATCH] cmd: Allow multiple `-s` for `jj rebase` This addresses a portion of #1158 --- CHANGELOG.md | 3 + src/commands/mod.rs | 56 ++++++++++---- tests/test_rebase_command.rs | 140 +++++++++++++++++++++++++++++++++++ 3 files changed, 184 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 25ee5ee56ec..1c6cbed71a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### New features +* `jj rebase` now accepts multiple `-s` arguments. Revsets with multiple commits + are allowed with `--allow-large-revsets`. + ### Fixed bugs * Modify/delete conflicts now include context lines diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 3b4ae90f479..1b224812661 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -731,9 +731,9 @@ struct RebaseArgs { /// Rebase the whole branch (relative to destination's ancestors) #[arg(long, short)] branch: Option, - /// Rebase this revision and its descendants + /// Rebase this revision and its descendants (can be repeated) #[arg(long, short)] - source: Option, + source: Vec, /// Rebase only this revision, rebasing descendants onto this revision's /// parent(s) #[arg(long, short)] @@ -2757,13 +2757,18 @@ fn cmd_rebase(ui: &mut Ui, command: &CommandHelper, args: &RebaseArgs) -> Result .collect_vec(); if let Some(rev_str) = &args.revision { rebase_revision(ui, command, &mut workspace_command, &new_parents, rev_str)?; - } else if let Some(source_str) = &args.source { + } else if !args.source.is_empty() { + let source_commits = resolve_mutliple_nonempty_revsets_flag_guarded( + &workspace_command, + &args.source, + args.allow_large_revsets, + )?; rebase_descendants( ui, command, &mut workspace_command, &new_parents, - source_str, + &source_commits, )?; } else { let branch_str = args.branch.as_deref().unwrap_or("@"); @@ -2820,18 +2825,39 @@ fn rebase_descendants( command: &CommandHelper, workspace_command: &mut WorkspaceCommandHelper, new_parents: &[Commit], - source_str: &str, + old_commits: &IndexSet, ) -> Result<(), CommandError> { - let old_commit = workspace_command.resolve_single_rev(source_str)?; - workspace_command.check_rewritable(&old_commit)?; - check_rebase_destinations(workspace_command.repo(), new_parents, &old_commit)?; - let mut tx = workspace_command.start_transaction(&format!( - "rebase commit {} and descendants", - old_commit.id().hex() - )); - rebase_commit(command.settings(), tx.mut_repo(), &old_commit, new_parents)?; - let num_rebased = tx.mut_repo().rebase_descendants(command.settings())? + 1; - writeln!(ui, "Rebased {num_rebased} commits")?; + for old_commit in old_commits.iter() { + workspace_command.check_rewritable(old_commit)?; + check_rebase_destinations(workspace_command.repo(), new_parents, old_commit)?; + } + let tx_message = if old_commits.len() == 1 { + format!( + "rebase commit {} and descendants", + old_commits.first().unwrap().id().hex() + ) + } else { + format!("rebase {} commits and their descendants", old_commits.len()) + }; + let mut tx = workspace_command.start_transaction(&tx_message); + let index = tx.base_repo().readonly_index().clone(); + let store = tx.base_repo().store().clone(); + for old_commit_id in index + // `rebase_descendants` seems to work even without a topo_order, but it feels + // safer to have it. + .topo_order(&mut old_commits.iter().map(|c| c.id())) + .into_iter() + .rev() + .map(|index_entry| index_entry.commit_id()) + { + let old_commit = store.get_commit(&old_commit_id).unwrap(); + rebase_commit(command.settings(), tx.mut_repo(), &old_commit, new_parents)?; + } + writeln!( + ui, + "Rebased {num_rebased} commits", + num_rebased = old_commits.len() + tx.mut_repo().rebase_descendants(command.settings())? + )?; tx.finish(ui)?; Ok(()) } diff --git a/tests/test_rebase_command.rs b/tests/test_rebase_command.rs index 5adbd01ce97..bc6272f2660 100644 --- a/tests/test_rebase_command.rs +++ b/tests/test_rebase_command.rs @@ -426,6 +426,146 @@ fn test_rebase_with_descendants() { o a o "###); + + test_env.jj_cmd_success(&repo_path, &["undo"]); + let stdout = test_env.jj_cmd_success(&repo_path, &["rebase", "-s", "b", "-s", "d", "-d", "a"]); + insta::assert_snapshot!(stdout, @r###" + Rebased 3 commits + Working copy now at: 92c2bc9a8623 d + Added 0 files, modified 0 files, removed 2 files + "###); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + o c + o b + │ @ d + ├─╯ + o a + o + "###); + test_env.jj_cmd_success(&repo_path, &["undo"]); + let stderr = + test_env.jj_cmd_failure(&repo_path, &["rebase", "-s", "b|d", "-s", "d", "-d", "a"]); + insta::assert_snapshot!(stderr, @r###" + Error: Revset "b|d" resolved to more than one revision + Hint: The revset "b|d" resolved to these revisions: + df54a9fd85ae d + d370aee184ba b + If this was intentional, specify the `--allow-large-revsets` argument + "###); + let stdout = test_env.jj_cmd_success( + &repo_path, + &[ + "rebase", + "-s", + "b|d", + "-s", + "d", + "-d", + "a", + "--allow-large-revsets", + ], + ); + insta::assert_snapshot!(stdout, @r###" + Rebased 3 commits + Working copy now at: f1e71cb78a06 d + Added 0 files, modified 0 files, removed 2 files + "###); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + o c + o b + │ @ d + ├─╯ + o a + o + "###); + + test_env.jj_cmd_success(&repo_path, &["undo"]); + create_commit(&test_env, &repo_path, "e", &[]); + // Test the setup + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ e + │ o d + │ o c + │ ├─╮ + │ o │ b + ├─╯ │ + │ o a + ├───╯ + o + "###); + + // TODO: Simple case, will be deleted + let stdout = test_env.jj_cmd_success(&repo_path, &["rebase", "-s", "a", "-s", "b", "-d", "e"]); + insta::assert_snapshot!(stdout, @r###" + Rebased 4 commits + "###); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + o d + o c + ├─╮ + o │ a + │ o b + ├─╯ + @ e + o + "###); + let stdout = test_env.jj_cmd_success( + &repo_path, + &["rebase", "-s", "a", "-s", "c", "-s", "d", "-d", "e"], + ); + insta::assert_snapshot!(stdout, @r###" + Rebased 3 commits + "###); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + o a + │ o c + ├─╯ + │ o d + ├─╯ + │ o b + ├─╯ + @ e + o + "###); + + test_env.jj_cmd_success(&repo_path, &["undo"]); + let stderr = + test_env.jj_cmd_failure(&repo_path, &["rebase", "-s", "a|c|d", "-s", "d", "-d", "e"]); + insta::assert_snapshot!(stderr, @r###" + Error: Revset "a|c|d" resolved to more than one revision + Hint: The revset "a|c|d" resolved to these revisions: + e1a883ee648c d + a3bab23dae21 c + 2d49d96057da a + If this was intentional, specify the `--allow-large-revsets` argument + "###); + let stdout = test_env.jj_cmd_success( + &repo_path, + &[ + "rebase", + "-s", + "a|c|d", + "-s", + "d", + "-d", + "e", + "--allow-large-revsets", + ], + ); + insta::assert_snapshot!(stdout, @r###" + Rebased 3 commits + "###); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + o a + │ o c + ├─╯ + │ o d + ├─╯ + │ o b + ├─╯ + @ e + o + "###); } fn get_log_output(test_env: &TestEnvironment, repo_path: &Path) -> String {