Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

repo: merge rewrite state into single parent_mapping with enum #3400

Merged
merged 1 commit into from
Mar 30, 2024
Merged

repo: merge rewrite state into single parent_mapping with enum #3400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 37 additions & 46 deletions lib/src/repo.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -728,21 +728,27 @@ impl RepoLoader {
}
}

#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub(crate) enum RewriteType {
Rewritten,
Divergent,
Abandoned,
}

pub struct MutableRepo {
base_repo: Arc<ReadonlyRepo>,
index: Box<dyn MutableIndex>,
view: DirtyCell<View>,
// TODO: make these fields private again
// The commit identified by the key has been replaced by all the ones in the value, typically
// because the key commit was abandoned (the value commits are then the abandoned commit's
// parents). A child of the key commit should be rebased onto all the value commits. A branch
// pointing to the key commit should become a conflict pointing to all the value commits.
pub(crate) parent_mapping: HashMap<CommitId, Vec<CommitId>>,
/// Commits with a key in `parent_mapping` that have been divergently
/// rewritten into all the commits indicated by the value.
pub(crate) divergent: HashSet<CommitId>,
// Commits that were abandoned. Their children should be rebased onto their parents.
pub(crate) abandoned: HashSet<CommitId>,
// The commit identified by the key has been replaced by all the ones in the value.
// * Branches pointing to the old commit should be updated to the new commit, resulting in a
// conflict if there multiple new commits.
// * Children of the old commit should be rebased onto the new commits. However, if the type is
// `Divergent`, they should be left in place.
// * Working copies pointing to the old commit should be updated to the first of the new
// commits. However, if the type is `Abandoned`, a new working-copy commit should be created
// on top of all of the new commits instead.
pub(crate) parent_mapping: HashMap<CommitId, (RewriteType, Vec<CommitId>)>,
}

impl MutableRepo {
Expand All @@ -758,8 +764,6 @@ impl MutableRepo {
index: mut_index,
view: DirtyCell::with_clean(mut_view),
parent_mapping: Default::default(),
divergent: Default::default(),
abandoned: Default::default(),
}
}

Expand All @@ -776,9 +780,7 @@ impl MutableRepo {
}

pub fn has_changes(&self) -> bool {
!(self.abandoned.is_empty()
&& self.parent_mapping.is_empty()
&& self.view() == &self.base_repo.view)
!(self.parent_mapping.is_empty() && self.view() == &self.base_repo.view)
}

pub(crate) fn consume(self) -> (Box<dyn MutableIndex>, View) {
Expand Down Expand Up @@ -826,9 +828,8 @@ impl MutableRepo {
/// docstring for `record_rewritten_commit` for details.
pub fn set_rewritten_commit(&mut self, old_id: CommitId, new_id: CommitId) {
assert_ne!(old_id, *self.store().root_commit_id());
self.divergent.remove(&old_id);
self.abandoned.remove(&old_id);
self.parent_mapping.insert(old_id, vec![new_id]);
self.parent_mapping
.insert(old_id, (RewriteType::Rewritten, vec![new_id]));
}

/// Record a commit as being rewritten into multiple other commits in this
Expand All @@ -844,10 +845,10 @@ impl MutableRepo {
new_ids: impl IntoIterator<Item = CommitId>,
) {
assert_ne!(old_id, *self.store().root_commit_id());
self.abandoned.remove(&old_id);
self.parent_mapping
.insert(old_id.clone(), new_ids.into_iter().collect());
self.divergent.insert(old_id);
self.parent_mapping.insert(
old_id.clone(),
(RewriteType::Divergent, new_ids.into_iter().collect()),
);
}

/// Record a commit as having been abandoned in this transaction.
Expand Down Expand Up @@ -877,21 +878,15 @@ impl MutableRepo {
old_id: CommitId,
new_parent_ids: impl IntoIterator<Item = CommitId>,
) {
self.divergent.remove(&old_id);
assert_ne!(old_id, *self.store().root_commit_id());
self.abandoned.insert(old_id.clone());
self.parent_mapping
.insert(old_id, new_parent_ids.into_iter().collect());
}

fn clear_descendant_rebaser_plans(&mut self) {
self.parent_mapping.clear();
self.divergent.clear();
self.abandoned.clear();
self.parent_mapping.insert(
old_id,
(RewriteType::Abandoned, new_parent_ids.into_iter().collect()),
);
}

pub fn has_rewrites(&self) -> bool {
!(self.parent_mapping.is_empty() && self.abandoned.is_empty())
!self.parent_mapping.is_empty()
}

/// Calculates new parents for a commit that's currently based on the given
Expand All @@ -901,8 +896,7 @@ impl MutableRepo {
/// Panics if `parent_mapping` contains cycles
pub fn new_parents(&self, old_ids: &[CommitId]) -> Vec<CommitId> {
fn single_substitution_round(
parent_mapping: &HashMap<CommitId, Vec<CommitId>>,
divergent: &HashSet<CommitId>,
parent_mapping: &HashMap<CommitId, (RewriteType, Vec<CommitId>)>,
ids: Vec<CommitId>,
) -> (Vec<CommitId>, bool) {
let mut made_replacements = false;
Expand All @@ -911,13 +905,11 @@ impl MutableRepo {
// being singletons. If CommitId-s were Copy. no allocations would be needed in
// that case, but it probably doesn't matter much while they are Vec<u8>-s.
for id in ids.into_iter() {
if divergent.contains(&id) {
new_ids.push(id);
continue;
}
match parent_mapping.get(&id) {
None => new_ids.push(id),
Some(replacements) => {
None | Some((RewriteType::Divergent, _)) => {
new_ids.push(id);
}
Some((_, replacements)) => {
assert!(
// Each commit must have a parent, so a parent can
// not just be mapped to nothing. This assertion
Expand All @@ -940,8 +932,7 @@ impl MutableRepo {
let mut allowed_iterations = 0..self.parent_mapping.len();
loop {
let made_replacements;
(new_ids, made_replacements) =
single_substitution_round(&self.parent_mapping, &self.divergent, new_ids);
(new_ids, made_replacements) = single_substitution_round(&self.parent_mapping, new_ids);
if !made_replacements {
break;
}
Expand All @@ -959,7 +950,7 @@ impl MutableRepo {
}

/// After the rebaser returned by this function is dropped,
/// self.clear_descendant_rebaser_plans() needs to be called.
/// self.parent_mapping needs to be cleared.
fn rebase_descendants_return_rebaser<'settings, 'repo>(
&'repo mut self,
settings: &'settings UserSettings,
Expand All @@ -986,7 +977,7 @@ impl MutableRepo {
let result = self
.rebase_descendants_return_rebaser(settings, options)?
.map_or(0, |rebaser| rebaser.into_map().len());
self.clear_descendant_rebaser_plans();
self.parent_mapping.clear();
Ok(result)
}

Expand Down Expand Up @@ -1014,7 +1005,7 @@ impl MutableRepo {
// abandoned
.rebase_descendants_return_rebaser(settings, options)?
.map_or(HashMap::new(), |rebaser| rebaser.into_map()));
self.clear_descendant_rebaser_plans();
self.parent_mapping.clear();
result
}

Expand Down
11 changes: 7 additions & 4 deletions lib/src/rewrite.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ use crate::matchers::{Matcher, Visit};
use crate::merged_tree::{MergedTree, MergedTreeBuilder};
use crate::object_id::ObjectId;
use crate::op_store::RefTarget;
use crate::repo::{MutableRepo, Repo};
use crate::repo::{MutableRepo, Repo, RewriteType};
use crate::repo_path::RepoPath;
use crate::revset::{RevsetExpression, RevsetIteratorExt};
use crate::settings::UserSettings;
Expand Down Expand Up @@ -317,7 +317,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
visited.insert(commit.id().clone());
let mut dependents = vec![];
for parent in commit.parents() {
if let Some(targets) = mut_repo.parent_mapping.get(parent.id()) {
if let Some((_, targets)) = mut_repo.parent_mapping.get(parent.id()) {
for target in targets {
if to_visit_set.contains(target) && !visited.contains(target) {
dependents.push(store.get_commit(target).unwrap());
Expand Down Expand Up @@ -367,7 +367,10 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
new_commit_ids: Vec<CommitId>,
) -> Result<(), BackendError> {
// We arbitrarily pick a new working-copy commit among the candidates.
let abandoned_old_commit = self.mut_repo.abandoned.contains(&old_commit_id);
let abandoned_old_commit = matches!(
self.mut_repo.parent_mapping.get(&old_commit_id),
Some((RewriteType::Abandoned, _))
);
self.update_wc_commits(&old_commit_id, &new_commit_ids[0], abandoned_old_commit)?;

// Build a map from commit to branches pointing to it, so we don't need to scan
Expand Down Expand Up @@ -476,7 +479,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
}

fn update_all_references(&mut self) -> Result<(), BackendError> {
for (old_parent_id, new_parent_ids) in self.mut_repo.parent_mapping.clone() {
for (old_parent_id, (_, new_parent_ids)) in self.mut_repo.parent_mapping.clone() {
// Call `new_parents()` here since `parent_mapping` only contains direct
// mappings, not transitive ones.
// TODO: keep parent_mapping updated with transitive mappings so we don't need
Expand Down