( )⚙ D10070 copies-rust: pass closures and iterators instead of `&ChangedFiles`

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D10070

copies-rust: pass closures and iterators instead of `&ChangedFiles`
ClosedPublic

Authored by SimonSapin on Feb 25 2021, 5:42 AM.

Details

Reviewers
pulkit
Group Reviewers
hg-reviewers
Commits
rHG80f7567ac9bb: copies-rust: pass closures and iterators instead of `&ChangedFiles`
Summary

… to some functions that only use one method.
This will makes it easier to unit-test them.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Revision Contents
Changeset List

PathPackages
Mrust/hg-core/src/copy_tracing.rs (39 lines)

Diff 25959

rust/hg-core/src/copy_tracing.rs

/// previously given for its parents (and transitively, its ancestors). /// previously given for its parents (and transitively, its ancestors).
pub fn add_revision( pub fn add_revision(
&mut self, &mut self,
rev: Revision, rev: Revision,
p1: Revision, p1: Revision,
p2: Revision, p2: Revision,
changes: ChangedFiles<'_>, changes: ChangedFiles<'_>,
) { ) {
self.add_revision_inner(rev, p1, p2, changes.iter_actions(), |path| {
changes.get_merge_case(path)
})
}
/// Separated out from `add_revsion` so that unit tests can call this
/// without synthetizing a `ChangedFiles` in binary format.
fn add_revision_inner<'a>(
&mut self,
rev: Revision,
p1: Revision,
p2: Revision,
copy_actions: impl Iterator<Item = Action<'a>>,
get_merge_case: impl Fn(&HgPath) -> MergeCase + Copy,
) {
// Retrieve data computed in a previous iteration // Retrieve data computed in a previous iteration
let p1_copies = match p1 { let p1_copies = match p1 {
NULL_REVISION => None, NULL_REVISION => None,
_ => get_and_clean_parent_copies( _ => get_and_clean_parent_copies(
&mut self.all_copies, &mut self.all_copies,
&mut self.children_count, &mut self.children_count,
p1, p1,
), // will be None if the vertex is not to be traversed ), // will be None if the vertex is not to be traversed
}; };
let p2_copies = match p2 { let p2_copies = match p2 {
NULL_REVISION => None, NULL_REVISION => None,
_ => get_and_clean_parent_copies( _ => get_and_clean_parent_copies(
&mut self.all_copies, &mut self.all_copies,
&mut self.children_count, &mut self.children_count,
p2, p2,
), // will be None if the vertex is not to be traversed ), // will be None if the vertex is not to be traversed
}; };
// combine it with data for that revision // combine it with data for that revision
let (p1_copies, p2_copies) = chain_changes( let (p1_copies, p2_copies) = chain_changes(
&mut self.path_map, &mut self.path_map,
p1_copies, p1_copies,
p2_copies, p2_copies,
&changes, copy_actions,
rev, rev,
); );
let copies = match (p1_copies, p2_copies) { let copies = match (p1_copies, p2_copies) {
(None, None) => None, (None, None) => None,
(c, None) => c, (c, None) => c,
(None, c) => c, (None, c) => c,
(Some(p1_copies), Some(p2_copies)) => Some(merge_copies_dict( (Some(p1_copies), Some(p2_copies)) => Some(merge_copies_dict(
&self.path_map, &self.path_map,
rev, rev,
p2_copies, p2_copies,
p1_copies, p1_copies,
&changes, get_merge_case,
)), )),
}; };
if let Some(c) = copies { if let Some(c) = copies {
self.all_copies.insert(rev, c); self.all_copies.insert(rev, c);
} }
} }
/// Drop intermediate data (such as which revision a copy was from) and /// Drop intermediate data (such as which revision a copy was from) and
Some(c) => Some(c.clone()), Some(c) => Some(c.clone()),
None => Some(InternalPathCopies::default()), None => Some(InternalPathCopies::default()),
} }
} }
} }
/// Combine ChangedFiles with some existing PathCopies information and return /// Combine ChangedFiles with some existing PathCopies information and return
/// the result /// the result
fn chain_changes( fn chain_changes<'a>(
path_map: &mut TwoWayPathMap, path_map: &mut TwoWayPathMap,
base_p1_copies: Option<InternalPathCopies>, base_p1_copies: Option<InternalPathCopies>,
base_p2_copies: Option<InternalPathCopies>, base_p2_copies: Option<InternalPathCopies>,
changes: &ChangedFiles, copy_actions: impl Iterator<Item = Action<'a>>,
current_rev: Revision, current_rev: Revision,
) -> (Option<InternalPathCopies>, Option<InternalPathCopies>) { ) -> (Option<InternalPathCopies>, Option<InternalPathCopies>) {
// Fast path the "nothing to do" case. // Fast path the "nothing to do" case.
if let (None, None) = (&base_p1_copies, &base_p2_copies) { if let (None, None) = (&base_p1_copies, &base_p2_copies) {
return (None, None); return (None, None);
} }
let mut p1_copies = base_p1_copies.clone(); let mut p1_copies = base_p1_copies.clone();
let mut p2_copies = base_p2_copies.clone(); let mut p2_copies = base_p2_copies.clone();
for action in changes.iter_actions() { for action in copy_actions {
match action { match action {
Action::CopiedFromP1(path_dest, path_source) => { Action::CopiedFromP1(path_dest, path_source) => {
match &mut p1_copies { match &mut p1_copies {
None => (), // This is not a vertex we should proceed. None => (), // This is not a vertex we should proceed.
Some(copies) => add_one_copy( Some(copies) => add_one_copy(
current_rev, current_rev,
path_map, path_map,
copies, copies,
/// ///
/// In case of conflict, value from "major" will be picked, unless in some /// In case of conflict, value from "major" will be picked, unless in some
/// cases. See inline documentation for details. /// cases. See inline documentation for details.
fn merge_copies_dict( fn merge_copies_dict(
path_map: &TwoWayPathMap, path_map: &TwoWayPathMap,
current_merge: Revision, current_merge: Revision,
minor: InternalPathCopies, minor: InternalPathCopies,
major: InternalPathCopies, major: InternalPathCopies,
changes: &ChangedFiles, get_merge_case: impl Fn(&HgPath) -> MergeCase + Copy,
) -> InternalPathCopies { ) -> InternalPathCopies {
use crate::utils::{ordmap_union_with_merge, MergeResult}; use crate::utils::{ordmap_union_with_merge, MergeResult};
ordmap_union_with_merge(minor, major, |&dest, src_minor, src_major| { ordmap_union_with_merge(minor, major, |&dest, src_minor, src_major| {
let (pick, overwrite) = compare_value( let (pick, overwrite) = compare_value(
path_map,
current_merge, current_merge,
changes, || get_merge_case(path_map.untokenize(dest)),
dest,
src_minor, src_minor,
src_major, src_major,
); );
if overwrite { if overwrite {
let (winner, loser) = match pick { let (winner, loser) = match pick {
MergePick::Major | MergePick::Any => (src_major, src_minor), MergePick::Major | MergePick::Any => (src_major, src_minor),
MergePick::Minor => (src_minor, src_major), MergePick::Minor => (src_minor, src_major),
}; };
MergePick::Minor => MergeResult::UseLeftValue, MergePick::Minor => MergeResult::UseLeftValue,
} }
} }
}) })
} }
/// represent the side that should prevail when merging two /// represent the side that should prevail when merging two
/// InternalPathCopies /// InternalPathCopies
#[derive(Debug, PartialEq)]
enum MergePick { enum MergePick {
/// The "major" (p1) side prevails /// The "major" (p1) side prevails
Major, Major,
/// The "minor" (p2) side prevails /// The "minor" (p2) side prevails
Minor, Minor,
/// Any side could be used (because they are the same) /// Any side could be used (because they are the same)
Any, Any,
} }
/// decide which side prevails in case of conflicting values /// decide which side prevails in case of conflicting values
#[allow(clippy::if_same_then_else)] #[allow(clippy::if_same_then_else)]
fn compare_value( fn compare_value(
path_map: &TwoWayPathMap,
current_merge: Revision, current_merge: Revision,
changes: &ChangedFiles, merge_case_for_dest: impl Fn() -> MergeCase,
dest: PathToken,
src_minor: &CopySource, src_minor: &CopySource,
src_major: &CopySource, src_major: &CopySource,
) -> (MergePick, bool) { ) -> (MergePick, bool) {
if src_major == src_minor { if src_major == src_minor {
(MergePick::Any, false) (MergePick::Any, false)
} else if src_major.rev == current_merge { } else if src_major.rev == current_merge {
// minor is different according to per minor == major check earlier // minor is different according to per minor == major check earlier
debug_assert!(src_minor.rev != current_merge); debug_assert!(src_minor.rev != current_merge);
(MergePick::Minor, false) (MergePick::Minor, false)
} else if src_minor.is_overwritten_by(src_major) { } else if src_minor.is_overwritten_by(src_major) {
(MergePick::Major, false) (MergePick::Major, false)
} else { } else {
(MergePick::Any, true) (MergePick::Any, true)
} }
} else { } else {
debug_assert!(src_major.rev != src_major.rev); debug_assert!(src_major.rev != src_major.rev);
let dest_path = path_map.untokenize(dest); let action = merge_case_for_dest();
let action = changes.get_merge_case(dest_path);
if src_minor.path.is_some() if src_minor.path.is_some()
&& src_major.path.is_none() && src_major.path.is_none()
&& action == MergeCase::Salvaged && action == MergeCase::Salvaged
{ {
// If the file is "deleted" in the major side but was // If the file is "deleted" in the major side but was
// salvaged by the merge, we keep the minor side alive // salvaged by the merge, we keep the minor side alive
(MergePick::Minor, true) (MergePick::Minor, true)
} else if src_major.path.is_some() } else if src_major.path.is_some()