Skip to content

Commit

Permalink
parallelize: make the command pass in more cases
Browse files Browse the repository at this point in the history
  • Loading branch information
@martinvonz
martinvonz committed Apr 17, 2024
1 parent b402f40 commit c933105
Show file tree
Hide file tree
Showing 2 changed files with 69 additions and 139 deletions.
124 changes: 2 additions & 122 deletions cli/src/commands/parallelize.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -81,10 +78,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());
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,
Expand Down Expand Up @@ -153,116 +146,3 @@ pub(crate) fn cmd_parallelize(

tx.finish(ui, format!("parallelize {} commits", num_targets))
}

/// 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<RevsetExpression>,
target_commits: &[Commit],
repo: &dyn Repo,
) -> Result<Vec<Commit>, 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<RevsetExpression>,
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<Commit> = 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<RevsetExpression>,
repo: &dyn Repo,
) -> Result<Vec<Commit>, CommandError> {
let target_roots: Vec<Commit> = 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<CommitId> = 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(())
}
84 changes: 67 additions & 17 deletions cli/tests/test_parallelize_command.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -242,9 +242,19 @@ fn test_parallelize_failure_disconnected_target_commits() {
◉ 000000000000
"###);

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
◉ d826910d21fb 2
◉ dc0e5d6135ce 1
◉ 000000000000
"###);
}

Expand Down Expand Up @@ -275,9 +285,19 @@ fn test_parallelize_head_is_a_merge() {
◉ 000000000000
"###);

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
├─╮
│ ◉ 5164ab888473 b
│ ◉ f16fe8ac5ce9 a
│ │ ◉ 36b2f866a798 2
├───╯
│ │ ◉ a915696cf0ad 1
├───╯
◉ │ a56846756248 0
├─╯
◉ 000000000000
"###);
}

Expand Down Expand Up @@ -305,9 +325,18 @@ fn test_parallelize_interior_target_is_a_merge() {
◉ 000000000000
"###);

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
├─╮
│ │ ◉ 1c240e875670 2
╭─┬─╯
│ ◉ 427890ea3f2b a
│ │ ◉ a915696cf0ad 1
├───╯
◉ │ a56846756248 0
├─╯
◉ 000000000000
"###);
}

Expand Down Expand Up @@ -472,16 +501,28 @@ fn test_parallelize_failure_multiple_heads_with_different_children() {
◉ 000000000000
"###);

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
◉ dd2db8b60a69 c
├─╮
│ ◉ 190b857f6cdd b
◉ │ f16fe8ac5ce9 a
├─╯
│ ◉ bbc313370f45 3
│ ├─╮
│ │ ◉ 96ce11389312 2
├───╯
│ ◉ dc0e5d6135ce 1
├─╯
◉ 000000000000
"###);
}

Expand Down Expand Up @@ -510,12 +551,21 @@ fn test_parallelize_failure_multiple_roots_with_different_parents() {
◉ 000000000000
"###);

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
├─╮
│ │ ◉ 401e43e9461f b
│ ├─╯
│ ◉ 66ea2ab19a70 a
│ │ ◉ d826910d21fb 2
├───╯
◉ │ dc0e5d6135ce 1
├─╯
◉ 000000000000
"###);
}

Expand Down

0 comments on commit c933105

Please sign in to comment.