Skip to content

Commit

Permalink
repo: move new commit ids into RewriteType enum
Browse files Browse the repository at this point in the history
`RewriteType::Rewritten` must have exactly one replacement. I think
it's better to encode that in the type by attaching the value to the
enum variant. I also renamed the type to just `Rewrite` since it now
has attached data and `Type` sounds like a traditional data-free enum
to me.
  • Loading branch information
martinvonz committed May 6, 2024
1 parent 748485d commit d66d685
Showing 1 changed file with 37 additions and 17 deletions.
54 changes: 37 additions & 17 deletions lib/src/repo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -761,11 +761,27 @@ impl RepoLoader {
}
}

#[derive(Clone, Copy, Debug, PartialEq, Eq)]
enum RewriteType {
Rewritten,
Divergent,
Abandoned,
#[derive(Clone, Debug, PartialEq, Eq)]
enum Rewrite {
/// The old commit was rewritten as this new commit. Children should be
/// rebased onto the new commit.
Rewritten(CommitId),
/// The old commit was rewritten as multiple other commits. Children should
/// not be rebased.
Divergent(Vec<CommitId>),
/// The old commit was abandoned. Children should be rebased onto the given
/// commits (typically the parents of the old commit).
Abandoned(Vec<CommitId>),
}

impl Rewrite {
fn new_parent_ids(&self) -> &[CommitId] {
match self {
Rewrite::Rewritten(new_parent_id) => std::slice::from_ref(new_parent_id),
Rewrite::Divergent(new_parent_ids) => new_parent_ids.as_slice(),
Rewrite::Abandoned(new_parent_ids) => new_parent_ids.as_slice(),
}
}
}

pub struct MutableRepo {
Expand All @@ -780,7 +796,7 @@ pub struct MutableRepo {
// * 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.
parent_mapping: HashMap<CommitId, (RewriteType, Vec<CommitId>)>,
parent_mapping: HashMap<CommitId, Rewrite>,
}

impl MutableRepo {
Expand Down Expand Up @@ -861,7 +877,7 @@ impl MutableRepo {
pub fn set_rewritten_commit(&mut self, old_id: CommitId, new_id: CommitId) {
assert_ne!(old_id, *self.store().root_commit_id());
self.parent_mapping
.insert(old_id, (RewriteType::Rewritten, vec![new_id]));
.insert(old_id, Rewrite::Rewritten(new_id));
}

/// Record a commit as being rewritten into multiple other commits in this
Expand All @@ -879,7 +895,7 @@ impl MutableRepo {
assert_ne!(old_id, *self.store().root_commit_id());
self.parent_mapping.insert(
old_id.clone(),
(RewriteType::Divergent, new_ids.into_iter().collect()),
Rewrite::Divergent(new_ids.into_iter().collect()),
);
}

Expand Down Expand Up @@ -913,7 +929,7 @@ impl MutableRepo {
assert_ne!(old_id, *self.store().root_commit_id());
self.parent_mapping.insert(
old_id,
(RewriteType::Abandoned, new_parent_ids.into_iter().collect()),
Rewrite::Abandoned(new_parent_ids.into_iter().collect()),
);
}

Expand All @@ -928,7 +944,7 @@ impl MutableRepo {
/// Panics if `parent_mapping` contains cycles
pub fn new_parents(&self, old_ids: Vec<CommitId>) -> Vec<CommitId> {
fn single_substitution_round(
parent_mapping: &HashMap<CommitId, (RewriteType, Vec<CommitId>)>,
parent_mapping: &HashMap<CommitId, Rewrite>,
ids: Vec<CommitId>,
) -> (Vec<CommitId>, bool) {
let mut made_replacements = false;
Expand All @@ -938,10 +954,14 @@ impl MutableRepo {
// that case, but it probably doesn't matter much while they are Vec<u8>-s.
for id in ids.into_iter() {
match parent_mapping.get(&id) {
None | Some((RewriteType::Divergent, _)) => {
None | Some(Rewrite::Divergent(_)) => {
new_ids.push(id);
}
Some((_, replacements)) => {
Some(Rewrite::Rewritten(replacement)) => {
made_replacements = true;
new_ids.push(replacement.clone())
}
Some(Rewrite::Abandoned(replacements)) => {
assert!(
// Each commit must have a parent, so a parent can
// not just be mapped to nothing. This assertion
Expand Down Expand Up @@ -990,12 +1010,12 @@ impl MutableRepo {
}

fn update_all_references(&mut self, settings: &UserSettings) -> Result<(), BackendError> {
for (old_parent_id, (_, new_parent_ids)) in self.parent_mapping.clone() {
for (old_parent_id, rewrite) in self.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
// to call `new_parents()` here.
let new_parent_ids = self.new_parents(new_parent_ids);
let new_parent_ids = self.new_parents(rewrite.new_parent_ids().to_vec());
self.update_references(settings, old_parent_id, new_parent_ids)?;
}
Ok(())
Expand All @@ -1010,7 +1030,7 @@ impl MutableRepo {
// We arbitrarily pick a new working-copy commit among the candidates.
let abandoned_old_commit = matches!(
self.parent_mapping.get(&old_commit_id),
Some((RewriteType::Abandoned, _))
Some(Rewrite::Abandoned(_))
);
self.update_wc_commits(
settings,
Expand Down Expand Up @@ -1138,8 +1158,8 @@ impl MutableRepo {
visited.insert(commit.id().clone());
let mut dependents = vec![];
for parent in commit.parents() {
if let Some((_, targets)) = self.parent_mapping.get(parent.id()) {
for target in targets {
if let Some(rewrite) = self.parent_mapping.get(parent.id()) {
for target in rewrite.new_parent_ids() {
if to_visit_set.contains(target) && !visited.contains(target) {
dependents.push(store.get_commit(target));
}
Expand Down

0 comments on commit d66d685

Please sign in to comment.