mirror of
https://github.com/martinvonz/jj.git
synced 2024-12-25 05:29:39 +00:00
parallelize: make the command pass in more cases
The checks are not needed by the new implementation, so just drop them.
This commit is contained in:
Martin von Zweigbergk
Martin von Zweigbergk
parent
860b39b80f
commit
89356aebc6
3 changed files with 94 additions and 165 deletions
|
|
@ -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<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(())
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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]...`
|
||||
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
"###);
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue