[Updated] D9613: copies: detect case when a merge decision overwrite previous data

marmoute (Pierre-Yves David) phabricator at mercurial-scm.org
Wed Feb 24 16:02:11 UTC 2021


Closed by commit rHG740234aca875: copies: detect case when a merge decision overwrite previous data (authored by marmoute).
This revision was automatically updated to reflect the committed changes.
This revision was not accepted when it landed; it landed in state "Needs Review".

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D9613?vs=25778&id=25843

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D9613/new/

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

AFFECTED FILES
  mercurial/copies.py
  rust/hg-core/src/copy_tracing.rs
  tests/test-copies-chain-merge.t

CHANGE DETAILS

diff --git a/tests/test-copies-chain-merge.t b/tests/test-copies-chain-merge.t
--- a/tests/test-copies-chain-merge.t
+++ b/tests/test-copies-chain-merge.t
@@ -3182,10 +3182,8 @@
   $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mK,AEm")' f
   A f
     a (filelog !)
-    a (missing-correct-output sidedata !)
-    a (missing-correct-output upgraded !)
-    b (known-bad-output sidedata !)
-    b (known-bad-output upgraded !)
+    a (sidedata !)
+    a (upgraded !)
 
 
 The result from mEAm is the same for the subsequent merge:
@@ -3205,10 +3203,8 @@
   $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mJ,EAm")' f
   A f
     a (filelog !)
-    b (missing-correct-output sidedata !)
-    b (missing-correct-output upgraded !)
-    a (known-bad-output sidedata !)
-    a (known-bad-output upgraded !)
+    b (sidedata !)
+    b (upgraded !)
 
 Subcase: chaining conflicting rename resolution
 ```````````````````````````````````````````````
@@ -3235,10 +3231,8 @@
   $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mT,PQm")' v
   A v
     r (filelog !)
-    p (missing-correct-output sidedata !)
-    p (missing-correct-output upgraded !)
-    r (known-bad-output sidedata !)
-    r (known-bad-output upgraded !)
+    p (sidedata !)
+    p (upgraded !)
 
 
 The result from mQPm is the same for the subsequent merge:
@@ -3254,10 +3248,8 @@
   $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mS,QPm")' v
   A v
     r (filelog !)
-    r (missing-correct-output sidedata !)
-    r (missing-correct-output upgraded !)
-    p (known-bad-output sidedata !)
-    p (known-bad-output upgraded !)
+    r (sidedata !)
+    r (upgraded !)
 
 
 Subcase: chaining salvage information during a merge
@@ -3271,9 +3263,7 @@
   $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mCB-revert-m-0")'
   M b
   A d
-    a (filelog !)
-    a (sidedata !)
-    a (upgraded !)
+    a (no-changeset no-compatibility !)
   A t
     p
   R a
@@ -3281,22 +3271,17 @@
   $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mBC-revert-m-0")'
   M b
   A d
-    a (filelog !)
-    a (sidedata !)
-    a (upgraded !)
+    a (no-changeset no-compatibility !)
   A t
     p
   R a
   R p
 
 chained output
-
   $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mBC+revert,Lm")'
   M b
   A d
-    a (filelog !)
-    a (missing-correct-output sidedata !)
-    a (missing-correct-output upgraded !)
+    a (no-changeset no-compatibility !)
   A t
     p
   A unrelated-l
@@ -3305,9 +3290,7 @@
   $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mCB+revert,Lm")'
   M b
   A d
-    a (filelog !)
-    a (missing-correct-output sidedata !)
-    a (missing-correct-output upgraded !)
+    a (no-changeset no-compatibility !)
   A t
     p
   A unrelated-l
@@ -3316,9 +3299,7 @@
   $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mL,BC+revertm")'
   M b
   A d
-    a (filelog !)
-    a (missing-correct-output sidedata !)
-    a (missing-correct-output upgraded !)
+    a (no-changeset no-compatibility !)
   A t
     p
   A unrelated-l
@@ -3327,9 +3308,7 @@
   $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mL,CB+revertm")'
   M b
   A d
-    a (filelog !)
-    a (missing-correct-output sidedata !)
-    a (missing-correct-output upgraded !)
+    a (no-changeset no-compatibility !)
   A t
     p
   A unrelated-l
@@ -3373,18 +3352,10 @@
 
   $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mGF,Nm")' d
   A d
-    a (filelog !)
-    a (missing-correct-output sidedata !)
-    a (missing-correct-output upgraded !)
-    h (known-bad-output sidedata !)
-    h (known-bad-output upgraded !)
+    a (no-changeset no-compatibility !)
   $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mN,GFm")' d
   A d
-    a (filelog !)
-    a (missing-correct-output sidedata !)
-    a (missing-correct-output upgraded !)
-    h (known-bad-output sidedata !)
-    h (known-bad-output upgraded !)
+    a (no-changeset no-compatibility !)
 
 
 Subcase: chaining conflicting rename resolution, with extra change during the merge
@@ -3411,11 +3382,7 @@
 
   $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mK,AE-change-m")' f
   A f
-    a (filelog !)
-    a (missing-correct-output sidedata !)
-    a (missing-correct-output upgraded !)
-    b (known-bad-output sidedata !)
-    b (known-bad-output upgraded !)
+    a (no-changeset no-compatibility !)
 
 
 The result from mEAm is the same for the subsequent merge:
@@ -3435,7 +3402,5 @@
   $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mJ,EA-change-m")' f
   A f
     a (filelog !)
-    b (missing-correct-output sidedata !)
-    b (missing-correct-output upgraded !)
-    a (known-bad-output sidedata !)
-    a (known-bad-output upgraded !)
+    b (sidedata !)
+    b (upgraded !)
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
@@ -568,20 +568,20 @@
     // This closure exist as temporary help while multiple developper are
     // actively working on this code. Feel free to re-inline it once this
     // code is more settled.
-    let mut cmp_value =
-        |dest: &PathToken,
-         src_minor: &TimeStampedPathCopy,
-         src_major: &TimeStampedPathCopy| {
-            compare_value(
-                path_map,
-                current_merge,
-                changes,
-                oracle,
-                dest,
-                src_minor,
-                src_major,
-            )
-        };
+    let cmp_value = |oracle: &mut AncestorOracle<A>,
+                     dest: &PathToken,
+                     src_minor: &TimeStampedPathCopy,
+                     src_major: &TimeStampedPathCopy| {
+        compare_value(
+            path_map,
+            current_merge,
+            changes,
+            oracle,
+            dest,
+            src_minor,
+            src_major,
+        )
+    };
     if minor.is_empty() {
         major
     } else if major.is_empty() {
@@ -605,11 +605,30 @@
         for (dest, src_minor) in minor {
             let src_major = major.get(&dest);
             match src_major {
-                None => major.insert(dest, src_minor),
+                None => {
+                    major.insert(dest, src_minor);
+                }
                 Some(src_major) => {
-                    match cmp_value(&dest, &src_minor, src_major) {
-                        MergePick::Any | MergePick::Major => None,
-                        MergePick::Minor => major.insert(dest, src_minor),
+                    let (pick, overwrite) =
+                        cmp_value(oracle, &dest, &src_minor, src_major);
+                    if overwrite {
+                        oracle.record_overwrite(src_minor.rev, current_merge);
+                        oracle.record_overwrite(src_major.rev, current_merge);
+                        let path = match pick {
+                            MergePick::Major => src_major.path,
+                            MergePick::Minor => src_minor.path,
+                            MergePick::Any => src_major.path,
+                        };
+                        let src = TimeStampedPathCopy {
+                            rev: current_merge,
+                            path,
+                        };
+                        major.insert(dest, src);
+                    } else {
+                        match pick {
+                            MergePick::Any | MergePick::Major => None,
+                            MergePick::Minor => major.insert(dest, src_minor),
+                        };
                     }
                 }
             };
@@ -621,11 +640,30 @@
         for (dest, src_major) in major {
             let src_minor = minor.get(&dest);
             match src_minor {
-                None => minor.insert(dest, src_major),
+                None => {
+                    minor.insert(dest, src_major);
+                }
                 Some(src_minor) => {
-                    match cmp_value(&dest, src_minor, &src_major) {
-                        MergePick::Any | MergePick::Minor => None,
-                        MergePick::Major => minor.insert(dest, src_major),
+                    let (pick, overwrite) =
+                        cmp_value(oracle, &dest, &src_major, src_minor);
+                    if overwrite {
+                        oracle.record_overwrite(src_minor.rev, current_merge);
+                        oracle.record_overwrite(src_major.rev, current_merge);
+                        let path = match pick {
+                            MergePick::Major => src_minor.path,
+                            MergePick::Minor => src_major.path,
+                            MergePick::Any => src_major.path,
+                        };
+                        let src = TimeStampedPathCopy {
+                            rev: current_merge,
+                            path,
+                        };
+                        minor.insert(dest, src);
+                    } else {
+                        match pick {
+                            MergePick::Any | MergePick::Major => None,
+                            MergePick::Minor => minor.insert(dest, src_major),
+                        };
                     }
                 }
             };
@@ -663,12 +701,32 @@
                 DiffItem::Update { old, new } => {
                     let (dest, src_major) = new;
                     let (_, src_minor) = old;
-                    match cmp_value(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!(),
+                    let (pick, overwrite) =
+                        cmp_value(oracle, dest, src_minor, src_major);
+                    if overwrite {
+                        oracle.record_overwrite(src_minor.rev, current_merge);
+                        oracle.record_overwrite(src_major.rev, current_merge);
+                        let path = match pick {
+                            MergePick::Major => src_major.path,
+                            MergePick::Minor => src_minor.path,
+                            // If the two entry are identical, no need to do
+                            // anything (but diff should not have yield them)
+                            MergePick::Any => src_major.path,
+                        };
+                        let src = TimeStampedPathCopy {
+                            rev: current_merge,
+                            path,
+                        };
+                        to_minor(dest, &src);
+                        to_major(dest, &src);
+                    } else {
+                        match pick {
+                            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!(),
+                        }
                     }
                 }
             };
@@ -717,39 +775,37 @@
     dest: &PathToken,
     src_minor: &TimeStampedPathCopy,
     src_major: &TimeStampedPathCopy,
-) -> MergePick {
+) -> (MergePick, bool) {
     if src_major.rev == current_merge {
         if src_minor.rev == current_merge {
             if src_major.path.is_none() {
                 // We cannot get different copy information for both p1 and p2
                 // from the same revision. Unless this was a
                 // deletion
-                MergePick::Any
+                (MergePick::Any, false)
             } else {
                 unreachable!();
             }
         } else {
             // The last value comes the current merge, this value -will- win
             // eventually.
-            oracle.record_overwrite(src_minor.rev, src_major.rev);
-            MergePick::Major
+            (MergePick::Major, true)
         }
     } else if src_minor.rev == current_merge {
         // The last value comes the current merge, this value -will- win
         // eventually.
-        oracle.record_overwrite(src_major.rev, src_minor.rev);
-        MergePick::Minor
+        (MergePick::Minor, true)
     } else if src_major.path == src_minor.path {
         // we have the same value, but from other source;
         if src_major.rev == src_minor.rev {
             // If the two entry are identical, they are both valid
-            MergePick::Any
+            (MergePick::Any, false)
         } else if oracle.is_overwrite(src_major.rev, src_minor.rev) {
-            MergePick::Minor
+            (MergePick::Minor, false)
         } else if oracle.is_overwrite(src_minor.rev, src_major.rev) {
-            MergePick::Major
+            (MergePick::Major, false)
         } else {
-            MergePick::Major
+            (MergePick::Any, true)
         }
     } else if src_major.rev == src_minor.rev {
         // We cannot get copy information for both p1 and p2 in the
@@ -766,7 +822,7 @@
         {
             // If the file is "deleted" in the major side but was
             // salvaged by the merge, we keep the minor side alive
-            MergePick::Minor
+            (MergePick::Minor, true)
         } else if src_major.path.is_some()
             && src_minor.path.is_none()
             && action == MergeCase::Salvaged
@@ -774,7 +830,7 @@
             // If the file is "deleted" in the minor side but was
             // salvaged by the merge, unconditionnaly preserve the
             // major side.
-            MergePick::Major
+            (MergePick::Major, true)
         } else if oracle.is_overwrite(src_minor.rev, src_major.rev) {
             // The information from the minor version are strictly older than
             // the major version
@@ -784,10 +840,10 @@
                 // mean the older copy information are still relevant.
                 //
                 // The major side wins such conflict.
-                MergePick::Major
+                (MergePick::Major, true)
             } else {
                 // No activity on the minor branch, pick the newer one.
-                MergePick::Major
+                (MergePick::Major, false)
             }
         } else if oracle.is_overwrite(src_major.rev, src_minor.rev) {
             if action == MergeCase::Merged {
@@ -796,20 +852,20 @@
                 // mean the older copy information are still relevant.
                 //
                 // The major side wins such conflict.
-                MergePick::Major
+                (MergePick::Major, true)
             } else {
                 // No activity on the minor branch, pick the newer one.
-                MergePick::Minor
+                (MergePick::Minor, false)
             }
         } else if src_minor.path.is_none() {
             // the minor side has no relevant information, pick the alive one
-            MergePick::Major
+            (MergePick::Major, true)
         } else if src_major.path.is_none() {
             // the major side has no relevant information, pick the alive one
-            MergePick::Minor
+            (MergePick::Minor, true)
         } else {
             // by default the major side wins
-            MergePick::Major
+            (MergePick::Major, true)
         }
     }
 }
diff --git a/mercurial/copies.py b/mercurial/copies.py
--- a/mercurial/copies.py
+++ b/mercurial/copies.py
@@ -434,7 +434,11 @@
                     # potential filelog related behavior.
                     assert parent == 2
                     current_copies = _merge_copies_dict(
-                        newcopies, current_copies, isancestor, changes
+                        newcopies,
+                        current_copies,
+                        isancestor,
+                        changes,
+                        current_rev,
                     )
             all_copies[current_rev] = current_copies
 
@@ -456,7 +460,7 @@
 PICK_EITHER = 2
 
 
-def _merge_copies_dict(minor, major, isancestor, changes):
+def _merge_copies_dict(minor, major, isancestor, changes, current_merge):
     """merge two copies-mapping together, minor and major
 
     In case of conflict, value from "major" will be picked.
@@ -474,8 +478,15 @@
         if other is None:
             minor[dest] = value
         else:
-            pick = _compare_values(changes, isancestor, dest, other, value)
-            if pick == PICK_MAJOR:
+            pick, overwrite = _compare_values(
+                changes, isancestor, dest, other, value
+            )
+            if overwrite:
+                if pick == PICK_MAJOR:
+                    minor[dest] = (current_merge, value[1])
+                else:
+                    minor[dest] = (current_merge, other[1])
+            elif pick == PICK_MAJOR:
                 minor[dest] = value
     return minor
 
@@ -483,9 +494,10 @@
 def _compare_values(changes, isancestor, dest, minor, major):
     """compare two value within a _merge_copies_dict loop iteration
 
-    return pick
+    return (pick, overwrite).
 
     - pick is one of PICK_MINOR, PICK_MAJOR or PICK_EITHER
+    - overwrite is True if pick is a return of an ambiguity that needs resolution.
     """
     major_tt, major_value = major
     minor_tt, minor_value = minor
@@ -493,7 +505,7 @@
     if major_tt == minor_tt:
         # if it comes from the same revision it must be the same value
         assert major_value == minor_value
-        return PICK_EITHER
+        return PICK_EITHER, False
     elif (
         changes is not None
         and minor_value is not None
@@ -502,7 +514,7 @@
     ):
         # In this case, a deletion was reverted, the "alive" value overwrite
         # the deleted one.
-        return PICK_MINOR
+        return PICK_MINOR, True
     elif (
         changes is not None
         and major_value is not None
@@ -511,30 +523,30 @@
     ):
         # In this case, a deletion was reverted, the "alive" value overwrite
         # the deleted one.
-        return PICK_MAJOR
+        return PICK_MAJOR, True
     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, True
         else:
-            return PICK_MAJOR
+            return PICK_MAJOR, False
     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, True
         else:
-            return PICK_MINOR
+            return PICK_MINOR, False
     elif minor_value is None:
         # in case of conflict, the "alive" side wins.
-        return PICK_MAJOR
+        return PICK_MAJOR, True
     elif major_value is None:
         # in case of conflict, the "alive" side wins.
-        return PICK_MINOR
+        return PICK_MINOR, True
     else:
         # in case of conflict where both side are alive, major wins.
-        return PICK_MAJOR
+        return PICK_MAJOR, True
 
 
 def _revinfo_getter_extra(repo):



To: marmoute, #hg-reviewers, Alphare
Cc: Alphare, mercurial-patches
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-patches/attachments/20210224/05c4ae12/attachment-0002.html>


More information about the Mercurial-patches mailing list