Patchwork D9612: copies: rearrange all value comparison conditional

login
register
mail settings
Submitter phabricator
Date Dec. 15, 2020, 9:08 a.m.
Message ID <differential-rev-PHID-DREV-seyro356etxpbkwtpu42-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/47910/
State Superseded
Headers show

Comments

phabricator - Dec. 15, 2020, 9:08 a.m.
marmoute created this revision.
Herald added a reviewer: hg-reviewers.
Herald added a subscriber: mercurial-patches.

REVISION SUMMARY
  To properly handle the newly tested case (chaining of merges) we will detect
  more accurately when an actualy merging of the copy information (and superseed
  the two existing data). Because starting to do so, we need to reorganise the
  value comparison to be able to make the difference when such actual merging is
  needed.
  
  To avoid mixing too many change in this complicated code, we do the
  reorganisation before adding the "overwrite detection" logic in the next
  changesets.

REPOSITORY
  rHG Mercurial

BRANCH
  default

REVISION DETAIL
  https://phab.mercurial-scm.org/D9612

AFFECTED FILES
  mercurial/copies.py
  rust/hg-core/src/copy_tracing.rs

CHANGE DETAILS




To: marmoute, #hg-reviewers
Cc: mercurial-patches, mercurial-devel

Patch

diff --git a/rust/hg-core/src/copy_tracing.rs b/rust/hg-core/src/copy_tracing.rs
--- a/rust/hg-core/src/copy_tracing.rs
+++ b/rust/hg-core/src/copy_tracing.rs
@@ -746,6 +746,8 @@ 
             MergePick::Any
         } else if oracle.is_overwrite(src_major.rev, src_minor.rev) {
             MergePick::Minor
+        } else if oracle.is_overwrite(src_minor.rev, src_major.rev) {
+            MergePick::Major
         } else {
             MergePick::Major
         }
@@ -753,45 +755,61 @@ 
         // We cannot get copy information for both p1 and p2 in the
         // same rev. So this is the same value.
         unreachable!(
-            "conflict information from p1 and p2 in the same revision"
+            "conflicting information from p1 and p2 in the same revision"
         );
     } else {
         let dest_path = path_map.untokenize(*dest);
         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
             // salvaged by the merge, we keep the minor side alive
             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
             // salvaged by the merge, unconditionnaly preserve the
             // major side.
             MergePick::Major
-        } else if action == MergeCase::Merged {
-            // If the file was actively merged, copy information
-            // from each side might conflict.  The major side will
-            // win such conflict.
-            MergePick::Major
+        } else if oracle.is_overwrite(src_minor.rev, src_major.rev) {
+            // The information from the minor version are strictly older than
+            // the major version
+            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
+            } else {
+                // No activity on the minor branch, pick the newer one.
+                MergePick::Major
+            }
         } else if oracle.is_overwrite(src_major.rev, src_minor.rev) {
-            // If the minor side is strictly newer than the major
-            // 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.
+            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
+            } else {
+                // No activity on the minor branch, pick the newer one.
+                MergePick::Minor
+            }
+        } else if src_minor.path.is_none() {
+            // the minor side has no relevant information, pick the alive one
             MergePick::Major
-        } else if oracle.is_overwrite(src_minor.rev, src_major.rev) {
-            // the "major" rev is a direct ancestors of "minor",
-            // any different value should
-            // overwrite
-            MergePick::Major
+        } else if src_major.path.is_none() {
+            // the major side has no relevant information, pick the alive one
+            MergePick::Minor
         } else {
-            // major version is None (so the file was deleted on
-            // that branch) and that branch is independant (neither
-            // minor nor major is an ancestors of the other one.)
-            // We preserve the new
-            // information about the new file.
-            MergePick::Minor
+            // by default the major side wins
+            MergePick::Major
         }
     }
 }
diff --git a/mercurial/copies.py b/mercurial/copies.py
--- a/mercurial/copies.py
+++ b/mercurial/copies.py
@@ -473,7 +473,12 @@ 
 
 
 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
     minor_tt, minor_value = minor
 
@@ -481,22 +486,47 @@ 
         # if it comes from the same revision it must be the same value
         assert major_value == minor_value
         return PICK_EITHER
-    elif major[1] == minor[1]:
-        return PICK_EITHER
-    # content from "major" wins, unless it is older
-    # than the branch point or there is a merge
-    elif changes is not None and major[1] is None and dest in changes.salvaged:
+    elif (
+        changes is not None
+        and minor_value is not None
+        and major_value is None
+        and dest in changes.salvaged
+    ):
+        # In this case, a deletion was reverted, the "alive" value overwrite
+        # the deleted one.
         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
-    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
+        else:
+            return PICK_MAJOR
+    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
+        else:
+            return PICK_MINOR
+    elif minor_value is None:
+        # in case of conflict, the "alive" side wins.
         return PICK_MAJOR
-    elif not isancestor(major_tt, minor_tt):
-        if major[1] is not None:
-            return PICK_MAJOR
-        elif isancestor(minor_tt, major_tt):
-            return PICK_MAJOR
-    return PICK_MINOR
+    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):