Skip to content

Commit

Permalink
rewrite: move_commits: add MoveCommitsTarget enum to specify roots …
Browse files Browse the repository at this point in the history 
…or commits to move

This also allows some minor optimizations to be performed, such as
avoiding recomputation of the connected target set when
`MoveCommitsTarget::Roots` is used since the connected target set is
identical to the target set (all descendants of the roots).
  • Loading branch information
@bnjmnt4n
bnjmnt4n committed Oct 16, 2024
1 parent ec087fb commit 0a83d6a
Show file tree
Hide file tree
Showing 2 changed files with 97 additions and 70 deletions.
26 changes: 8 additions & 18 deletions cli/src/commands/rebase.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ use jj_lib::revset::RevsetIteratorExt;
use jj_lib::rewrite::move_commits;
use jj_lib::rewrite::EmptyBehaviour;
use jj_lib::rewrite::MoveCommitsStats;
use jj_lib::rewrite::MoveCommitsTarget;
use jj_lib::rewrite::RebaseOptions;
use jj_lib::settings::UserSettings;
use tracing::instrument;
Expand Down Expand Up @@ -325,7 +326,7 @@ fn rebase_revisions(
workspace_command,
&new_parents.iter().ids().cloned().collect_vec(),
&new_children,
&target_commits,
target_commits,
rebase_options,
)
}
Expand Down Expand Up @@ -358,7 +359,7 @@ fn rebase_source(
workspace_command,
&new_parents.iter().ids().cloned().collect_vec(),
&new_children,
&source_commits,
source_commits,
rebase_options,
)
}
Expand Down Expand Up @@ -396,7 +397,7 @@ fn rebase_branch(
workspace_command,
&parent_ids,
&[],
&root_commits,
root_commits,
&rebase_options,
)
}
Expand All @@ -407,7 +408,7 @@ fn rebase_descendants_transaction(
workspace_command: &mut WorkspaceCommandHelper,
new_parent_ids: &[CommitId],
new_children: &[Commit],
target_roots: &[Commit],
target_roots: Vec<Commit>,
rebase_options: &RebaseOptions,
) -> Result<(), CommandError> {
if target_roots.is_empty() {
Expand All @@ -427,15 +428,6 @@ fn rebase_descendants_transaction(
)
};

let target_commits: Vec<_> =
RevsetExpression::commits(target_roots.iter().ids().cloned().collect_vec())
.descendants()
.evaluate_programmatic(tx.repo())?
.iter()
.commits(tx.repo().store())
.try_collect()?;
let target_roots = target_roots.iter().ids().cloned().collect_vec();

let MoveCommitsStats {
num_rebased_targets,
num_rebased_descendants,
Expand All @@ -446,8 +438,7 @@ fn rebase_descendants_transaction(
tx.repo_mut(),
new_parent_ids,
new_children,
&target_commits,
Some(&target_roots),
MoveCommitsTarget::Roots(target_roots),
rebase_options,
)?;

Expand Down Expand Up @@ -551,7 +542,7 @@ fn rebase_revisions_transaction(
workspace_command: &mut WorkspaceCommandHelper,
new_parent_ids: &[CommitId],
new_children: &[Commit],
target_commits: &[Commit],
target_commits: Vec<Commit>,
rebase_options: &RebaseOptions,
) -> Result<(), CommandError> {
if target_commits.is_empty() {
Expand Down Expand Up @@ -579,8 +570,7 @@ fn rebase_revisions_transaction(
tx.repo_mut(),
new_parent_ids,
new_children,
target_commits,
None,
MoveCommitsTarget::Commits(target_commits),
rebase_options,
)?;
// TODO(ilyagr): Consider making it possible for descendants of the target set
Expand Down
141 changes: 89 additions & 52 deletions lib/src/rewrite.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -464,70 +464,107 @@ pub struct MoveCommitsStats {
pub num_abandoned: u32,
}

pub enum MoveCommitsTarget {
/// The commits to be moved. Commits should be mutable and in reverse
/// topological order.
Commits(Vec<Commit>),
/// The root commits to be moved, along with all their descendants.
/// Root commits should be mutable and in reverse topological order.
Roots(Vec<Commit>),
}

/// Moves `target_commits` from their current location to a new location in the
/// graph.
///
/// Commits in `target_roots` are rebased onto the new parents
/// given by `new_parent_ids`, while the `new_children` commits are
/// rebased onto the heads of `target_commits`. If `target_roots` is
/// `None`, it will be computed as the roots of the connected set of
/// target commits. This assumes that `target_commits` and
/// `new_children` can be rewritten, and there will be no cycles in
/// the resulting graph. `target_commits` should be in reverse
/// topological order. `target_roots`, if provided, should be a subset
/// of `target_commits`.
/// Commits in `target` are rebased onto the new parents given by
/// `new_parent_ids`, while the `new_children` commits are rebased onto the
/// heads of the commits in `targets`. This assumes that commits in `target` and
/// `new_children` can be rewritten, and there will be no cycles in the
/// resulting graph. Commits in `target` should be in reverse topological order.
pub fn move_commits(
settings: &UserSettings,
mut_repo: &mut MutableRepo,
new_parent_ids: &[CommitId],
new_children: &[Commit],
target_commits: &[Commit],
target_roots: Option<&[CommitId]>,
target: MoveCommitsTarget,
options: &RebaseOptions,
) -> BackendResult<MoveCommitsStats> {
if target_commits.is_empty() {
return Ok(MoveCommitsStats {
num_rebased_targets: 0,
num_rebased_descendants: 0,
num_skipped_rebases: 0,
num_abandoned: 0,
});
}

let target_commit_ids: HashSet<_> = target_commits.iter().ids().cloned().collect();
let target_commits: Vec<Commit>;
let target_commit_ids: HashSet<_>;
let connected_target_commits: Vec<Commit>;
let connected_target_commits_internal_parents: HashMap<CommitId, Vec<CommitId>>;
let target_roots: HashSet<CommitId>;

match target {
MoveCommitsTarget::Commits(commits) => {
if commits.is_empty() {
return Ok(MoveCommitsStats {
num_rebased_targets: 0,
num_rebased_descendants: 0,
num_skipped_rebases: 0,
num_abandoned: 0,
});
}

let connected_target_commits: Vec<_> =
RevsetExpression::commits(target_commits.iter().ids().cloned().collect_vec())
.connected()
.evaluate_programmatic(mut_repo)
.map_err(|err| match err {
RevsetEvaluationError::StoreError(err) => err,
RevsetEvaluationError::Other(_) => panic!("Unexpected revset error: {err}"),
})?
.iter()
.commits(mut_repo.store())
.try_collect()?;
target_commits = commits;
target_commit_ids = target_commits.iter().ids().cloned().collect();

connected_target_commits =
RevsetExpression::commits(target_commits.iter().ids().cloned().collect_vec())
.connected()
.evaluate_programmatic(mut_repo)
.map_err(|err| match err {
RevsetEvaluationError::StoreError(err) => err,
RevsetEvaluationError::Other(_) => panic!("Unexpected revset error: {err}"),
})?
.iter()
.commits(mut_repo.store())
.try_collect()?;
connected_target_commits_internal_parents =
compute_connected_target_commits_internal_parents(
&target_commit_ids,
&connected_target_commits,
);

target_roots = connected_target_commits_internal_parents
.iter()
.filter(|(commit_id, parents)| {
target_commit_ids.contains(commit_id) && parents.is_empty()
})
.map(|(commit_id, _)| commit_id.clone())
.collect();
}
MoveCommitsTarget::Roots(roots) => {
if roots.is_empty() {
return Ok(MoveCommitsStats {
num_rebased_targets: 0,
num_rebased_descendants: 0,
num_skipped_rebases: 0,
num_abandoned: 0,
});
}

// Compute the parents of all commits in the connected target set, allowing only
// commits in the target set as parents.
let connected_target_commits_internal_parents =
compute_connected_target_commits_internal_parents(
&target_commit_ids,
&connected_target_commits,
);

// Compute the roots of `target_commits` if not provided.
let target_roots: HashSet<_> = if let Some(target_roots) = target_roots {
target_roots.iter().cloned().collect()
} else {
connected_target_commits_internal_parents
.iter()
.filter(|(commit_id, parents)| {
target_commit_ids.contains(commit_id) && parents.is_empty()
})
.map(|(commit_id, _)| commit_id.clone())
.collect()
};
target_commits = RevsetExpression::commits(roots.iter().ids().cloned().collect_vec())
.descendants()
.evaluate_programmatic(mut_repo)
.map_err(|err| match err {
RevsetEvaluationError::StoreError(err) => err,
RevsetEvaluationError::Other(_) => panic!("Unexpected revset error: {err}"),
})?
.iter()
.commits(mut_repo.store())
.try_collect()?;
target_commit_ids = target_commits.iter().ids().cloned().collect();

connected_target_commits = target_commits.iter().cloned().collect_vec();
connected_target_commits_internal_parents =
compute_connected_target_commits_internal_parents(
&target_commit_ids,
&connected_target_commits,
);
target_roots = roots.iter().ids().cloned().collect();
}
}

// If a commit outside the target set has a commit in the target set as a
// parent, then - after the transformation - it should have that commit's
Expand Down

0 comments on commit 0a83d6a

Please sign in to comment.