[PATCH 1 of 8] tests: add criss cross merging tests whose behavior need to be fixed

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 1 of 8] tests: add criss cross merging tests whose behavior need to be fixed

Pulkit Goyal
# HG changeset patch
# User Pierre-Yves David <[hidden email]>
# Date 1592566066 -7200
#      Fri Jun 19 13:27:46 2020 +0200
# Node ID eab095687e7b655011a26001dc83c802ab474ceb
# Parent  6df7dc3e9c3578587c96f244b685d284d114f732
# EXP-Topic merge-newnode
tests: add criss cross merging tests whose behavior need to be fixed

Merging two changesets can mark a file as removed post merge. However, in some
cases, a user might not want to remove that file and they revert the removal
back and commit the merge. All this works perfectly well.

However, when we do criss-cross merges with such merge where user explicitly
choose to revert the removal with one where another user choose the removal,
we does not get any conflict.

The intent here is conflicting and merge should result in conflicts. One user
merged and want to keep the file while other user merged and want to remove the
file. Merging those merges should result in conflicts.

This patch adds test cases for these cases.

Differential Revision: https://phab.mercurial-scm.org/D8939

diff --git a/tests/test-merge-criss-cross.t b/tests/test-merge-criss-cross.t
--- a/tests/test-merge-criss-cross.t
+++ b/tests/test-merge-criss-cross.t
@@ -469,3 +469,225 @@ Verify that the old context ancestor wor
   getting d2/b
   1 files updated, 0 files merged, 2 files removed, 0 files unresolved
   (branch merge, don't forget to commit)
+
+
+Check that removal reversion does not go unotified
+==================================================
+
+On a merge, a file can be removed and user can revert that removal. This means
+user has made an explicit choice of keeping the file or reverting the removal
+even though the merge algo wanted to remove it.
+Based on this, when we do criss cross merges, merge algorithm should not again
+choose to remove the file as in one of the merges, user made an explicit choice
+to revert the removal.
+Following test cases demonstrate how merge algo does not take in account
+explicit choices made by users to revert the removal and on criss-cross merging
+removes the file again.
+
+"Simple" case where the filenode changes
+----------------------------------------
+
+  $ cd ..
+  $ hg init criss-cross-merge-reversal-with-update
+  $ cd criss-cross-merge-reversal-with-update
+  $ echo the-file > the-file
+  $ echo other-file > other-file
+  $ hg add the-file other-file
+  $ hg ci -m 'root-commit'
+  $ echo foo >> the-file
+  $ echo bar >> other-file
+  $ hg ci -m 'updating-both-file'
+  $ hg up 'desc("root-commit")'
+  2 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ hg rm the-file
+  $ hg ci -m 'delete-the-file'
+  created new head
+  $ hg log -G -T '{node|short} {desc}\n'
+  @  7801bc9b9899 delete-the-file
+  |
+  | o  9b610631ab29 updating-both-file
+  |/
+  o  955800955977 root-commit
+  
+
+Do all the merge combination (from the deleted or the update side × keeping and deleting the file
+
+  $ hg update 'desc("delete-the-file")'
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ hg merge 'desc("updating-both-file")' -t :local
+  1 files updated, 1 files merged, 0 files removed, 0 files unresolved
+  (branch merge, don't forget to commit)
+  $ hg ci -m "merge-deleting-the-file-from-deleted"
+  $ hg manifest
+  other-file
+
+  $ hg update 'desc("updating-both-file")'
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ hg merge 'desc("delete-the-file")' -t :other
+  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  (branch merge, don't forget to commit)
+  $ hg ci -m "merge-deleting-the-file-from-updated"
+  created new head
+  $ hg manifest
+  other-file
+
+  $ hg update 'desc("delete-the-file")'
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ hg merge 'desc("updating-both-file")' -t :other
+  1 files updated, 1 files merged, 0 files removed, 0 files unresolved
+  (branch merge, don't forget to commit)
+  $ hg ci -m "merge-keeping-the-file-from-deleted"
+  created new head
+  $ hg manifest
+  other-file
+  the-file
+
+  $ hg update 'desc("updating-both-file")'
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ hg merge 'desc("delete-the-file")' -t :local
+  0 files updated, 1 files merged, 0 files removed, 0 files unresolved
+  (branch merge, don't forget to commit)
+  $ hg ci -m "merge-keeping-the-file-from-updated"
+  created new head
+  $ hg manifest
+  other-file
+  the-file
+
+There the resulting merge together (leading to criss cross situation). Check
+the conflict is properly detected.
+
+(merging two deletion together → no conflict)
+
+  $ hg update --clean 'desc("merge-deleting-the-file-from-deleted")'
+  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  $ hg merge          'desc("merge-deleting-the-file-from-updated")'
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  (branch merge, don't forget to commit)
+  $ ls -1
+  other-file
+
+(merging a deletion with keeping → conflict)
+BROKEN: this should result in conflict
+
+  $ hg update --clean 'desc("merge-deleting-the-file-from-deleted")'
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ hg merge          'desc("merge-keeping-the-file-from-deleted")'
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  (branch merge, don't forget to commit)
+  $ ls -1
+  other-file
+  the-file
+
+(merging a deletion with keeping → conflict)
+BROKEN: this should result in conflict
+
+  $ hg update --clean 'desc("merge-deleting-the-file-from-deleted")'
+  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  $ hg merge          'desc("merge-keeping-the-file-from-updated")'
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  (branch merge, don't forget to commit)
+  $ ls -1
+  other-file
+  the-file
+
+(merging two deletion together → no conflict)
+
+  $ hg update --clean 'desc("merge-deleting-the-file-from-updated")'
+  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  $ hg merge          'desc("merge-deleting-the-file-from-deleted")'
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  (branch merge, don't forget to commit)
+  $ ls -1
+  other-file
+
+(merging a deletion with keeping → conflict)
+BROKEN: this should result in conflict
+
+  $ hg update --clean 'desc("merge-deleting-the-file-from-updated")'
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ hg merge          'desc("merge-keeping-the-file-from-deleted")'
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  (branch merge, don't forget to commit)
+  $ ls -1
+  other-file
+  the-file
+
+(merging a deletion with keeping → conflict)
+BROKEN: this should result in conflict
+
+  $ hg update --clean 'desc("merge-deleting-the-file-from-updated")'
+  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  $ hg merge          'desc("merge-keeping-the-file-from-updated")'
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  (branch merge, don't forget to commit)
+  $ ls -1
+  other-file
+  the-file
+
+(merging two "keeping" together → no conflict)
+
+  $ hg update --clean 'desc("merge-keeping-the-file-from-updated")'
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ hg merge          'desc("merge-keeping-the-file-from-deleted")'
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  (branch merge, don't forget to commit)
+  $ ls -1
+  other-file
+  the-file
+
+(merging a deletion with keeping → conflict)
+BROKEN: this should result in conflict
+
+  $ hg update --clean 'desc("merge-keeping-the-file-from-updated")'
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ hg merge          'desc("merge-deleted-the-file-from-deleted")'
+  abort: empty revision set
+  [255]
+  $ ls -1
+  other-file
+  the-file
+
+(merging a deletion with keeping → conflict)
+BROKEN: this should result in conflict
+
+  $ hg update --clean 'desc("merge-keeping-the-file-from-updated")'
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ hg merge          'desc("merge-deleting-the-file-from-updated")'
+  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  (branch merge, don't forget to commit)
+  $ ls -1
+  other-file
+
+(merging two "keeping" together → no conflict)
+
+  $ hg update --clean 'desc("merge-keeping-the-file-from-deleted")'
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ hg merge          'desc("merge-keeping-the-file-from-updated")'
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  (branch merge, don't forget to commit)
+  $ ls -1
+  other-file
+  the-file
+
+(merging a deletion with keeping → conflict)
+BROKEN: this should result in conflict
+
+  $ hg update --clean 'desc("merge-keeping-the-file-from-deleted")'
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ hg merge          'desc("merge-deleted-the-file-from-deleted")'
+  abort: empty revision set
+  [255]
+  $ ls -1
+  other-file
+  the-file
+
+(merging a deletion with keeping → conflict)
+BROKEN: this should result in conflict
+
+  $ hg update --clean 'desc("merge-keeping-the-file-from-deleted")'
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ hg merge          'desc("merge-deleting-the-file-from-updated")'
+  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  (branch merge, don't forget to commit)
+  $ ls -1
+  other-file
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 2 of 8] mergeresult: introduce dedicated tuple for no-op actions

Pulkit Goyal
# HG changeset patch
# User Pulkit Goyal <[hidden email]>
# Date 1600071699 -19800
#      Mon Sep 14 13:51:39 2020 +0530
# Node ID cfabb1ccdf7a2cb67bb2819f798bad53c6aac977
# Parent  eab095687e7b655011a26001dc83c802ab474ceb
# EXP-Topic merge-newnode
mergeresult: introduce dedicated tuple for no-op actions

This will help us in adding more no-op actions in next patch while keeping the
code cleaner.

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -523,7 +523,6 @@ def _filternarrowactions(narrowmatch, br
     narrowed.
     """
     # TODO: handle with nonconflicttypes
-    nooptypes = {mergestatemod.ACTION_KEEP}
     nonconflicttypes = {
         mergestatemod.ACTION_ADD,
         mergestatemod.ACTION_ADD_MODIFIED,
@@ -541,7 +540,7 @@ def _filternarrowactions(narrowmatch, br
             pass
         elif not branchmerge:
             mresult.removefile(f)  # just updating, ignore changes outside clone
-        elif action[0] in nooptypes:
+        elif action[0] in mergeresult.NO_OP_ACTIONS:
             mresult.removefile(f)  # merge does not affect file
         elif action[0] in nonconflicttypes:
             raise error.Abort(
@@ -564,6 +563,8 @@ class mergeresult(object):
     It has information about what actions need to be performed on dirstate
     mapping of divergent renames and other such cases. '''
 
+    NO_OP_ACTIONS = (mergestatemod.ACTION_KEEP,)
+
     def __init__(self):
         """
         filemapping: dict of filename as keys and action related info as values
@@ -711,12 +712,12 @@ class mergeresult(object):
                 a
                 not in (
                     mergestatemod.ACTION_GET,
-                    mergestatemod.ACTION_KEEP,
                     mergestatemod.ACTION_EXEC,
                     mergestatemod.ACTION_REMOVE,
                     mergestatemod.ACTION_PATH_CONFLICT_RESOLVE,
                 )
                 and self._actionmapping[a]
+                and a not in self.NO_OP_ACTIONS
             ):
                 return True
 
@@ -1376,7 +1377,7 @@ def applyupdates(
         # mergestate so that it can be reused on commit
         ms.addcommitinfo(f, op)
 
-    numupdates = mresult.len() - mresult.len((mergestatemod.ACTION_KEEP,))
+    numupdates = mresult.len() - mresult.len(mergeresult.NO_OP_ACTIONS)
     progress = repo.ui.makeprogress(
         _(b'updating'), unit=_(b'files'), total=numupdates
     )
diff --git a/mercurial/sparse.py b/mercurial/sparse.py
--- a/mercurial/sparse.py
+++ b/mercurial/sparse.py
@@ -399,7 +399,7 @@ def filterupdatesactions(repo, wctx, mct
             temporaryfiles.append(file)
             prunedactions[file] = action
         elif branchmerge:
-            if type != mergestatemod.ACTION_KEEP:
+            if type not in mergemod.mergeresult.NO_OP_ACTIONS:
                 temporaryfiles.append(file)
                 prunedactions[file] = action
         elif type == mergestatemod.ACTION_FORGET:
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 3 of 8] merge: add `ACTION_KEEP_ABSENT` to represent files we want to keep absent

Pulkit Goyal
In reply to this post by Pulkit Goyal
# HG changeset patch
# User Pulkit Goyal <[hidden email]>
# Date 1598960306 -19800
#      Tue Sep 01 17:08:26 2020 +0530
# Node ID 2c58747e0a1ebe17b6bf1f4f393da2be57bce63a
# Parent  cfabb1ccdf7a2cb67bb2819f798bad53c6aac977
# EXP-Topic merge-newnode
merge: add `ACTION_KEEP_ABSENT` to represent files we want to keep absent

There are files which were deleted/not present in working copy parent but were
present on other side of merge. On merge, we might decide to keep them deleted.
We want to track such cases more closely, rather all kind of cases which results
from some kind of merging logic.

We do have `ACTION_KEEP` but having a dedicated action for the absent case is
more cleaner.

Initially I named the action as `ACTION_KEEP_DELETED` but later realized that
file can be not-present because of other reasons than deletion like rename,
hence decided to use more generic name `ACTION_KEEP_ABSENT`.

Differential Revision: https://phab.mercurial-scm.org/D8974

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -563,7 +563,10 @@ class mergeresult(object):
     It has information about what actions need to be performed on dirstate
     mapping of divergent renames and other such cases. '''
 
-    NO_OP_ACTIONS = (mergestatemod.ACTION_KEEP,)
+    NO_OP_ACTIONS = (
+        mergestatemod.ACTION_KEEP,
+        mergestatemod.ACTION_KEEP_ABSENT,
+    )
 
     def __init__(self):
         """
@@ -1175,6 +1178,11 @@ def calculateupdates(
                 repo.ui.note(_(b" %s: picking 'keep' action\n") % f)
                 mresult.addfile(f, *bids[mergestatemod.ACTION_KEEP][0])
                 continue
+            # If keep absent is an option, just do that
+            if mergestatemod.ACTION_KEEP_ABSENT in bids:
+                repo.ui.note(_(b" %s: picking 'keep absent' action\n") % f)
+                mresult.addfile(f, *bids[mergestatemod.ACTION_KEEP_ABSENT][0])
+                continue
             # If there are gets and they all agree [how could they not?], do it.
             if mergestatemod.ACTION_GET in bids:
                 ga0 = bids[mergestatemod.ACTION_GET][0]
@@ -1486,6 +1494,11 @@ def applyupdates(
     ):
         repo.ui.debug(b" %s: %s -> k\n" % (f, msg))
         # no progress
+    for f, args, msg in mresult.getactions(
+        (mergestatemod.ACTION_KEEP_ABSENT,), sort=True
+    ):
+        repo.ui.debug(b" %s: %s -> ka\n" % (f, msg))
+        # no progress
 
     # directory rename, move local
     for f, args, msg in mresult.getactions(
diff --git a/mercurial/mergestate.py b/mercurial/mergestate.py
--- a/mercurial/mergestate.py
+++ b/mercurial/mergestate.py
@@ -120,6 +120,10 @@ ACTION_MERGE = b'm'
 ACTION_LOCAL_DIR_RENAME_GET = b'dg'
 ACTION_DIR_RENAME_MOVE_LOCAL = b'dm'
 ACTION_KEEP = b'k'
+# the file was absent on local side before merge and we should
+# keep it absent (absent means file not present, it can be a result
+# of file deletion, rename etc.)
+ACTION_KEEP_ABSENT = b'ka'
 ACTION_EXEC = b'e'
 ACTION_CREATED_MERGE = b'cm'
 
@@ -837,6 +841,10 @@ def recordupdates(repo, actions, branchm
     for f, args, msg in actions.get(ACTION_KEEP, []):
         pass
 
+    # keep deleted
+    for f, args, msg in actions.get(ACTION_KEEP_ABSENT, []):
+        pass
+
     # get
     for f, args, msg in actions.get(ACTION_GET, []):
         if branchmerge:
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 4 of 8] merge: store ACTION_KEEP_ABSENT when we are keeping the file absent locally

Pulkit Goyal
In reply to this post by Pulkit Goyal
# HG changeset patch
# User Pulkit Goyal <[hidden email]>
# Date 1598262609 -19800
#      Mon Aug 24 15:20:09 2020 +0530
# Node ID 8c918c4872a151ba17e744c6a7a4610fef8f16b3
# Parent  2c58747e0a1ebe17b6bf1f4f393da2be57bce63a
# EXP-Topic merge-newnode
merge: store ACTION_KEEP_ABSENT when we are keeping the file absent locally

If a file is not present on the local side, and it's unchanged between other
merge parent and ancestor, we don't use any action, neither we had a if-else
branch for that condition. This leads to bid-merge missing that there is a
such action possible which can be performed.

As test changes demonstrate, we now choose the locally deleted side instead
of choosing the remote one consistently. This is also wrong behavior which is
resulted because of missing possible action. It will be fixed in next patch.

This whole logic is not acurrate as we should prompt user on what to do
when this kind of criss-cross merge is in play.

Differential Revision: https://phab.mercurial-scm.org/D8940

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -1023,6 +1023,13 @@ def manifestmerge(
                         (None, f, f, False, pa.node()),
                         b'prompt deleted/changed',
                     )
+            else:
+                mresult.addfile(
+                    f,
+                    mergestatemod.ACTION_KEEP_ABSENT,
+                    None,
+                    b'local not present, remote unchanged',
+                )
 
     if repo.ui.configbool(b'experimental', b'merge.checkpathconflicts'):
         # If we are merging, look for path conflicts.
diff --git a/tests/test-merge-criss-cross.t b/tests/test-merge-criss-cross.t
--- a/tests/test-merge-criss-cross.t
+++ b/tests/test-merge-criss-cross.t
@@ -572,28 +572,26 @@ BROKEN: this should result in conflict
   $ hg update --clean 'desc("merge-deleting-the-file-from-deleted")'
   0 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ hg merge          'desc("merge-keeping-the-file-from-deleted")'
-  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
   (branch merge, don't forget to commit)
   $ ls -1
   other-file
-  the-file
 
 (merging a deletion with keeping → conflict)
 BROKEN: this should result in conflict
 
   $ hg update --clean 'desc("merge-deleting-the-file-from-deleted")'
-  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ hg merge          'desc("merge-keeping-the-file-from-updated")'
-  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
   (branch merge, don't forget to commit)
   $ ls -1
   other-file
-  the-file
 
 (merging two deletion together → no conflict)
 
   $ hg update --clean 'desc("merge-deleting-the-file-from-updated")'
-  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ hg merge          'desc("merge-deleting-the-file-from-deleted")'
   0 files updated, 0 files merged, 0 files removed, 0 files unresolved
   (branch merge, don't forget to commit)
@@ -606,23 +604,21 @@ BROKEN: this should result in conflict
   $ hg update --clean 'desc("merge-deleting-the-file-from-updated")'
   0 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ hg merge          'desc("merge-keeping-the-file-from-deleted")'
-  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
   (branch merge, don't forget to commit)
   $ ls -1
   other-file
-  the-file
 
 (merging a deletion with keeping → conflict)
 BROKEN: this should result in conflict
 
   $ hg update --clean 'desc("merge-deleting-the-file-from-updated")'
-  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ hg merge          'desc("merge-keeping-the-file-from-updated")'
-  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
   (branch merge, don't forget to commit)
   $ ls -1
   other-file
-  the-file
 
 (merging two "keeping" together → no conflict)
 
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 5 of 8] merge: add missing ACTION_KEEP when both remote and ancestor are not present

Pulkit Goyal
In reply to this post by Pulkit Goyal
# HG changeset patch
# User Pulkit Goyal <[hidden email]>
# Date 1598269948 -19800
#      Mon Aug 24 17:22:28 2020 +0530
# Node ID 9be2d61f80be6059bcdc890bfb889d0a9670dbda
# Parent  8c918c4872a151ba17e744c6a7a4610fef8f16b3
# EXP-Topic merge-newnode
merge: add missing ACTION_KEEP when both remote and ancestor are not present

Previous patch may lead to confusion that the related criss-cross merge is
consistent when done from any of the parents. However this is not true and we
were missing setting an ACTION_KEEP.

This patch now exposes that bid-merge favors ACTION_KEEP always and the result
of merge is different when started from different parents.

This change also effects a test case above where bid merge was wrongly picking
`r` because it was missing keep related information from one of the ancestor.

After this test, we are back in a state in the criss-cross merge tests where the
result depends on which parent we are merging from.

Differential Revision: https://phab.mercurial-scm.org/D8941

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -932,6 +932,14 @@ def manifestmerge(
                     mresult.addfile(
                         f, mergestatemod.ACTION_REMOVE, None, b'other deleted',
                     )
+            else:  # file not in ancestor, not in remote
+                mresult.addfile(
+                    f,
+                    mergestatemod.ACTION_KEEP,
+                    None,
+                    b'ancestor missing, remote missing',
+                )
+
         elif n2:  # file exists only on remote side
             if f in copied1:
                 pass  # we'll deal with it on m1 side
diff --git a/tests/test-merge-criss-cross.t b/tests/test-merge-criss-cross.t
--- a/tests/test-merge-criss-cross.t
+++ b/tests/test-merge-criss-cross.t
@@ -431,6 +431,8 @@ Verify that the old context ancestor wor
   resolving manifests
    branchmerge: True, force: False, partial: False
    ancestor: 11b5b303e36c, local: c0ef19750a22+, remote: 6ca01f7342b9
+   d1/a: ancestor missing, remote missing -> k
+   d1/b: ancestor missing, remote missing -> k
    d2/b: remote created -> g
   
   calculating bids for ancestor 154e6000f54e
@@ -450,24 +452,24 @@ Verify that the old context ancestor wor
   
   auction for merging merge bids (2 ancestors)
    list of bids for d1/a:
+     ancestor missing, remote missing -> k
      other deleted -> r
-   d1/a: consensus for r
+   d1/a: picking 'keep' action
    list of bids for d1/b:
+     ancestor missing, remote missing -> k
      other deleted -> r
-   d1/b: consensus for r
+   d1/b: picking 'keep' action
    list of bids for d2/b:
      remote created -> g
      remote created -> g
    d2/b: consensus for g
   end of auction
   
-   d1/a: other deleted -> r
-  removing d1/a
-   d1/b: other deleted -> r
-  removing d1/b
    d2/b: remote created -> g
   getting d2/b
-  1 files updated, 0 files merged, 2 files removed, 0 files unresolved
+   d1/a: ancestor missing, remote missing -> k
+   d1/b: ancestor missing, remote missing -> k
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   (branch merge, don't forget to commit)
 
 
@@ -649,15 +651,16 @@ BROKEN: this should result in conflict
   $ hg update --clean 'desc("merge-keeping-the-file-from-updated")'
   0 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ hg merge          'desc("merge-deleting-the-file-from-updated")'
-  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
   (branch merge, don't forget to commit)
   $ ls -1
   other-file
+  the-file
 
 (merging two "keeping" together → no conflict)
 
   $ hg update --clean 'desc("merge-keeping-the-file-from-deleted")'
-  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ hg merge          'desc("merge-keeping-the-file-from-updated")'
   0 files updated, 0 files merged, 0 files removed, 0 files unresolved
   (branch merge, don't forget to commit)
@@ -683,7 +686,8 @@ BROKEN: this should result in conflict
   $ hg update --clean 'desc("merge-keeping-the-file-from-deleted")'
   0 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ hg merge          'desc("merge-deleting-the-file-from-updated")'
-  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
   (branch merge, don't forget to commit)
   $ ls -1
   other-file
+  the-file
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 6 of 8] merge: update commitinfo from all mergeresults during bid merge

Pulkit Goyal
In reply to this post by Pulkit Goyal
# HG changeset patch
# User Pulkit Goyal <[hidden email]>
# Date 1598440043 -19800
#      Wed Aug 26 16:37:23 2020 +0530
# Node ID 21f520670eb08917eeb5fcf11c75cc1dfb242530
# Parent  9be2d61f80be6059bcdc890bfb889d0a9670dbda
# EXP-Topic merge-newnode
merge: update commitinfo from all mergeresults during bid merge

During bid merge, it's not clear which commitinfo should be stored and which one
should not. This depends on which side the bid merge chooses for a file. For
this we will need to refactor bid merge code and commitinfo handling.

For now, we just blindly updates info since we hardly have any users of
commitinfo and this will help us in testing and clearing out further path.

Differential Revision: https://phab.mercurial-scm.org/D8965

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -1130,6 +1130,7 @@ def calculateupdates(
         # mapping of following form:
         # {ACTION_X : [info, ..], ACTION_Y : [info, ..]}
         fbids = {}
+        mresult = mergeresult()
         diverge, renamedelete = None, None
         for ancestor in ancestors:
             repo.ui.note(_(b'\ncalculating bids for ancestor %s\n') % ancestor)
@@ -1156,6 +1157,12 @@ def calculateupdates(
             ):
                 renamedelete = mresult1.renamedelete
 
+            # blindly update final mergeresult commitinfo with what we get
+            # from mergeresult object for each ancestor
+            # TODO: some commitinfo depends on what bid merge choose and hence
+            # we will need to make commitinfo also depend on bid merge logic
+            mresult._commitinfo.update(mresult1._commitinfo)
+
             for f, a in mresult1.filemap(sort=True):
                 m, args, msg = a
                 repo.ui.debug(b' %s: %s -> %s\n' % (f, msg, m))
@@ -1174,7 +1181,6 @@ def calculateupdates(
             _(b'\nauction for merging merge bids (%d ancestors)\n')
             % len(ancestors)
         )
-        mresult = mergeresult()
         for f, bids in sorted(fbids.items()):
             repo.ui.debug(_(b" list of bids for %s:\n") % f)
             for m, l in sorted(bids.items()):
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 7 of 8] merge: check for dir rename dest before adding ACTION_KEEP

Pulkit Goyal
In reply to this post by Pulkit Goyal
# HG changeset patch
# User Pulkit Goyal <[hidden email]>
# Date 1599034669 -19800
#      Wed Sep 02 13:47:49 2020 +0530
# Node ID a6378fd3a90effdd150f94f4da8266687276e0e4
# Parent  21f520670eb08917eeb5fcf11c75cc1dfb242530
# EXP-Topic merge-newnode
merge: check for dir rename dest before adding ACTION_KEEP

A previous patch in the series blindly uses `ACTION_KEEP` if the file is not
present on both remote and ancestor. This was wrong.

We tries to detect directory renames and in some graft cases, it can be possible
that file is not present on both sides but is created in rename destination of
other directory which exists on remote. In such cases, we need to merge the
rename source from remote with rename dest from local.

This patch makes sure we checks for such rename cases before falling back to
`ACTION_KEEP`.

Also, we moved the adding of action for the destination file while processing
the destination file which makes things much simpler as an action for file will
only be added while processing it. And we added a `ACTION_KEEP_ABSENT` for the
rename source because otherwise an action for that was missing.

Differential Revision: https://phab.mercurial-scm.org/D8977

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -933,12 +933,41 @@ def manifestmerge(
                         f, mergestatemod.ACTION_REMOVE, None, b'other deleted',
                     )
             else:  # file not in ancestor, not in remote
-                mresult.addfile(
-                    f,
-                    mergestatemod.ACTION_KEEP,
-                    None,
-                    b'ancestor missing, remote missing',
-                )
+                rename_found = False
+                for source, dest in branch_copies1.dirmove.items():
+                    if f.startswith(dest):
+                        # this directory in which this file is created
+                        # is moved from somewhere else
+                        # we should check if the source file exists on
+                        # remote side
+                        sf = source + f[len(dest) :]
+                        if sf in m2:
+                            rename_found = True
+                            if m2[sf] == ma[sf]:
+                                mresult.addfile(
+                                    f,
+                                    mergestatemod.ACTION_KEEP,
+                                    None,
+                                    b'file is rename dest and rename source'
+                                    b' did not change',
+                                )
+                            else:
+                                mresult.addfile(
+                                    f,
+                                    mergestatemod.ACTION_MERGE,
+                                    (f, sf, sf, False, pa.node()),
+                                    b'local directory rename - respect move '
+                                    b'from %s' % sf,
+                                )
+                            break
+
+                if not rename_found:
+                    mresult.addfile(
+                        f,
+                        mergestatemod.ACTION_KEEP,
+                        None,
+                        b'ancestor missing, remote missing',
+                    )
 
         elif n2:  # file exists only on remote side
             if f in copied1:
@@ -1010,12 +1039,16 @@ def manifestmerge(
                         df = branch_copies1.dirmove[d] + f[len(d) :]
                         break
                 if df is not None and df in m1:
+                    # this file is not present in local however its a source
+                    # of rename for another file. keep this file not
+                    # present in local. Action for rename dest is written
+                    # while processing it
                     mresult.addfile(
-                        df,
-                        mergestatemod.ACTION_MERGE,
-                        (df, f, f, False, pa.node()),
-                        b'local directory rename - respect move '
-                        b'from %s' % f,
+                        f,
+                        mergestatemod.ACTION_KEEP_ABSENT,
+                        None,
+                        b'remote directory is rename source - respect move '
+                        b'to %s' % df,
                     )
                 elif acceptremote:
                     mresult.addfile(
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 8 of 8] mergestate: introduce a new ACTION_KEEP_NEW

Pulkit Goyal
In reply to this post by Pulkit Goyal
# HG changeset patch
# User Pulkit Goyal <[hidden email]>
# Date 1599650359 -19800
#      Wed Sep 09 16:49:19 2020 +0530
# Node ID d5e63af55451c06b706484f1ba5ee589bfc8f26c
# Parent  a6378fd3a90effdd150f94f4da8266687276e0e4
# EXP-Topic merge-newnode
mergestate: introduce a new ACTION_KEEP_NEW

`ACTION_KEEP` is overloaded and it's hard to figure out how we end up with this
KEEP, what was the state of things.

In a previous patch, we introduced `ACTION_KEEP_ABSENT` which represents files
which are kept absent in the working directory.

There is another special case where we keep the file when it's not present on
both ancestor and remote side. We introduce a dedicated action for that.

The goal is to use these information to make bid merge smarter.

Differential Revision: https://phab.mercurial-scm.org/D9002

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -566,6 +566,7 @@ class mergeresult(object):
     NO_OP_ACTIONS = (
         mergestatemod.ACTION_KEEP,
         mergestatemod.ACTION_KEEP_ABSENT,
+        mergestatemod.ACTION_KEEP_NEW,
     )
 
     def __init__(self):
@@ -964,7 +965,7 @@ def manifestmerge(
                 if not rename_found:
                     mresult.addfile(
                         f,
-                        mergestatemod.ACTION_KEEP,
+                        mergestatemod.ACTION_KEEP_NEW,
                         None,
                         b'ancestor missing, remote missing',
                     )
@@ -1237,6 +1238,11 @@ def calculateupdates(
                 repo.ui.note(_(b" %s: picking 'keep absent' action\n") % f)
                 mresult.addfile(f, *bids[mergestatemod.ACTION_KEEP_ABSENT][0])
                 continue
+            # If keep new is an option, let's just do that
+            if mergestatemod.ACTION_KEEP_NEW in bids:
+                repo.ui.note(_(b" %s: picking 'keep new' action\n") % f)
+                mresult.addfile(f, *bids[mergestatemod.ACTION_KEEP_NEW][0])
+                continue
             # If there are gets and they all agree [how could they not?], do it.
             if mergestatemod.ACTION_GET in bids:
                 ga0 = bids[mergestatemod.ACTION_GET][0]
@@ -1543,15 +1549,9 @@ def applyupdates(
         progress.increment(item=f)
 
     # keep (noop, just log it)
-    for f, args, msg in mresult.getactions(
-        (mergestatemod.ACTION_KEEP,), sort=True
-    ):
-        repo.ui.debug(b" %s: %s -> k\n" % (f, msg))
-        # no progress
-    for f, args, msg in mresult.getactions(
-        (mergestatemod.ACTION_KEEP_ABSENT,), sort=True
-    ):
-        repo.ui.debug(b" %s: %s -> ka\n" % (f, msg))
+    for a in mergeresult.NO_OP_ACTIONS:
+        for f, args, msg in mresult.getactions((a,), sort=True):
+            repo.ui.debug(b" %s: %s -> %s\n" % (f, msg, a))
         # no progress
 
     # directory rename, move local
diff --git a/mercurial/mergestate.py b/mercurial/mergestate.py
--- a/mercurial/mergestate.py
+++ b/mercurial/mergestate.py
@@ -124,6 +124,9 @@ ACTION_KEEP = b'k'
 # keep it absent (absent means file not present, it can be a result
 # of file deletion, rename etc.)
 ACTION_KEEP_ABSENT = b'ka'
+# the file is absent on the ancestor and remote side of the merge
+# hence this file is new and we should keep it
+ACTION_KEEP_NEW = b'kn'
 ACTION_EXEC = b'e'
 ACTION_CREATED_MERGE = b'cm'
 
@@ -845,6 +848,10 @@ def recordupdates(repo, actions, branchm
     for f, args, msg in actions.get(ACTION_KEEP_ABSENT, []):
         pass
 
+    # keep new
+    for f, args, msg in actions.get(ACTION_KEEP_NEW, []):
+        pass
+
     # get
     for f, args, msg in actions.get(ACTION_GET, []):
         if branchmerge:
diff --git a/tests/test-merge-criss-cross.t b/tests/test-merge-criss-cross.t
--- a/tests/test-merge-criss-cross.t
+++ b/tests/test-merge-criss-cross.t
@@ -431,8 +431,8 @@ Verify that the old context ancestor wor
   resolving manifests
    branchmerge: True, force: False, partial: False
    ancestor: 11b5b303e36c, local: c0ef19750a22+, remote: 6ca01f7342b9
-   d1/a: ancestor missing, remote missing -> k
-   d1/b: ancestor missing, remote missing -> k
+   d1/a: ancestor missing, remote missing -> kn
+   d1/b: ancestor missing, remote missing -> kn
    d2/b: remote created -> g
   
   calculating bids for ancestor 154e6000f54e
@@ -452,13 +452,13 @@ Verify that the old context ancestor wor
   
   auction for merging merge bids (2 ancestors)
    list of bids for d1/a:
-     ancestor missing, remote missing -> k
+     ancestor missing, remote missing -> kn
      other deleted -> r
-   d1/a: picking 'keep' action
+   d1/a: picking 'keep new' action
    list of bids for d1/b:
-     ancestor missing, remote missing -> k
+     ancestor missing, remote missing -> kn
      other deleted -> r
-   d1/b: picking 'keep' action
+   d1/b: picking 'keep new' action
    list of bids for d2/b:
      remote created -> g
      remote created -> g
@@ -467,8 +467,8 @@ Verify that the old context ancestor wor
   
    d2/b: remote created -> g
   getting d2/b
-   d1/a: ancestor missing, remote missing -> k
-   d1/b: ancestor missing, remote missing -> k
+   d1/a: ancestor missing, remote missing -> kn
+   d1/b: ancestor missing, remote missing -> kn
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   (branch merge, don't forget to commit)
 
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1 of 8] tests: add criss cross merging tests whose behavior need to be fixed

Yuya Nishihara
In reply to this post by Pulkit Goyal
On Mon, 14 Sep 2020 16:45:17 +0530, Pulkit Goyal wrote:
> # HG changeset patch
> # User Pierre-Yves David <[hidden email]>
> # Date 1592566066 -7200
> #      Fri Jun 19 13:27:46 2020 +0200
> # Node ID eab095687e7b655011a26001dc83c802ab474ceb
> # Parent  6df7dc3e9c3578587c96f244b685d284d114f732
> # EXP-Topic merge-newnode
> tests: add criss cross merging tests whose behavior need to be fixed

I'm not an expert, but the direction of this series sounds reasonable to me.
So queued, thanks.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2 of 8] mergeresult: introduce dedicated tuple for no-op actions

Yuya Nishihara
In reply to this post by Pulkit Goyal
On Mon, 14 Sep 2020 16:45:18 +0530, Pulkit Goyal wrote:
> # HG changeset patch
> # User Pulkit Goyal <[hidden email]>
> # Date 1600071699 -19800
> #      Mon Sep 14 13:51:39 2020 +0530
> # Node ID cfabb1ccdf7a2cb67bb2819f798bad53c6aac977
> # Parent  eab095687e7b655011a26001dc83c802ab474ceb
> # EXP-Topic merge-newnode
> mergeresult: introduce dedicated tuple for no-op actions

> @@ -564,6 +563,8 @@ class mergeresult(object):
>      It has information about what actions need to be performed on dirstate
>      mapping of divergent renames and other such cases. '''
>  
> +    NO_OP_ACTIONS = (mergestatemod.ACTION_KEEP,)

I prefer defining this kind of constants in module scope (i.e.
mergestatemod.NO_OP_ACTIONS) to make sure it isn't intended to be
overridden by subclasses.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 7 of 8] merge: check for dir rename dest before adding ACTION_KEEP

Yuya Nishihara
In reply to this post by Pulkit Goyal
On Mon, 14 Sep 2020 16:45:23 +0530, Pulkit Goyal wrote:
> # HG changeset patch
> # User Pulkit Goyal <[hidden email]>
> # Date 1599034669 -19800
> #      Wed Sep 02 13:47:49 2020 +0530
> # Node ID a6378fd3a90effdd150f94f4da8266687276e0e4
> # Parent  21f520670eb08917eeb5fcf11c75cc1dfb242530
> # EXP-Topic merge-newnode
> merge: check for dir rename dest before adding ACTION_KEEP

Apparently this breaks test-graft-rename.t. I've queued first 6 patches.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2 of 8] mergeresult: introduce dedicated tuple for no-op actions

Pierre-Yves David-2
In reply to this post by Yuya Nishihara
+1

On 9/16/20 12:49 PM, Yuya Nishihara wrote:

> On Mon, 14 Sep 2020 16:45:18 +0530, Pulkit Goyal wrote:
>> # HG changeset patch
>> # User Pulkit Goyal <[hidden email]>
>> # Date 1600071699 -19800
>> #      Mon Sep 14 13:51:39 2020 +0530
>> # Node ID cfabb1ccdf7a2cb67bb2819f798bad53c6aac977
>> # Parent  eab095687e7b655011a26001dc83c802ab474ceb
>> # EXP-Topic merge-newnode
>> mergeresult: introduce dedicated tuple for no-op actions
>
>> @@ -564,6 +563,8 @@ class mergeresult(object):
>>       It has information about what actions need to be performed on dirstate
>>       mapping of divergent renames and other such cases. '''
>>  
>> +    NO_OP_ACTIONS = (mergestatemod.ACTION_KEEP,)
>
> I prefer defining this kind of constants in module scope (i.e.
> mergestatemod.NO_OP_ACTIONS) to make sure it isn't intended to be
> overridden by subclasses.
> _______________________________________________
> Mercurial-devel mailing list
> [hidden email]
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>

--
Pierre-Yves David
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel