From 668b991cd4c2f00de2e70e22ea2be50ef5990f60 Mon Sep 17 00:00:00 2001 From: Evan Mesterhazy Date: Mon, 1 Apr 2024 15:27:13 -0400 Subject: [PATCH] WIP: Handle complex descendants --- cli/src/commands/parallelize.rs | 89 ++++++++++++++++++--------- cli/tests/test_parallelize_command.rs | 28 ++------- 2 files changed, 66 insertions(+), 51 deletions(-) diff --git a/cli/src/commands/parallelize.rs b/cli/src/commands/parallelize.rs index ccee71de90..912bbfc722 100644 --- a/cli/src/commands/parallelize.rs +++ b/cli/src/commands/parallelize.rs @@ -76,8 +76,12 @@ pub(crate) fn cmd_parallelize( let mut tx = workspace_command.start_transaction(); let target_revset = tx .base_workspace_helper() - .parse_union_revsets(&args.revisions)?; - let target_commits: Vec = target_revset + .parse_union_revsets(&args.revisions)? + .expression() + .clone(); + let target_commits: Vec = tx + .base_workspace_helper() + .attach_revset_evaluator(target_revset.clone())? .evaluate()? .iter() // We want parents before children, so we need to reverse the order. @@ -89,7 +93,7 @@ pub(crate) fn cmd_parallelize( } let target_heads: Vec = tx .base_workspace_helper() - .attach_revset_evaluator(target_revset.expression().clone().heads())? + .attach_revset_evaluator(target_revset.heads())? .evaluate()? .iter() .collect(); @@ -101,7 +105,7 @@ pub(crate) fn cmd_parallelize( } let target_roots: Vec = tx .base_workspace_helper() - .attach_revset_evaluator(target_revset.expression().clone().roots())? + .attach_revset_evaluator(target_revset.roots())? .evaluate()? .iter() .collect(); @@ -111,34 +115,61 @@ pub(crate) fn cmd_parallelize( "Cannot parallelize a set of revisions with multiple roots. Roots: {roots}" ))); } + let connected_length = + RevsetExpression::commits(vec![target_heads[0].clone(), target_roots[0].clone()]) + .connected() + .evaluate_programmatic(tx.base_repo().as_ref())? + .iter() + .count(); + if connected_length != target_commits.len() { + return Err(user_error( + "Cannot parallelize since the target revisions are not connected.", + )); + } - // Rebase the children of the head commit onto the target commits. - let new_children: Vec = RevsetExpression::commit(target_heads[0].clone()) - .children() - .evaluate_programmatic(tx.base_repo().as_ref())? - .iter() - .commits(tx.base_repo().store()) - .try_collect()?; - for child in new_children { - // The ordering here is intentional. We iterate over the target commits first so - // that when `jj log` prints the graph they will be printed in their original - // order. After that, we add any existing parents which aren't being + // Rebase the children of each target commit onto its new parents. A child + // which had a target commit as an ancestor before parallelize ran will have + // the target commit as a parent afterward. + for target_commit in target_commits.iter() { + // These parents are shared by all children of the target commit. This + // includes the target commit itself plus any ancestors which are being // parallelized. - let new_parents_for_child: Vec = target_commits - .iter() - .map(|c| c.id().clone()) - .chain(child.parent_ids().iter().cloned()) - .collect::>() + let common_parents: IndexSet = + tx.base_workspace_helper() + .attach_revset_evaluator(target_revset.intersection( + &RevsetExpression::commit(target_commit.id().clone()).ancestors(), + ))? + .evaluate()? + .iter() + // We want parents before children, so we need to reverse the order. + .reversed() + .collect(); + let children: Vec = tx + .base_workspace_helper() + .attach_revset_evaluator( + RevsetExpression::commit(target_commit.id().clone()) + .children() + .minus(&target_revset), + )? + .evaluate()? .iter() - .map(|c| tx.mut_repo().store().get_commit(c)) - .collect::, _>>()?; - rebase_descendants( - &mut tx, - command.settings(), - &new_parents_for_child, - &[child], - Default::default(), - )?; + .commits(tx.base_repo().store()) + .try_collect()?; + for child in children { + let mut new_parents = common_parents.clone(); + new_parents.extend(child.parent_ids().iter().cloned()); + let parents = new_parents + .iter() + .map(|c| tx.mut_repo().store().get_commit(c)) + .try_collect::<_, Vec, _>()?; + rebase_descendants( + &mut tx, + command.settings(), + &parents, + &[child], + Default::default(), + )?; + } } // Rebase the target commits onto the parents of the root commit. diff --git a/cli/tests/test_parallelize_command.rs b/cli/tests/test_parallelize_command.rs index e6564c18d8..1ede92474e 100644 --- a/cli/tests/test_parallelize_command.rs +++ b/cli/tests/test_parallelize_command.rs @@ -123,17 +123,14 @@ fn test_parallelize_with_descendants_complex() { │ │ │ ◉ b8f977c12383 4 │ │ ◉ │ 7be8374575b9 3 │ │ ├─╯ - ◉ │ │ 2bfe3fe3e472 1 - ├───╯ - │ │ ◉ bf7eef1f1dc6 child-of-2 + │ │ │ ◉ 3b27a65c2742 child-of-2 + ╭─┬───╯ + │ ◉ │ 96ce11389312 2 │ ├─╯ - │ ◉ 96ce11389312 2 + ◉ │ 2bfe3fe3e472 1 ├─╯ ◉ 000000000000 "###); - // TODO: This is wrong. the "child-of-2" branch should have 1 as a parent as - // well. - panic!(); } #[test] @@ -156,22 +153,9 @@ fn test_parallelize_failure_disjoint_target_commits() { ◉ 000000000000 "###); - test_env.jj_cmd_ok(&workspace_path, &["parallelize", "dc0", "9f5"]); - insta::assert_snapshot!(get_log_output(&test_env, &workspace_path), @r###" - @ 3d657803ddcd 6 - ◉ b82bb741509e 5 - ◉ aa0a032f2d92 4 - ├─╮ - │ ◉ a9334ecaa379 3 - │ │ ◉ f382cea5a7a8 2 - ├───╯ - ◉ │ 761e67df44b7 1 - ├─╯ - ◉ 000000000000 + insta::assert_snapshot!(test_env.jj_cmd_failure(&workspace_path, &["parallelize", "dc0", "9f5"]),@r###" + Error: Cannot parallelize since the target revisions are not connected. "###); - // TODO: this isn't right. Either we should return an error, or 4 should - // have 2 as a parent instead of 1. - panic!(); } #[test]