From 89356aebc6453b322b663161b97f03ae6c388c56 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Tue, 16 Apr 2024 21:32:56 -0700 Subject: [PATCH] parallelize: make the command pass in more cases The checks are not needed by the new implementation, so just drop them. --- cli/src/commands/parallelize.rs | 147 +++----------------------- cli/tests/cli-reference@.md.snap | 22 ++-- cli/tests/test_parallelize_command.rs | 90 ++++++++++++---- 3 files changed, 94 insertions(+), 165 deletions(-) diff --git a/cli/src/commands/parallelize.rs b/cli/src/commands/parallelize.rs index 0de3615d0..3a2b4c595 100644 --- a/cli/src/commands/parallelize.rs +++ b/cli/src/commands/parallelize.rs @@ -12,20 +12,17 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use std::io::Write; -use std::rc::Rc; use indexmap::IndexSet; use itertools::Itertools; use jj_lib::backend::CommitId; use jj_lib::commit::{Commit, CommitIteratorExt}; -use jj_lib::repo::Repo; -use jj_lib::revset::{RevsetExpression, RevsetIteratorExt}; use tracing::instrument; use crate::cli_util::{CommandHelper, RevisionArg}; -use crate::command_error::{user_error, CommandError}; +use crate::command_error::CommandError; use crate::ui::Ui; /// Parallelize revisions by making them siblings @@ -41,18 +38,18 @@ use crate::ui::Ui; /// 0 /// ``` /// -/// Each of the target revisions is rebased onto the parents of the root(s) of -/// the target revset (not to be confused with the repo root). The children of -/// the head(s) of the target revset are rebased onto the target revisions. +/// The command effectively says "these revisions are actually independent", +/// meaning that they should no longer be ancestors/descendants of each other. +/// However, revisions outside the set that were previously ancestors of a +/// revision in the set will remain ancestors of it. For example, revision 0 +/// above remains an ancestor of both 1 and 2. Similarly, +/// revisions outside the set that were previously descendants of a revision +/// in the set will remain descendants of it. For example, revision 3 above +/// remains a descendant of both 1 and 2. /// -/// The target revset is the union of the `revisions` arguments and must satisfy -/// several conditions, otherwise the command will fail. -/// -/// 1. The heads of the target revset must have either the same children as the -/// other heads or none. -/// 2. The roots of the target revset have the same parents. -/// 3. The parents of all target revisions except the roots must also be -/// parallelized. This means that the target revisions must be connected. +/// Therefore, `jj parallelize '1 | 3'` is a no-op. That's because 2, which is +/// not in the target set, was a descendant of 1 before, so it remains a +/// descendant, and it was an ancestor of 3 before, so it remains an ancestor. #[derive(clap::Args, Clone, Debug)] #[command(verbatim_doc_comment)] pub(crate) struct ParallelizeArgs { @@ -80,11 +77,6 @@ pub(crate) fn cmd_parallelize( workspace_command.check_rewritable(target_commits.iter().ids())?; let mut tx = workspace_command.start_transaction(); - let target_revset = - RevsetExpression::commits(target_commits.iter().ids().cloned().collect_vec()); - // TODO: The checks are now unnecessary, so drop them - let _new_parents = - check_preconditions_and_get_new_parents(&target_revset, &target_commits, tx.repo())?; // New parents for commits in the target set. Since commits in the set are now // supposed to be independent, they inherit the parent's non-target parents, @@ -151,116 +143,3 @@ pub(crate) fn cmd_parallelize( tx.finish(ui, format!("parallelize {} commits", target_commits.len())) } - -/// Returns the new parents of the parallelized commits or an error if any of -/// the following preconditions are not met: -/// -/// 1. If the heads of the target revset must not have different children. -/// 2. The roots of the target revset must not have different parents. -/// 3. The parents of all target revisions except the roots must also be -/// parallelized. This means that the target revisions must be connected. -/// -/// The `target_revset` must evaluate to the commits in `target_commits` when -/// the provided `repo` is used. -fn check_preconditions_and_get_new_parents( - target_revset: &Rc, - target_commits: &[Commit], - repo: &dyn Repo, -) -> Result, CommandError> { - check_target_heads(target_revset, repo)?; - let target_roots = check_target_roots(target_revset, repo)?; - check_target_commits_are_connected(&target_roots, target_commits)?; - - // We already verified that the roots have the same parents, so we can just - // use the first root. - Ok(target_roots[0].parents()) -} - -/// Returns an error if the heads of the target revset have children which are -/// different. -fn check_target_heads( - target_revset: &Rc, - repo: &dyn Repo, -) -> Result<(), CommandError> { - let target_heads = target_revset - .heads() - .evaluate_programmatic(repo)? - .iter() - .sorted() - .collect_vec(); - if target_heads.len() == 1 { - return Ok(()); - } - let all_children: Vec = target_revset - .heads() - .children() - .evaluate_programmatic(repo)? - .iter() - .commits(repo.store()) - .try_collect()?; - // Every child must have every target head as a parent, otherwise it means - // that the target heads have different children. - for child in all_children { - let parents = child.parent_ids().iter().sorted(); - if !parents.eq(target_heads.iter()) { - return Err(user_error( - "All heads of the target revisions must have the same children.", - )); - } - } - Ok(()) -} - -/// Returns the roots of the target revset or an error if their parents are -/// different. -fn check_target_roots( - target_revset: &Rc, - repo: &dyn Repo, -) -> Result, CommandError> { - let target_roots: Vec = target_revset - .roots() - .evaluate_programmatic(repo)? - .iter() - .commits(repo.store()) - .try_collect()?; - let expected_parents = target_roots[0].parent_ids().iter().sorted().collect_vec(); - for root in target_roots[1..].iter() { - let root_parents = root.parent_ids().iter().sorted(); - if !root_parents.eq(expected_parents.iter().copied()) { - return Err(user_error( - "All roots of the target revisions must have the same parents.", - )); - } - } - Ok(target_roots) -} - -/// The target commits must be connected. The parents of every target commit -/// except the root commit must also be target commits. Returns an error if this -/// requirement is not met. -fn check_target_commits_are_connected( - target_roots: &[Commit], - target_commits: &[Commit], -) -> Result<(), CommandError> { - let target_commit_ids: HashSet = target_commits.iter().ids().cloned().collect(); - for target_commit in target_commits.iter() { - if target_roots.iter().ids().contains(target_commit.id()) { - continue; - } - for parent in target_commit.parent_ids() { - if !target_commit_ids.contains(parent) { - // We check this condition to return a more useful error to the user. - if target_commit.parent_ids().len() == 1 { - return Err(user_error( - "Cannot parallelize since the target revisions are not connected.", - )); - } - return Err(user_error( - "Only the roots of the target revset are allowed to have parents which are \ - not being parallelized.", - )); - } - } - } - Ok(()) -} diff --git a/cli/tests/cli-reference@.md.snap b/cli/tests/cli-reference@.md.snap index c3a50add6..4dd0ba62c 100644 --- a/cli/tests/cli-reference@.md.snap +++ b/cli/tests/cli-reference@.md.snap @@ -1369,18 +1369,18 @@ Running `jj parallelize 1::2` will transform the history like this: 0 ``` -Each of the target revisions is rebased onto the parents of the root(s) of -the target revset (not to be confused with the repo root). The children of -the head(s) of the target revset are rebased onto the target revisions. +The command effectively says "these revisions are actually independent", +meaning that they should no longer be ancestors/descendants of each other. +However, revisions outside the set that were previously ancestors of a +revision in the set will remain ancestors of it. For example, revision 0 +above remains an ancestor of both 1 and 2. Similarly, +revisions outside the set that were previously descendants of a revision +in the set will remain descendants of it. For example, revision 3 above +remains a descendant of both 1 and 2. -The target revset is the union of the `revisions` arguments and must satisfy -several conditions, otherwise the command will fail. - -1. The heads of the target revset must have either the same children as the - other heads or none. -2. The roots of the target revset have the same parents. -3. The parents of all target revisions except the roots must also be - parallelized. This means that the target revisions must be connected. +Therefore, `jj parallelize '1 | 3'` is a no-op. That's because 2, which is +not in the target set, was a descendant of 1 before, so it remains a +descendant, and it was an ancestor of 3 before, so it remains an ancestor. **Usage:** `jj parallelize [REVISIONS]...` diff --git a/cli/tests/test_parallelize_command.rs b/cli/tests/test_parallelize_command.rs index eb5870081..ea43756cf 100644 --- a/cli/tests/test_parallelize_command.rs +++ b/cli/tests/test_parallelize_command.rs @@ -226,7 +226,7 @@ fn test_parallelize_with_merge_commit_child() { } #[test] -fn test_parallelize_failure_disconnected_target_commits() { +fn test_parallelize_disconnected_target_commits() { let test_env = TestEnvironment::default(); test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]); let workspace_path = test_env.env_root().join("repo"); @@ -242,9 +242,19 @@ fn test_parallelize_failure_disconnected_target_commits() { ◉ 000000000000 parents: "###); - insta::assert_snapshot!(test_env.jj_cmd_failure( - &workspace_path, &["parallelize", "description(1)", "description(3)"]),@r###" - Error: Cannot parallelize since the target revisions are not connected. + let (stdout, stderr) = test_env.jj_cmd_ok( + &workspace_path, + &["parallelize", "description(1)", "description(3)"], + ); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Nothing changed. + "###); + insta::assert_snapshot!(get_log_output(&test_env, &workspace_path), @r###" + @ 9f5b59fa4622 3 parents: 2 + ◉ d826910d21fb 2 parents: 1 + ◉ dc0e5d6135ce 1 parents: + ◉ 000000000000 parents: "###); } @@ -275,9 +285,19 @@ fn test_parallelize_head_is_a_merge() { ◉ 000000000000 parents: "###); - insta::assert_snapshot!(test_env.jj_cmd_failure(&workspace_path,&["parallelize", "description(1)::"]), - @r###" - Error: Only the roots of the target revset are allowed to have parents which are not being parallelized. + test_env.jj_cmd_ok(&workspace_path, &["parallelize", "description(1)::"]); + insta::assert_snapshot!(get_log_output(&test_env, &workspace_path), @r###" + @ babb4191912d merged-head parents: 0 b + ├─╮ + │ ◉ 5164ab888473 b parents: a + │ ◉ f16fe8ac5ce9 a parents: + │ │ ◉ 36b2f866a798 2 parents: 0 + ├───╯ + │ │ ◉ a915696cf0ad 1 parents: 0 + ├───╯ + ◉ │ a56846756248 0 parents: + ├─╯ + ◉ 000000000000 parents: "###); } @@ -305,9 +325,18 @@ fn test_parallelize_interior_target_is_a_merge() { ◉ 000000000000 parents: "###); - insta::assert_snapshot!(test_env.jj_cmd_failure(&workspace_path,&["parallelize", "description(1)::"]), - @r###" - Error: Only the roots of the target revset are allowed to have parents which are not being parallelized. + test_env.jj_cmd_ok(&workspace_path, &["parallelize", "description(1)::"]); + insta::assert_snapshot!(get_log_output(&test_env, &workspace_path), @r###" + @ cd0ac6ad1415 3 parents: 0 a + ├─╮ + │ │ ◉ 1c240e875670 2 parents: 0 a + ╭─┬─╯ + │ ◉ 427890ea3f2b a parents: + │ │ ◉ a915696cf0ad 1 parents: 0 + ├───╯ + ◉ │ a56846756248 0 parents: + ├─╯ + ◉ 000000000000 parents: "###); } @@ -449,7 +478,7 @@ fn test_parallelize_multiple_roots() { } #[test] -fn test_parallelize_failure_multiple_heads_with_different_children() { +fn test_parallelize_multiple_heads_with_different_children() { let test_env = TestEnvironment::default(); test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]); let workspace_path = test_env.env_root().join("repo"); @@ -472,21 +501,33 @@ fn test_parallelize_failure_multiple_heads_with_different_children() { ◉ 000000000000 parents: "###); - insta::assert_snapshot!( - test_env.jj_cmd_failure( + test_env.jj_cmd_ok( &workspace_path, &[ "parallelize", "description(1)::description(2)", "description(a)::description(b)", ], - ),@r###" - Error: All heads of the target revisions must have the same children. + ); + insta::assert_snapshot!(get_log_output(&test_env, &workspace_path), @r###" + @ 582c6bd1e1fd parents: c + ◉ dd2db8b60a69 c parents: a b + ├─╮ + │ ◉ 190b857f6cdd b parents: + ◉ │ f16fe8ac5ce9 a parents: + ├─╯ + │ ◉ bbc313370f45 3 parents: 1 2 + │ ├─╮ + │ │ ◉ 96ce11389312 2 parents: + ├───╯ + │ ◉ dc0e5d6135ce 1 parents: + ├─╯ + ◉ 000000000000 parents: "###); } #[test] -fn test_parallelize_failure_multiple_roots_with_different_parents() { +fn test_parallelize_multiple_roots_with_different_parents() { let test_env = TestEnvironment::default(); test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]); let workspace_path = test_env.env_root().join("repo"); @@ -510,12 +551,21 @@ fn test_parallelize_failure_multiple_roots_with_different_parents() { ◉ 000000000000 parents: "###); - insta::assert_snapshot!( - test_env.jj_cmd_failure( + test_env.jj_cmd_ok( &workspace_path, &["parallelize", "description(2)::", "description(b)::"], - ),@r###" - Error: All roots of the target revisions must have the same parents. + ); + insta::assert_snapshot!(get_log_output(&test_env, &workspace_path), @r###" + @ 4224f9c9e598 merged-head parents: 1 a + ├─╮ + │ │ ◉ 401e43e9461f b parents: a + │ ├─╯ + │ ◉ 66ea2ab19a70 a parents: + │ │ ◉ d826910d21fb 2 parents: 1 + ├───╯ + ◉ │ dc0e5d6135ce 1 parents: + ├─╯ + ◉ 000000000000 parents: "###); }