Patchwork [2,of,2,stable] merge: fix crash on criss cross merge with dir move and delete (issue5020)

login
register
mail settings
Submitter Mads Kiilerich
Date Jan. 31, 2017, 2:26 a.m.
Message ID <9aa0ed4d2693608c2511.1485829612@localhost.localdomain>
Download mbox | patch
Permalink /patch/18288/
State Accepted
Headers show

Comments

Mads Kiilerich - Jan. 31, 2017, 2:26 a.m.
# HG changeset patch
# User Mads Kiilerich <mads@kiilerich.com>
# Date 1485829559 -3600
#      Tue Jan 31 03:25:59 2017 +0100
# Branch stable
# Node ID 9aa0ed4d2693608c25112f0770809f5be1c8be1c
# Parent  f1cb9a55f1ff7251d85ff60ff5a190c183f1a012
merge: fix crash on criss cross merge with dir move and delete (issue5020)

Work around that 'dm' in the data model only can have one operation for the
target file, but still can have multiple and conflicting operations on the
source file where the other operation is a 'rm'. The move would thus fail with
'abort: No such file or directory'.

In this case it is "obvious" that the file should be removed, either before or
after moving it. We thus keep the 'rm' of the source file but drop the 'dm'.

This is not a pretty fix but quite "obviously" safe (famous last words...) as
it only touches a rare code path that used to crash. It is possible that it
would be better to swap the files for 'dm' as suggested on
https://bz.mercurial-scm.org/show_bug.cgi?id=5020#c13 but it is not entirely
obvious that it not just would create conflicts on the other file. That can be
revisited later.
Yuya Nishihara - Jan. 31, 2017, 2:49 p.m.
On Tue, 31 Jan 2017 03:26:52 +0100, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <mads@kiilerich.com>
> # Date 1485829559 -3600
> #      Tue Jan 31 03:25:59 2017 +0100
> # Branch stable
> # Node ID 9aa0ed4d2693608c25112f0770809f5be1c8be1c
> # Parent  f1cb9a55f1ff7251d85ff60ff5a190c183f1a012
> merge: fix crash on criss cross merge with dir move and delete (issue5020)
> 
> Work around that 'dm' in the data model only can have one operation for the
> target file, but still can have multiple and conflicting operations on the
> source file where the other operation is a 'rm'. The move would thus fail with
> 'abort: No such file or directory'.
> 
> In this case it is "obvious" that the file should be removed, either before or
> after moving it. We thus keep the 'rm' of the source file but drop the 'dm'.

This 'dm'-vs-'r' story makes sense.

> @@ -1029,7 +1032,19 @@ def calculateupdates(repo, wctx, mctx, a
>              repo.ui.warn(_(' %s: ambiguous merge - picked %s action\n') %
>                           (f, m))
>              actions[f] = l[0]
> +            if m == 'dm':
> +                dms.append(f)
>              continue
> +        # Work around 'dm' that can cause multiple actions for the same file
> +        for f in dms:
> +            dm, (f0, flags), msg = actions[f]
> +            assert dm == 'dm', dm
> +            m, args, msg = actions[f0]

I was a bit worried whether actions[f0] always exist, but that seemed
unnecessary concern.

Queued the series, thanks.
Mads Kiilerich - Feb. 1, 2017, 1:15 a.m.
On 01/31/2017 03:49 PM, Yuya Nishihara wrote:
>
>> @@ -1029,7 +1032,19 @@ def calculateupdates(repo, wctx, mctx, a
>>               repo.ui.warn(_(' %s: ambiguous merge - picked %s action\n') %
>>                            (f, m))
>>               actions[f] = l[0]
>> +            if m == 'dm':
>> +                dms.append(f)
>>               continue
>> +        # Work around 'dm' that can cause multiple actions for the same file
>> +        for f in dms:
>> +            dm, (f0, flags), msg = actions[f]
>> +            assert dm == 'dm', dm
>> +            m, args, msg = actions[f0]
> I was a bit worried whether actions[f0] always exist, but that seemed
> unnecessary concern.

I agree it doesn't seem to be a valid concern but I can't give a 
convincing proof. I thus sent  patch to make it more safe, just incase 
the concern should be valid.

/Mads

Patch

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -997,6 +997,7 @@  def calculateupdates(repo, wctx, mctx, a
         # Pick the best bid for each file
         repo.ui.note(_('\nauction for merging merge bids\n'))
         actions = {}
+        dms = [] # filenames that have dm actions
         for f, bids in sorted(fbids.items()):
             # bids is a mapping from action method to list af actions
             # Consensus?
@@ -1005,6 +1006,8 @@  def calculateupdates(repo, wctx, mctx, a
                 if all(a == l[0] for a in l[1:]): # len(bids) is > 1
                     repo.ui.note(_(" %s: consensus for %s\n") % (f, m))
                     actions[f] = l[0]
+                    if m == 'dm':
+                        dms.append(f)
                     continue
             # If keep is an option, just do it.
             if 'k' in bids:
@@ -1029,7 +1032,19 @@  def calculateupdates(repo, wctx, mctx, a
             repo.ui.warn(_(' %s: ambiguous merge - picked %s action\n') %
                          (f, m))
             actions[f] = l[0]
+            if m == 'dm':
+                dms.append(f)
             continue
+        # Work around 'dm' that can cause multiple actions for the same file
+        for f in dms:
+            dm, (f0, flags), msg = actions[f]
+            assert dm == 'dm', dm
+            m, args, msg = actions[f0]
+            if m == 'r':
+                # We have one bid for removing a file and another for moving it.
+                # These two could be merged as first move and then delete ...
+                # but instead drop moving and just delete.
+                del actions[f]
         repo.ui.note(_('end of auction\n\n'))
 
     _resolvetrivial(repo, wctx, mctx, ancestors[0], actions)
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
@@ -11,9 +11,15 @@  Criss cross merging
 
   $ hg up -qr0
   $ echo '2 first change' > f2
+  $ mkdir d1
+  $ echo '0 base' > d1/f3
+  $ echo '0 base' > d1/f4
+  $ hg add -q d1
   $ hg ci -qm '2 first change f2'
 
   $ hg merge -qr 1
+  $ hg rm d1/f3
+  $ hg mv -q d1 d2
   $ hg ci -m '3 merge'
 
   $ hg up -qr2
@@ -24,38 +30,38 @@  Criss cross merging
   $ hg ci -m '5 second change f1'
 
   $ hg up -r3
-  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  2 files updated, 0 files merged, 2 files removed, 0 files unresolved
   $ echo '6 second change' > f2
   $ hg ci -m '6 second change f2'
 
   $ hg log -G
-  @  changeset:   6:3b08d01b0ab5
+  @  changeset:   6:6373bbfdae1d
   |  tag:         tip
-  |  parent:      3:cf89f02107e5
+  |  parent:      3:c202c8af058d
   |  user:        test
   |  date:        Thu Jan 01 00:00:00 1970 +0000
   |  summary:     6 second change f2
   |
-  | o  changeset:   5:adfe50279922
+  | o  changeset:   5:e673248094b1
   | |  user:        test
   | |  date:        Thu Jan 01 00:00:00 1970 +0000
   | |  summary:     5 second change f1
   | |
-  | o    changeset:   4:7d3e55501ae6
-  | |\   parent:      2:40663881a6dd
+  | o    changeset:   4:177f58377c06
+  | |\   parent:      2:d1d156401c1b
   | | |  parent:      1:0f6b37dbe527
   | | |  user:        test
   | | |  date:        Thu Jan 01 00:00:00 1970 +0000
   | | |  summary:     4 merge
   | | |
-  o---+  changeset:   3:cf89f02107e5
-  | | |  parent:      2:40663881a6dd
+  o---+  changeset:   3:c202c8af058d
+  | | |  parent:      2:d1d156401c1b
   |/ /   parent:      1:0f6b37dbe527
   | |    user:        test
   | |    date:        Thu Jan 01 00:00:00 1970 +0000
   | |    summary:     3 merge
   | |
-  | o  changeset:   2:40663881a6dd
+  | o  changeset:   2:d1d156401c1b
   | |  parent:      0:40494bf2444c
   | |  user:        test
   | |  date:        Thu Jan 01 00:00:00 1970 +0000
@@ -73,27 +79,51 @@  Criss cross merging
   
 
   $ hg merge -v --debug --tool internal:dump 5 --config merge.preferancestor='!'
-  note: using 0f6b37dbe527 as ancestor of 3b08d01b0ab5 and adfe50279922
-        alternatively, use --config merge.preferancestor=40663881a6dd
+  note: using 0f6b37dbe527 as ancestor of 6373bbfdae1d and e673248094b1
+        alternatively, use --config merge.preferancestor=d1d156401c1b
     searching for copies back to rev 3
+    unmatched files in local:
+     d2/f4
+    unmatched files in other:
+     d1/f3
+     d1/f4
+    all copies found (* = to merge, ! = divergent, % = renamed and deleted):
+     src: 'd1/f4' -> dst: 'd2/f4' 
+    checking for directory renames
+     discovered dir src: 'd1/' -> dst: 'd2/'
+     pending file src: 'd1/f3' -> dst: 'd2/f3'
+     pending file src: 'd1/f4' -> dst: 'd2/f4'
   resolving manifests
    branchmerge: True, force: False, partial: False
-   ancestor: 0f6b37dbe527, local: 3b08d01b0ab5+, remote: adfe50279922
+   ancestor: 0f6b37dbe527, local: 6373bbfdae1d+, remote: e673248094b1
+   preserving d2/f4 for resolve of d2/f4
    preserving f2 for resolve of f2
    f1: remote is newer -> g
   getting f1
+   d2/f3: local directory rename - get from d1/f3 -> dg
+  getting d1/f3 to d2/f3
+   d2/f4: local directory rename, both created -> m (premerge)
    f2: versions differ -> m (premerge)
   picked tool ':dump' for f2 (binary False symlink False changedelete False)
   merging f2
-  my f2@3b08d01b0ab5+ other f2@adfe50279922 ancestor f2@0f6b37dbe527
+  my f2@6373bbfdae1d+ other f2@e673248094b1 ancestor f2@0f6b37dbe527
    f2: versions differ -> m (merge)
   picked tool ':dump' for f2 (binary False symlink False changedelete False)
-  my f2@3b08d01b0ab5+ other f2@adfe50279922 ancestor f2@0f6b37dbe527
-  1 files updated, 0 files merged, 0 files removed, 1 files unresolved
+  my f2@6373bbfdae1d+ other f2@e673248094b1 ancestor f2@0f6b37dbe527
+  3 files updated, 0 files merged, 0 files removed, 1 files unresolved
   use 'hg resolve' to retry unresolved file merges or 'hg update -C .' to abandon
   [1]
 
-  $ f --dump *
+  $ f --dump --recurse *
+  d2: directory with 2 files
+  d2/f3:
+  >>>
+  0 base
+  <<<
+  d2/f4:
+  >>>
+  0 base
+  <<<
   f1:
   >>>
   5 second change
@@ -121,11 +151,13 @@  Criss cross merging
 
   $ hg up -qC .
   $ hg merge -v --tool internal:dump 5 --config merge.preferancestor="null 40663881 3b08d"
-  note: using 40663881a6dd as ancestor of 3b08d01b0ab5 and adfe50279922
-        alternatively, use --config merge.preferancestor=0f6b37dbe527
+  note: using 0f6b37dbe527 as ancestor of 6373bbfdae1d and e673248094b1
+        alternatively, use --config merge.preferancestor=d1d156401c1b
   resolving manifests
-  merging f1
-  0 files updated, 0 files merged, 0 files removed, 1 files unresolved
+  getting f1
+  getting d1/f3 to d2/f3
+  merging f2
+  3 files updated, 0 files merged, 0 files removed, 1 files unresolved
   use 'hg resolve' to retry unresolved file merges or 'hg update -C .' to abandon
   [1]
 
@@ -134,36 +166,70 @@  Redo merge with merge.preferancestor="*"
   $ rm f*
   $ hg up -qC .
   $ hg merge -v --debug --tool internal:dump 5 --config merge.preferancestor="*"
-  note: merging 3b08d01b0ab5+ and adfe50279922 using bids from ancestors 0f6b37dbe527 and 40663881a6dd
+  note: merging 6373bbfdae1d+ and e673248094b1 using bids from ancestors 0f6b37dbe527 and d1d156401c1b
   
   calculating bids for ancestor 0f6b37dbe527
     searching for copies back to rev 3
+    unmatched files in local:
+     d2/f4
+    unmatched files in other:
+     d1/f3
+     d1/f4
+    all copies found (* = to merge, ! = divergent, % = renamed and deleted):
+     src: 'd1/f4' -> dst: 'd2/f4' 
+    checking for directory renames
+     discovered dir src: 'd1/' -> dst: 'd2/'
+     pending file src: 'd1/f3' -> dst: 'd2/f3'
+     pending file src: 'd1/f4' -> dst: 'd2/f4'
   resolving manifests
    branchmerge: True, force: False, partial: False
-   ancestor: 0f6b37dbe527, local: 3b08d01b0ab5+, remote: adfe50279922
+   ancestor: 0f6b37dbe527, local: 6373bbfdae1d+, remote: e673248094b1
+   d2/f3: local directory rename - get from d1/f3 -> dg
+   d2/f4: local directory rename, both created -> m
    f1: remote is newer -> g
    f2: versions differ -> m
   
-  calculating bids for ancestor 40663881a6dd
+  calculating bids for ancestor d1d156401c1b
     searching for copies back to rev 3
+    unmatched files in local:
+     d2/f4
+    all copies found (* = to merge, ! = divergent, % = renamed and deleted):
+     src: 'd1/f4' -> dst: 'd2/f4' 
+    checking for directory renames
+     discovered dir src: 'd1/' -> dst: 'd2/'
   resolving manifests
    branchmerge: True, force: False, partial: False
-   ancestor: 40663881a6dd, local: 3b08d01b0ab5+, remote: adfe50279922
+   ancestor: d1d156401c1b, local: 6373bbfdae1d+, remote: e673248094b1
    f1: versions differ -> m
    f2: remote unchanged -> k
   
   auction for merging merge bids
+   d2/f3: consensus for dg
+   d2/f4: consensus for m
    f1: picking 'get' action
    f2: picking 'keep' action
   end of auction
   
+   preserving d2/f4 for resolve of d2/f4
    f1: remote is newer -> g
   getting f1
    f2: remote unchanged -> k
-  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+   d2/f3: local directory rename - get from d1/f3 -> dg
+  getting d1/f3 to d2/f3
+   d2/f4: local directory rename, both created -> m (premerge)
+  3 files updated, 0 files merged, 0 files removed, 0 files unresolved
   (branch merge, don't forget to commit)
 
-  $ f --dump *
+  $ f --dump --recurse *
+  d2: directory with 2 files
+  d2/f3:
+  >>>
+  0 base
+  <<<
+  d2/f4:
+  >>>
+  0 base
+  <<<
   f1:
   >>>
   5 second change
@@ -177,38 +243,79 @@  Redo merge with merge.preferancestor="*"
 The other way around:
 
   $ hg up -C -r5
-  2 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  4 files updated, 0 files merged, 1 files removed, 0 files unresolved
   $ hg merge -v --debug --config merge.preferancestor="*"
-  note: merging adfe50279922+ and 3b08d01b0ab5 using bids from ancestors 0f6b37dbe527 and 40663881a6dd
+  note: merging e673248094b1+ and 6373bbfdae1d using bids from ancestors 0f6b37dbe527 and d1d156401c1b
   
   calculating bids for ancestor 0f6b37dbe527
     searching for copies back to rev 3
+    unmatched files in local:
+     d1/f3
+     d1/f4
+    unmatched files in other:
+     d2/f4
+    all copies found (* = to merge, ! = divergent, % = renamed and deleted):
+     src: 'd1/f4' -> dst: 'd2/f4' 
+    checking for directory renames
+     discovered dir src: 'd1/' -> dst: 'd2/'
+     pending file src: 'd1/f3' -> dst: 'd2/f3'
+     pending file src: 'd1/f4' -> dst: 'd2/f4'
   resolving manifests
    branchmerge: True, force: False, partial: False
-   ancestor: 0f6b37dbe527, local: adfe50279922+, remote: 3b08d01b0ab5
+   ancestor: 0f6b37dbe527, local: e673248094b1+, remote: 6373bbfdae1d
+   d2/f3: remote directory rename - move from d1/f3 -> dm
+   d2/f4: remote directory rename, both created -> m
    f1: remote unchanged -> k
    f2: versions differ -> m
   
-  calculating bids for ancestor 40663881a6dd
+  calculating bids for ancestor d1d156401c1b
     searching for copies back to rev 3
+    unmatched files in other:
+     d2/f4
+    all copies found (* = to merge, ! = divergent, % = renamed and deleted):
+     src: 'd1/f4' -> dst: 'd2/f4' 
+    checking for directory renames
+     discovered dir src: 'd1/' -> dst: 'd2/'
   resolving manifests
    branchmerge: True, force: False, partial: False
-   ancestor: 40663881a6dd, local: adfe50279922+, remote: 3b08d01b0ab5
+   ancestor: d1d156401c1b, local: e673248094b1+, remote: 6373bbfdae1d
+   d1/f3: other deleted -> r
+   d1/f4: other deleted -> r
+   d2/f4: remote created -> g
    f1: versions differ -> m
    f2: remote is newer -> g
   
   auction for merging merge bids
+   d1/f3: consensus for r
+   d1/f4: consensus for r
+   d2/f3: consensus for dm
+   d2/f4: picking 'get' action
    f1: picking 'keep' action
    f2: picking 'get' action
   end of auction
   
+   d1/f3: other deleted -> r
+  removing d1/f3
+   d1/f4: other deleted -> r
+  removing d1/f4
+   d2/f4: remote created -> g
+  getting d2/f4
    f2: remote is newer -> g
   getting f2
    f1: remote unchanged -> k
-  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  2 files updated, 0 files merged, 2 files removed, 0 files unresolved
   (branch merge, don't forget to commit)
 
-  $ f --dump *
+  $ f --dump --recurse *
+  d2: directory with 2 files
+  d2/f3:
+  >>>
+  0 base
+  <<<
+  d2/f4:
+  >>>
+  0 base
+  <<<
   f1:
   >>>
   5 second change
@@ -222,57 +329,85 @@  Verify how the output looks and and how 
 
   $ hg up -qC
   $ hg merge
-  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  2 files updated, 0 files merged, 2 files removed, 0 files unresolved
   (branch merge, don't forget to commit)
 
   $ hg up -qC tip
   $ hg merge -v
-  note: merging 3b08d01b0ab5+ and adfe50279922 using bids from ancestors 0f6b37dbe527 and 40663881a6dd
+  note: merging 6373bbfdae1d+ and e673248094b1 using bids from ancestors 0f6b37dbe527 and d1d156401c1b
   
   calculating bids for ancestor 0f6b37dbe527
   resolving manifests
   
-  calculating bids for ancestor 40663881a6dd
+  calculating bids for ancestor d1d156401c1b
   resolving manifests
   
   auction for merging merge bids
+   d2/f3: consensus for dg
+   d2/f4: consensus for m
    f1: picking 'get' action
    f2: picking 'keep' action
   end of auction
   
   getting f1
-  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  getting d1/f3 to d2/f3
+  3 files updated, 0 files merged, 0 files removed, 0 files unresolved
   (branch merge, don't forget to commit)
 
   $ hg up -qC
   $ hg merge -v --debug --config merge.preferancestor="*"
-  note: merging 3b08d01b0ab5+ and adfe50279922 using bids from ancestors 0f6b37dbe527 and 40663881a6dd
+  note: merging 6373bbfdae1d+ and e673248094b1 using bids from ancestors 0f6b37dbe527 and d1d156401c1b
   
   calculating bids for ancestor 0f6b37dbe527
     searching for copies back to rev 3
+    unmatched files in local:
+     d2/f4
+    unmatched files in other:
+     d1/f3
+     d1/f4
+    all copies found (* = to merge, ! = divergent, % = renamed and deleted):
+     src: 'd1/f4' -> dst: 'd2/f4' 
+    checking for directory renames
+     discovered dir src: 'd1/' -> dst: 'd2/'
+     pending file src: 'd1/f3' -> dst: 'd2/f3'
+     pending file src: 'd1/f4' -> dst: 'd2/f4'
   resolving manifests
    branchmerge: True, force: False, partial: False
-   ancestor: 0f6b37dbe527, local: 3b08d01b0ab5+, remote: adfe50279922
+   ancestor: 0f6b37dbe527, local: 6373bbfdae1d+, remote: e673248094b1
+   d2/f3: local directory rename - get from d1/f3 -> dg
+   d2/f4: local directory rename, both created -> m
    f1: remote is newer -> g
    f2: versions differ -> m
   
-  calculating bids for ancestor 40663881a6dd
+  calculating bids for ancestor d1d156401c1b
     searching for copies back to rev 3
+    unmatched files in local:
+     d2/f4
+    all copies found (* = to merge, ! = divergent, % = renamed and deleted):
+     src: 'd1/f4' -> dst: 'd2/f4' 
+    checking for directory renames
+     discovered dir src: 'd1/' -> dst: 'd2/'
   resolving manifests
    branchmerge: True, force: False, partial: False
-   ancestor: 40663881a6dd, local: 3b08d01b0ab5+, remote: adfe50279922
+   ancestor: d1d156401c1b, local: 6373bbfdae1d+, remote: e673248094b1
    f1: versions differ -> m
    f2: remote unchanged -> k
   
   auction for merging merge bids
+   d2/f3: consensus for dg
+   d2/f4: consensus for m
    f1: picking 'get' action
    f2: picking 'keep' action
   end of auction
   
+   preserving d2/f4 for resolve of d2/f4
    f1: remote is newer -> g
   getting f1
    f2: remote unchanged -> k
-  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+   d2/f3: local directory rename - get from d1/f3 -> dg
+  getting d1/f3 to d2/f3
+   d2/f4: local directory rename, both created -> m (premerge)
+  3 files updated, 0 files merged, 0 files removed, 0 files unresolved
   (branch merge, don't forget to commit)
 
   $ cd ..