Patchwork D8392: [RFC] mergestate: store about files resolved in favour of other

login
register
mail settings
Submitter phabricator
Date April 10, 2020, 10:44 a.m.
Message ID <differential-rev-PHID-DREV-nqrorlcdh5epp55xzma2-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/46055/
State Superseded
Headers show

Comments

phabricator - April 10, 2020, 10:44 a.m.
pulkit created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This starts storing information in mergestate about files which were
  automatically merged and the other/remote version of file was used.
  We need this information at commit to pick the filenode parent for the new
  commit. The commit code has weird filenode resolution here which does not depend
  on what merge did and rely on some file ancestry logic. That leads to problem in
  some cases, the tests which are changed by this patch.
  
  Somethings which can be further investigated are:
  
  1. refactoring of commit logic more to depend on this information
  2. maybe a more generic solution?

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  mercurial/commands.py
  mercurial/localrepo.py
  mercurial/merge.py
  tests/test-convert-hg-source.t
  tests/test-copies-chain-merge.t

CHANGE DETAILS




To: pulkit, #hg-reviewers
Cc: mercurial-devel
phabricator - April 10, 2020, 11:40 a.m.
pulkit added a comment.


  This attempts to fix the issue which D8243 <https://phab.mercurial-scm.org/D8243> tries to fix by storing extra information in mergestate.

REPOSITORY
  rHG Mercurial

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

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

To: pulkit, #hg-reviewers
Cc: mercurial-devel
phabricator - April 10, 2020, 11:56 a.m.
marmoute added a comment.
marmoute accepted this revision.


  The approach seems good and the test change seems consistent with D8243 <https://phab.mercurial-scm.org/D8243> (@pulkit can you confirm there are no difference?)
  
  This looks good to me.

REPOSITORY
  rHG Mercurial

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

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

To: pulkit, #hg-reviewers, marmoute
Cc: marmoute, mercurial-devel
phabricator - April 10, 2020, 4 p.m.
durin42 added a comment.
durin42 accepted this revision as: durin42.
durin42 added subscribers: martinvonz, durin42.


  I like it, but want to get @martinvonz to look too.

REPOSITORY
  rHG Mercurial

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

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

To: pulkit, #hg-reviewers, marmoute, durin42
Cc: durin42, martinvonz, marmoute, mercurial-devel
phabricator - Oct. 6, 2020, 8:29 p.m.
Herald added a subscriber: mercurial-patches.
martinvonz added inline comments.

INLINE COMMENTS

> test-copies-chain-merge.t:311-313
>  Note:
>  | In this case, one of the merge wrongly record a merge while there is none.
>  | This lead to bad copy tracing information to be dug up.

Could you send a follow-up updating this?

> test-copies-chain-merge.t:367
>  
>  The 0bb5445dc4d02f4e0d86cf16f9f3a411d0f17744 entry is wrong, since the file was
>  deleted on one side (then recreate) and untouched on the other side, no "merge"

The hash no longer exists, and maybe it's no longer wrong?

> test-copies-chain-merge.t:381
>  
>  (This `hg log` output if wrong, since no merge actually happened).
>  

And I don't think this is wrong anymore (i.e. the comment is now wrong)

> test-copies-chain-merge.t:388
>  
>  This `hg log` output is correct
>  

Would be clearer to update this too. Either simply delete it or say what it should show in order to be correct (e.g. "log output should not include the merge commit").

> test-copies-chain-merge.t:549
>    
>  The overwriting should take over. However, the behavior is currently buggy
>  

Is this still true?

> test-copies-chain-merge.t:555
> +    h
>      h (false !)
>    R a

delete

> test-copies-chain-merge.t:583
>  
>  The following graphlog is wrong, the "a -> c -> d" chain was overwritten and should not appear.
>  

And this comment also seems wrong now

> test-copies-chain-merge.t:593
>  
>  The following output is correct.
>  

As above, delete to clarify

REPOSITORY
  rHG Mercurial

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

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

To: pulkit, #hg-reviewers, marmoute, durin42, martinvonz
Cc: mercurial-patches, durin42, martinvonz, marmoute, mercurial-devel
phabricator - Oct. 7, 2020, 7:59 a.m.
pulkit added a comment.


  @martinvonz sent D9168 <https://phab.mercurial-scm.org/D9168> as followup for all the comments.

REPOSITORY
  rHG Mercurial

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

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

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

Patch

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
@@ -319,7 +319,7 @@ 
   (branch merge, don't forget to commit)
   $ hg ci -m 'mBDm-0 simple merge - one way'
   $ hg up 'desc("d-2")'
-  2 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ hg merge 'desc("b-1")'
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   (branch merge, don't forget to commit)
@@ -348,7 +348,6 @@ 
   M d
   $ hg status --copies --rev 'desc("d-2")' --rev 'desc("mBDm-0")'
   M b
-  M d
   $ hg status --copies --rev 'desc("d-2")' --rev 'desc("mDBm-0")'
   M b
   $ hg status --copies --rev 'desc("i-2")' --rev 'desc("mBDm-0")'
@@ -361,7 +360,7 @@ 
 The bugs makes recorded copy is different depending of where we started the merge from since
 
   $ hg manifest --debug --rev 'desc("mBDm-0")' | grep '644   d'
-  0bb5445dc4d02f4e0d86cf16f9f3a411d0f17744 644   d
+  b004912a8510032a0350a74daa2803dadfb00e12 644   d
   $ hg manifest --debug --rev 'desc("mDBm-0")' | grep '644   d'
   b004912a8510032a0350a74daa2803dadfb00e12 644   d
 
@@ -378,21 +377,13 @@ 
      rev linkrev nodeid       p1           p2
        0       2 01c2f5eabdc4 000000000000 000000000000
        1       8 b004912a8510 000000000000 000000000000
-       2      17 0bb5445dc4d0 01c2f5eabdc4 b004912a8510
 
 (This `hg log` output if wrong, since no merge actually happened).
 
   $ hg log -Gfr 'desc("mBDm-0")' d
-  o    17 mBDm-0 simple merge - one way
-  |\
-  o :  8 d-2 re-add d
-  :/
-  o  2 i-2: c -move-> d
+  o  8 d-2 re-add d
   |
-  o  1 i-1: a -move-> c
-  |
-  o  0 i-0 initial commit: a b h
-  
+  ~
 
 This `hg log` output is correct
 
@@ -404,7 +395,6 @@ 
   $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mBDm-0")'
   M b
   A d
-    a
   R a
   $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mDBm-0")'
   M b
@@ -525,8 +515,7 @@ 
      rev linkrev nodeid       p1           p2
        0       2 01c2f5eabdc4 000000000000 000000000000
        1       8 b004912a8510 000000000000 000000000000
-       2      17 0bb5445dc4d0 01c2f5eabdc4 b004912a8510
-       3      22 c72365ee036f 000000000000 000000000000
+       2      22 c72365ee036f 000000000000 000000000000
   $ hg up 'desc("b-1")'
   3 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ hg merge 'desc("f-2")'
@@ -534,7 +523,7 @@ 
   (branch merge, don't forget to commit)
   $ hg ci -m 'mBFm-0 simple merge - one way'
   $ hg up 'desc("f-2")'
-  2 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ hg merge 'desc("b-1")'
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   (branch merge, don't forget to commit)
@@ -562,7 +551,7 @@ 
   $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mBFm-0")'
   M b
   A d
-    a (true !)
+    h
     h (false !)
   R a
   R h
@@ -577,7 +566,6 @@ 
   R h
   $ hg status --copies --rev 'desc("f-2")' --rev 'desc("mBFm-0")'
   M b
-  M d
   $ hg status --copies --rev 'desc("f-1")' --rev 'desc("mBFm-0")'
   M b
   M d
@@ -595,16 +583,10 @@ 
 The following graphlog is wrong, the "a -> c -> d" chain was overwritten and should not appear.
 
   $ hg log -Gfr 'desc("mBFm-0")' d
-  o    23 mBFm-0 simple merge - one way
-  |\
-  o :  22 f-2: rename i -> d
-  | :
-  o :  21 f-1: rename h -> i
-  :/
-  o  2 i-2: c -move-> d
+  o  22 f-2: rename i -> d
   |
-  o  1 i-1: a -move-> c
-  |
+  o  21 f-1: rename h -> i
+  :
   o  0 i-0 initial commit: a b h
   
 
diff --git a/tests/test-convert-hg-source.t b/tests/test-convert-hg-source.t
--- a/tests/test-convert-hg-source.t
+++ b/tests/test-convert-hg-source.t
@@ -62,9 +62,9 @@ 
   6 make bar and baz copies of foo
   5 merge local copy
   4 merge remote copy
-  3 Added tag that for changeset 88586c4e9f02
+  3 Added tag that for changeset 8601262d7472
   2 Removed tag that
-  1 Added tag this for changeset c56a7f387039
+  1 Added tag this for changeset 706614b458c1
   0 mark baz executable
   updating bookmarks
   $ cd new
@@ -76,7 +76,7 @@ 
 #if execbit
   $ hg bookmarks
      premerge1                 3:973ef48a98a4
-     premerge2                 8:91d107c423ba
+     premerge2                 8:c4968fdf2e5d
 #else
 Different hash because no x bit
   $ hg bookmarks
@@ -96,19 +96,19 @@ 
   6 make bar and baz copies of foo
   5 merge local copy
   4 merge remote copy
-  3 Added tag that for changeset 88586c4e9f02
+  3 Added tag that for changeset 8601262d7472
   2 Removed tag that
-  1 Added tag this for changeset c56a7f387039
+  1 Added tag this for changeset 706614b458c1
   0 mark baz executable
   updating bookmarks
   $ hg -R new log -G -T '{rev} {desc}'
   o  8 mark baz executable
   |
-  o  7 Added tag this for changeset c56a7f387039
+  o  7 Added tag this for changeset 706614b458c1
   |
   o  6 Removed tag that
   |
-  o  5 Added tag that for changeset 88586c4e9f02
+  o  5 Added tag that for changeset 8601262d7472
   |
   o    4 merge remote copy
   |\
diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -64,6 +64,7 @@ 
 RECORD_OVERRIDE = b't'
 RECORD_UNSUPPORTED_MANDATORY = b'X'
 RECORD_UNSUPPORTED_ADVISORY = b'x'
+RECORD_RESOLVED_OTHER = b'R'
 
 MERGE_DRIVER_STATE_UNMARKED = b'u'
 MERGE_DRIVER_STATE_MARKED = b'm'
@@ -74,6 +75,9 @@ 
 MERGE_RECORD_UNRESOLVED_PATH = b'pu'
 MERGE_RECORD_RESOLVED_PATH = b'pr'
 MERGE_RECORD_DRIVER_RESOLVED = b'd'
+# represents that the file was automatically merged in favor
+# of other version. This info is used on commit.
+MERGE_RECORD_MERGED_OTHER = b'o'
 
 ACTION_FORGET = b'f'
 ACTION_REMOVE = b'r'
@@ -91,6 +95,8 @@ 
 ACTION_KEEP = b'k'
 ACTION_EXEC = b'e'
 ACTION_CREATED_MERGE = b'cm'
+# GET the other/remote side and store this info in mergestate
+ACTION_GET_OTHER_AND_STORE = b'gs'
 
 
 class mergestate(object):
@@ -227,6 +233,7 @@ 
                 RECORD_CHANGEDELETE_CONFLICT,
                 RECORD_PATH_CONFLICT,
                 RECORD_MERGE_DRIVER_MERGE,
+                RECORD_RESOLVED_OTHER,
             ):
                 bits = record.split(b'\0')
                 self._state[bits[0]] = bits[1:]
@@ -453,6 +460,10 @@ 
                 records.append(
                     (RECORD_PATH_CONFLICT, b'\0'.join([filename] + v))
                 )
+            elif v[0] == MERGE_RECORD_MERGED_OTHER:
+                records.append(
+                    (RECORD_RESOLVED_OTHER, b'\0'.join([filename] + v))
+                )
             elif v[1] == nullhex or v[6] == nullhex:
                 # Change/Delete or Delete/Change conflicts. These are stored in
                 # 'C' records. v[1] is the local file, and is nullhex when the
@@ -551,6 +562,10 @@ 
         self._state[path] = [MERGE_RECORD_UNRESOLVED_PATH, frename, forigin]
         self._dirty = True
 
+    def addmergedother(self, path):
+        self._state[path] = [MERGE_RECORD_MERGED_OTHER, nullhex, nullhex]
+        self._dirty = True
+
     def __contains__(self, dfile):
         return dfile in self._state
 
@@ -594,6 +609,8 @@ 
         """rerun merge process for file path `dfile`"""
         if self[dfile] in (MERGE_RECORD_RESOLVED, MERGE_RECORD_DRIVER_RESOLVED):
             return True, 0
+        if self._state[dfile][0] == MERGE_RECORD_MERGED_OTHER:
+            return True, 0
         stateentry = self._state[dfile]
         state, localkey, lfile, afile, anode, ofile, onode, flags = stateentry
         octx = self._repo[self._other]
@@ -1209,7 +1226,7 @@ 
     narrowed.
     """
     nooptypes = {b'k'}  # TODO: handle with nonconflicttypes
-    nonconflicttypes = set(b'a am c cm f g r e'.split())
+    nonconflicttypes = set(b'a am c cm f g gs r e'.split())
     # We mutate the items in the dict during iteration, so iterate
     # over a copy.
     for f, action in list(actions.items()):
@@ -1348,14 +1365,22 @@ 
                         )
                     else:
                         actions[f] = (
-                            ACTION_GET,
+                            ACTION_GET_OTHER_AND_STORE
+                            if branchmerge
+                            else ACTION_GET,
                             (fl2, False),
                             b'remote is newer',
                         )
                 elif nol and n2 == a:  # remote only changed 'x'
                     actions[f] = (ACTION_EXEC, (fl2,), b'update permissions')
                 elif nol and n1 == a:  # local only changed 'x'
-                    actions[f] = (ACTION_GET, (fl1, False), b'remote is newer')
+                    actions[f] = (
+                        ACTION_GET_OTHER_AND_STORE
+                        if branchmerge
+                        else ACTION_GET,
+                        (fl1, False),
+                        b'remote is newer',
+                    )
                 else:  # both changed something
                     actions[f] = (
                         ACTION_MERGE,
@@ -1588,6 +1613,8 @@ 
 
             for f, a in sorted(pycompat.iteritems(actions)):
                 m, args, msg = a
+                if m == ACTION_GET_OTHER_AND_STORE:
+                    m = ACTION_GET
                 repo.ui.debug(b' %s: %s -> %s\n' % (f, msg, m))
                 if f in fbids:
                     d = fbids[f]
@@ -1813,6 +1840,7 @@ 
             ACTION_KEEP,
             ACTION_PATH_CONFLICT,
             ACTION_PATH_CONFLICT_RESOLVE,
+            ACTION_GET_OTHER_AND_STORE,
         )
     }
 
@@ -1835,6 +1863,11 @@ 
 
     updated, merged, removed = 0, 0, 0
     ms = mergestate.clean(repo, wctx.p1().node(), mctx.node(), labels)
+
+    # add ACTION_GET_OTHER_AND_STORE to mergestate
+    for e in actions[ACTION_GET_OTHER_AND_STORE]:
+        ms.addmergedother(e[0])
+
     moves = []
     for m, l in actions.items():
         l.sort()
@@ -2415,6 +2448,7 @@ 
                     ACTION_EXEC,
                     ACTION_REMOVE,
                     ACTION_PATH_CONFLICT_RESOLVE,
+                    ACTION_GET_OTHER_AND_STORE,
                 ):
                     msg = _(b"conflicting changes")
                     hint = _(b"commit or update --clean to discard changes")
@@ -2477,6 +2511,10 @@ 
                 actions[m] = []
             actions[m].append((f, args, msg))
 
+        # ACTION_GET_OTHER_AND_STORE is a ACTION_GET + store in mergestate
+        for e in actions[ACTION_GET_OTHER_AND_STORE]:
+            actions[ACTION_GET].append(e)
+
         if not util.fscasesensitive(repo.path):
             # check collision between files only in p2 for clean update
             if not branchmerge and (
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -2855,6 +2855,14 @@ 
                 fparent1, fparent2 = fparent2, nullid
             elif fparent2 in fparentancestors:
                 fparent2 = nullid
+            elif not fparentancestors:
+                # TODO: this whole if-else might be simplified much more
+                ms = mergemod.mergestate.read(self)
+                if (
+                    fname in ms
+                    and ms[fname] == mergemod.MERGE_RECORD_MERGED_OTHER
+                ):
+                    fparent1, fparent2 = fparent2, nullid
 
         # is the file changed?
         text = fctx.data()
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -5955,6 +5955,8 @@ 
             if not m(f):
                 continue
 
+            if ms[f] == mergemod.MERGE_RECORD_MERGED_OTHER:
+                continue
             label, key = mergestateinfo[ms[f]]
             fm.startitem()
             fm.context(ctx=wctx)
@@ -6002,6 +6004,9 @@ 
 
             didwork = True
 
+            if ms[f] == mergemod.MERGE_RECORD_MERGED_OTHER:
+                continue
+
             # don't let driver-resolved files be marked, and run the conclude
             # step if asked to resolve
             if ms[f] == mergemod.MERGE_RECORD_DRIVER_RESOLVED: