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 18, 2024
1 parent b6d91f8 commit 23c77f5
Show file tree
Hide file tree
Showing 3 changed files with 95 additions and 165 deletions.
146 changes: 13 additions & 133 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 All @@ -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 {
Expand Down Expand Up @@ -80,10 +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());
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 @@ -152,116 +145,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<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(())
}
24 changes: 12 additions & 12 deletions cli/tests/[email protected]
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 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.
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.
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]...`
Expand Down
90 changes: 70 additions & 20 deletions cli/tests/test_parallelize_command.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -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 @@ -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");
Expand All @@ -472,21 +501,33 @@ 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
"###);
}

#[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");
Expand All @@ -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 23c77f5

Please sign in to comment.