This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9612

copies: rearrange all value comparison conditional
ClosedPublic

Authored by marmoute on Dec 15 2020, 4:07 AM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHGc692384bb559: copies: rearrange all value comparison conditional
rHG842115e3b307: copies: rearrange all value comparison conditional
Summary

To properly handle the newly tested case (chaining of merges) we will need to
detect more accurately when an actualy merging of the copy information (and
superseed the two existing data). Before starting to do so, we need to
reorganise the values comparison to introduce different conditional branches
when such actual merging is needed/detected.

To avoid mixing too many change in this complicated code, we do the
reorganisation before adding the "overwrite detection" logic in the next
changesets.

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

marmoute created this revision.Dec 15 2020, 4:07 AM
baymax updated this revision to Diff 24375.Dec 17 2020, 4:04 PM

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

baymax updated this revision to Diff 24402.Dec 18 2020, 9:12 AM

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

baymax updated this revision to Diff 24409.Dec 18 2020, 9:32 AM

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

baymax updated this revision to Diff 24417.Dec 18 2020, 9:54 AM

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

baymax updated this revision to Diff 24455.Dec 21 2020, 3:00 AM

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

baymax updated this revision to Diff 24571.Jan 5 2021, 3:38 AM

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

baymax updated this revision to Diff 24594.Jan 5 2021, 9:02 AM

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

Alphare accepted this revision.Jan 8 2021, 5:45 AM
Alphare added a subscriber: Alphare.

Balancing both algorithms in my head is not easy, but this looks fine, of course as long as the next patch does something more useful. :P
Your commit message may be missing a word or two, if you want to take a look at it.

marmoute added a comment.Jan 8 2021, 9:02 AM
In D9612#146250, @Alphare wrote:

Balancing both algorithms in my head is not easy, but this looks fine, of course as long as the next patch does something more useful. :P
Your commit message may be missing a word or two, if you want to take a look at it.

I did an extra pass on the commit message.

baymax edited the summary of this revision. (Show Details)Jan 8 2021, 10:25 AM
baymax updated this revision to Diff 24635.

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

baymax updated this revision to Diff 25738.Feb 22 2021, 10:27 AM

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

baymax updated this revision to Diff 25777.Feb 22 2021, 2:45 PM

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

marmoute added a commit: rHG842115e3b307: copies: rearrange all value comparison conditional.Feb 24 2021, 11:00 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG842115e3b307: copies: rearrange all value comparison conditional (authored by marmoute). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/copies.py (60 lines)
Mrust/hg-core/src/copy_tracing.rs (68 lines)

Diff 25842

mercurial/copies.py

else: else:
pick = _compare_values(changes, isancestor, dest, other, value) pick = _compare_values(changes, isancestor, dest, other, value)
if pick == PICK_MAJOR: if pick == PICK_MAJOR:
minor[dest] = value minor[dest] = value
return minor return minor
def _compare_values(changes, isancestor, dest, minor, major): def _compare_values(changes, isancestor, dest, minor, major):
"""compare two value within a _merge_copies_dict loop iteration""" """compare two value within a _merge_copies_dict loop iteration
return pick
- pick is one of PICK_MINOR, PICK_MAJOR or PICK_EITHER
"""
major_tt, major_value = major major_tt, major_value = major
minor_tt, minor_value = minor minor_tt, minor_value = minor
# evacuate some simple case first:
if major_tt == minor_tt: if major_tt == minor_tt:
# if it comes from the same revision it must be the same value # if it comes from the same revision it must be the same value
assert major_value == minor_value assert major_value == minor_value
return PICK_EITHER return PICK_EITHER
elif major[1] == minor[1]: elif (
return PICK_EITHER changes is not None
and minor_value is not None
# actual merging needed: content from "major" wins, unless it is older than and major_value is None
# the branch point or there is a merge and dest in changes.salvaged
elif changes is not None and major[1] is None and dest in changes.salvaged: ):
# In this case, a deletion was reverted, the "alive" value overwrite
# the deleted one.
return PICK_MINOR return PICK_MINOR
elif changes is not None and minor[1] is None and dest in changes.salvaged: elif (
changes is not None
and major_value is not None
and minor_value is None
and dest in changes.salvaged
):
# In this case, a deletion was reverted, the "alive" value overwrite
# the deleted one.
return PICK_MAJOR return PICK_MAJOR
elif changes is not None and dest in changes.merged: elif isancestor(minor_tt, major_tt):
if changes is not None and dest in changes.merged:
# change to dest happened on the branch without copy-source change,
# so both source are valid and "major" wins.
return PICK_MAJOR return PICK_MAJOR
elif not isancestor(major_tt, minor_tt): else:
if major[1] is not None:
return PICK_MAJOR return PICK_MAJOR
elif isancestor(minor_tt, major_tt): elif isancestor(major_tt, minor_tt):
if changes is not None and dest in changes.merged:
# change to dest happened on the branch without copy-source change,
# so both source are valid and "major" wins.
return PICK_MAJOR return PICK_MAJOR
else:
return PICK_MINOR return PICK_MINOR
elif minor_value is None:
# in case of conflict, the "alive" side wins.
return PICK_MAJOR
elif major_value is None:
# in case of conflict, the "alive" side wins.
return PICK_MINOR
else:
# in case of conflict where both side are alive, major wins.
return PICK_MAJOR
def _revinfo_getter_extra(repo): def _revinfo_getter_extra(repo):
"""return a function that return multiple data given a <rev>"i """return a function that return multiple data given a <rev>"i
* p1: revision number of first parent * p1: revision number of first parent
* p2: revision number of first parent * p2: revision number of first parent
* p1copies: mapping of copies from p1 * p1copies: mapping of copies from p1

rust/hg-core/src/copy_tracing.rs

MergePick::Minor MergePick::Minor
} else if src_major.path == src_minor.path { } else 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, they are both valid // If the two entry are identical, they are both valid
MergePick::Any MergePick::Any
} else if oracle.is_overwrite(src_major.rev, src_minor.rev) { } else if oracle.is_overwrite(src_major.rev, src_minor.rev) {
MergePick::Minor MergePick::Minor
} else if oracle.is_overwrite(src_minor.rev, src_major.rev) {
MergePick::Major
} else { } else {
MergePick::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" "conflicting information from p1 and p2 in the same revision"
); );
} else { } else {
let dest_path = path_map.untokenize(*dest); let dest_path = path_map.untokenize(*dest);
let action = changes.get_merge_case(dest_path); let action = changes.get_merge_case(dest_path);
if src_major.path.is_none() && action == MergeCase::Salvaged { if src_minor.path.is_some()
&& src_major.path.is_none()
&& 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 MergePick::Minor
} else if src_minor.path.is_none() && action == MergeCase::Salvaged { } else if src_major.path.is_some()
&& src_minor.path.is_none()
&& 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.
MergePick::Major MergePick::Major
} else if action == MergeCase::Merged { } else if oracle.is_overwrite(src_minor.rev, src_major.rev) {
// If the file was actively merged, copy information // The information from the minor version are strictly older than
// from each side might conflict. The major side will // the major version
// win such conflict. if action == MergeCase::Merged {
// If the file was actively merged, its means some non-copy
// activity happened on the other branch. It
// mean the older copy information are still relevant.
//
// The major side wins such conflict.
MergePick::Major MergePick::Major
} else if oracle.is_overwrite(src_major.rev, src_minor.rev) { } else {
// If the minor side is strictly newer than the major // No activity on the minor branch, pick the newer one.
// side, it should be kept.
MergePick::Minor
} else if src_major.path.is_some() {
// without any special case, the "major" value win
// other the "minor" one.
MergePick::Major MergePick::Major
} else if oracle.is_overwrite(src_minor.rev, src_major.rev) { }
// the "major" rev is a direct ancestors of "minor", } else if oracle.is_overwrite(src_major.rev, src_minor.rev) {
// any different value should if action == MergeCase::Merged {
// overwrite // If the file was actively merged, its means some non-copy
// activity happened on the other branch. It
// mean the older copy information are still relevant.
//
// The major side wins such conflict.
MergePick::Major MergePick::Major
} else { } else {
// major version is None (so the file was deleted on // No activity on the minor branch, pick the newer one.
// that branch) and that branch is independant (neither MergePick::Minor
// minor nor major is an ancestors of the other one.) }
// We preserve the new } else if src_minor.path.is_none() {
// information about the new file. // the minor side has no relevant information, pick the alive one
MergePick::Major
} else if src_major.path.is_none() {
// the major side has no relevant information, pick the alive one
MergePick::Minor MergePick::Minor
} else {
// by default the major side wins
MergePick::Major
} }
} }
} }