This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9424

copies-rust: extract conflicting value comparison in its own function
ClosedPublic

Authored by marmoute on Nov 27 2020, 11:12 AM.

Details

Reviewers
Alphare
pulkit
Group Reviewers
hg-reviewers
Commits
rHG61afe6215aef: copies-rust: extract conflicting value comparison in its own function
Summary

First, that logic is complicated enough to be in it own function. Second, we
want to start adding alternative path within the merge code so we need this logic
easily accessible in multiple places.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Alphare created this revision.Nov 27 2020, 11:12 AM
marmoute updated this revision to Diff 23822.Nov 29 2020, 6:13 AM
marmoute updated this revision to Diff 23870.Nov 29 2020, 3:29 PM
marmoute added a reviewer: Alphare.Dec 8 2020, 5:48 AM
marmoute commandeered this revision.
baymax updated this revision to Diff 24117.Dec 8 2020, 6:04 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

baymax updated this revision to Diff 24130.Dec 8 2020, 9:58 PM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

pulkit added a subscriber: pulkit.Dec 12 2020, 3:23 AM
pulkit added inline comments.
rust/hg-core/src/copy_tracing.rs
541

IIUC, s/p1/p2 right?

Alphare accepted this revision.Dec 14 2020, 4:21 AM

Agree with Pulkit on the comment

rust/hg-core/src/copy_tracing.rs
509

It's better to have a readable error string in unreachable! than nothing.

marmoute updated this revision to Diff 24211.Dec 14 2020, 6:16 AM
baymax updated this revision to Diff 24235.Dec 14 2020, 7:06 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

pulkit accepted this revision.Dec 17 2020, 6:05 AM
This revision is now accepted and ready to land.Dec 17 2020, 6:05 AM

Revision Contents
Changeset List

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

Diff 24322

rust/hg-core/src/copy_tracing.rs

} }
copies copies
} }
/// merge two copies-mapping together, minor and major /// merge two copies-mapping together, minor and major
/// ///
/// 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.
#[allow(clippy::if_same_then_else)]
fn merge_copies_dict<A: Fn(Revision, Revision) -> bool>( fn merge_copies_dict<A: Fn(Revision, Revision) -> bool>(
minor: TimeStampedPathCopies, minor: TimeStampedPathCopies,
major: TimeStampedPathCopies, major: TimeStampedPathCopies,
changes: &ChangedFiles, changes: &ChangedFiles,
oracle: &mut AncestorOracle<A>, oracle: &mut AncestorOracle<A>,
) -> TimeStampedPathCopies { ) -> TimeStampedPathCopies {
if minor.is_empty() { if minor.is_empty() {
return major; return major;
// differently. // differently.
for d in minor.diff(&major) { for d in minor.diff(&major) {
match d { match d {
DiffItem::Add(k, v) => to_minor(k, v), DiffItem::Add(k, v) => to_minor(k, v),
DiffItem::Remove(k, v) => to_major(k, v), DiffItem::Remove(k, v) => to_major(k, v),
DiffItem::Update { old, new } => { DiffItem::Update { old, new } => {
let (dest, src_major) = new; let (dest, src_major) = new;
let (_, src_minor) = old; let (_, src_minor) = old;
let mut pick_minor = || (to_major(dest, src_minor)); match compare_value(
let mut pick_major = || (to_minor(dest, src_major)); changes, oracle, dest, src_minor, src_major,
) {
MergePick::Major => to_minor(dest, src_major),
MergePick::Minor => to_major(dest, src_minor),
// If the two entry are identical, no need to do
// anything (but diff should not have yield them)
MergePick::Any => unreachable!(),
AlphareUnsubmitted
Not Done

It's better to have a readable error string in unreachable! than nothing.

Alphare: It's better to have a readable error string in `unreachable!` than nothing.
}
}
};
}
let updates;
let mut result;
if override_major.is_empty() {
result = major
} else if override_minor.is_empty() {
result = minor
} else {
if override_minor.len() < override_major.len() {
updates = override_minor;
result = minor;
} else {
updates = override_major;
result = major;
}
for (k, v) in updates {
result.insert(k, v);
}
}
result
}
/// represent the side that should prevail when merging two
/// TimeStampedPathCopies
enum MergePick {
/// The "major" (p1) side prevails
Major,
/// The "minor" (p2) side prevails
pulkitUnsubmitted
Not Done

IIUC, s/p1/p2 right?

pulkit: IIUC, `s/p1/p2` right?
Minor,
/// Any side could be used (because they are the same)
Any,
}
/// decide which side prevails in case of conflicting values
#[allow(clippy::if_same_then_else)]
fn compare_value<A: Fn(Revision, Revision) -> bool>(
changes: &ChangedFiles,
oracle: &mut AncestorOracle<A>,
dest: &HgPathBuf,
src_minor: &TimeStampedPathCopy,
src_major: &TimeStampedPathCopy,
) -> MergePick {
if src_major.path == src_minor.path { if src_major.path == src_minor.path {
// we have the same value, but from other source; // we have the same value, but from other source;
if src_major.rev == src_minor.rev { if src_major.rev == src_minor.rev {
// If the two entry are identical, no need to do // If the two entry are identical, they are both valid
// anything (but diff should not have yield them) MergePick::Any
unreachable!(); } else if oracle.is_ancestor(src_major.rev, src_minor.rev) {
} else if oracle.is_ancestor(src_major.rev, src_minor.rev) MergePick::Minor
{
pick_minor();
} else { } else {
pick_major(); MergePick::Major
} }
} else if src_major.rev == src_minor.rev { } else if src_major.rev == src_minor.rev {
// We cannot get copy information for both p1 and p2 in the // We cannot get copy information for both p1 and p2 in the
// same rev. So this is the same value. // same rev. So this is the same value.
unreachable!(); unreachable!(
"conflict information from p1 and p2 in the same revision"
);
} else { } else {
let action = changes.get_merge_case(&dest); let action = changes.get_merge_case(&dest);
if src_major.path.is_none() if 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
pick_minor(); MergePick::Minor
} else if src_minor.path.is_none() } else if src_minor.path.is_none() && action == MergeCase::Salvaged {
&& action == MergeCase::Salvaged
{
// If the file is "deleted" in the minor side but was // If the file is "deleted" in the minor side but was
// salvaged by the merge, unconditionnaly preserve the // salvaged by the merge, unconditionnaly preserve the
// major side. // major side.
pick_major(); MergePick::Major
} else if action == MergeCase::Merged { } else if action == MergeCase::Merged {
// If the file was actively merged, copy information // If the file was actively merged, copy information
// from each side might conflict. The major side will // from each side might conflict. The major side will
// win such conflict. // win such conflict.
pick_major(); MergePick::Major
} else if oracle.is_ancestor(src_major.rev, src_minor.rev) } else if oracle.is_ancestor(src_major.rev, src_minor.rev) {
{
// If the minor side is strictly newer than the major // If the minor side is strictly newer than the major
// side, it should be kept. // side, it should be kept.
pick_minor(); MergePick::Minor
} else if src_major.path.is_some() { } else if src_major.path.is_some() {
// without any special case, the "major" value win // without any special case, the "major" value win
// other the "minor" one. // other the "minor" one.
pick_major(); MergePick::Major
} else if oracle.is_ancestor(src_minor.rev, src_major.rev) } else if oracle.is_ancestor(src_minor.rev, src_major.rev) {
{
// the "major" rev is a direct ancestors of "minor", // the "major" rev is a direct ancestors of "minor",
// any different value should // any different value should
// overwrite // overwrite
pick_major(); MergePick::Major
} else { } else {
// major version is None (so the file was deleted on // major version is None (so the file was deleted on
// that branch) and that branch is independant (neither // that branch) and that branch is independant (neither
// minor nor major is an ancestors of the other one.) // minor nor major is an ancestors of the other one.)
// We preserve the new // We preserve the new
// information about the new file. // information about the new file.
pick_minor(); MergePick::Minor
}
}
}
};
}
let updates;
let mut result;
if override_major.is_empty() {
result = major
} else if override_minor.is_empty() {
result = minor
} else {
if override_minor.len() < override_major.len() {
updates = override_minor;
result = minor;
} else {
updates = override_major;
result = major;
}
for (k, v) in updates {
result.insert(k, v);
} }
} }
result
} }