Patchwork [5,of,5] merge: move initial handling of mergeactions near to later one

login
register
mail settings
Submitter Pulkit Goyal
Date Sept. 11, 2020, 7:05 a.m.
Message ID <c48ee69a8c7202517cad.1599807933@workspace>
Download mbox | patch
Permalink /patch/47135/
State Accepted
Headers show

Comments

Pulkit Goyal - Sept. 11, 2020, 7:05 a.m.
# HG changeset patch
# User Pulkit Goyal <7895pulkit@gmail.com>
# Date 1599119729 -19800
#      Thu Sep 03 13:25:29 2020 +0530
# Node ID c48ee69a8c7202517cade973b3abe3715bfc3c5e
# Parent  8abe98dc3e3c020566d64ac3f3b802de2751d55f
# EXP-Topic merge-newnode
merge: move initial handling of mergeactions near to later one

We build `mergeactions` in the beginning and use it in end. Let's build it just
before where it will be used. Helps making code much easier to understand.

Differential Revision: https://phab.mercurial-scm.org/D8983
Yuya Nishihara - Sept. 12, 2020, 2:59 a.m.
On Fri, 11 Sep 2020 12:35:33 +0530, Pulkit Goyal wrote:
> # HG changeset patch
> # User Pulkit Goyal <7895pulkit@gmail.com>
> # Date 1599119729 -19800
> #      Thu Sep 03 13:25:29 2020 +0530
> # Node ID c48ee69a8c7202517cade973b3abe3715bfc3c5e
> # Parent  8abe98dc3e3c020566d64ac3f3b802de2751d55f
> # EXP-Topic merge-newnode
> merge: move initial handling of mergeactions near to later one
> 
> We build `mergeactions` in the beginning and use it in end. Let's build it just
> before where it will be used. Helps making code much easier to understand.

> -        ms.add(fcl, fco, fca, f)
> -        if f1 != f and move:
> -            moves.append(f1)
> -
> -    # remove renamed files after safely stored
> -    for f in moves:
> -        if wctx[f].lexists():
> -            repo.ui.debug(b"removing %s\n" % f)
> -            wctx[f].audit()
> -            wctx[f].remove()

Needs more eyes. I have no expertise to say this remove() can be moved
after batch gets.
Pulkit Goyal - Sept. 14, 2020, 8:11 a.m.
On Sat, Sep 12, 2020 at 8:31 AM Yuya Nishihara <yuya@tcha.org> wrote:
>
> On Fri, 11 Sep 2020 12:35:33 +0530, Pulkit Goyal wrote:
> > # HG changeset patch
> > # User Pulkit Goyal <7895pulkit@gmail.com>
> > # Date 1599119729 -19800
> > #      Thu Sep 03 13:25:29 2020 +0530
> > # Node ID c48ee69a8c7202517cade973b3abe3715bfc3c5e
> > # Parent  8abe98dc3e3c020566d64ac3f3b802de2751d55f
> > # EXP-Topic merge-newnode
> > merge: move initial handling of mergeactions near to later one
> >
> > We build `mergeactions` in the beginning and use it in end. Let's build it just
> > before where it will be used. Helps making code much easier to understand.
>
> > -        ms.add(fcl, fco, fca, f)
> > -        if f1 != f and move:
> > -            moves.append(f1)
> > -
> > -    # remove renamed files after safely stored
> > -    for f in moves:
> > -        if wctx[f].lexists():
> > -            repo.ui.debug(b"removing %s\n" % f)
> > -            wctx[f].audit()
> > -            wctx[f].remove()
>
> Needs more eyes. I have no expertise to say this remove() can be moved
> after batch gets.

I also was not sure, but all tests passes which made me confident :D

Patch

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -1370,49 +1370,6 @@  def applyupdates(
         # mergestate so that it can be reused on commit
         ms.addcommitinfo(f, op)
 
-    moves = []
-
-    # 'cd' and 'dc' actions are treated like other merge conflicts
-    mergeactions = list(
-        mresult.getactions(
-            [
-                mergestatemod.ACTION_CHANGED_DELETED,
-                mergestatemod.ACTION_DELETED_CHANGED,
-                mergestatemod.ACTION_MERGE,
-            ],
-            sort=True,
-        )
-    )
-    for f, args, msg in mergeactions:
-        f1, f2, fa, move, anc = args
-        if f == b'.hgsubstate':  # merged internally
-            continue
-        if f1 is None:
-            fcl = filemerge.absentfilectx(wctx, fa)
-        else:
-            repo.ui.debug(b" preserving %s for resolve of %s\n" % (f1, f))
-            fcl = wctx[f1]
-        if f2 is None:
-            fco = filemerge.absentfilectx(mctx, fa)
-        else:
-            fco = mctx[f2]
-        actx = repo[anc]
-        if fa in actx:
-            fca = actx[fa]
-        else:
-            # TODO: move to absentfilectx
-            fca = repo.filectx(f1, fileid=nullrev)
-        ms.add(fcl, fco, fca, f)
-        if f1 != f and move:
-            moves.append(f1)
-
-    # remove renamed files after safely stored
-    for f in moves:
-        if wctx[f].lexists():
-            repo.ui.debug(b"removing %s\n" % f)
-            wctx[f].audit()
-            wctx[f].remove()
-
     numupdates = mresult.len() - mresult.len((mergestatemod.ACTION_KEEP,))
     progress = repo.ui.makeprogress(
         _(b'updating'), unit=_(b'files'), total=numupdates
@@ -1555,6 +1512,49 @@  def applyupdates(
         wctx[f].audit()
         wctx[f].setflags(b'l' in flags, b'x' in flags)
 
+    moves = []
+
+    # 'cd' and 'dc' actions are treated like other merge conflicts
+    mergeactions = list(
+        mresult.getactions(
+            [
+                mergestatemod.ACTION_CHANGED_DELETED,
+                mergestatemod.ACTION_DELETED_CHANGED,
+                mergestatemod.ACTION_MERGE,
+            ],
+            sort=True,
+        )
+    )
+    for f, args, msg in mergeactions:
+        f1, f2, fa, move, anc = args
+        if f == b'.hgsubstate':  # merged internally
+            continue
+        if f1 is None:
+            fcl = filemerge.absentfilectx(wctx, fa)
+        else:
+            repo.ui.debug(b" preserving %s for resolve of %s\n" % (f1, f))
+            fcl = wctx[f1]
+        if f2 is None:
+            fco = filemerge.absentfilectx(mctx, fa)
+        else:
+            fco = mctx[f2]
+        actx = repo[anc]
+        if fa in actx:
+            fca = actx[fa]
+        else:
+            # TODO: move to absentfilectx
+            fca = repo.filectx(f1, fileid=nullrev)
+        ms.add(fcl, fco, fca, f)
+        if f1 != f and move:
+            moves.append(f1)
+
+    # remove renamed files after safely stored
+    for f in moves:
+        if wctx[f].lexists():
+            repo.ui.debug(b"removing %s\n" % f)
+            wctx[f].audit()
+            wctx[f].remove()
+
     # these actions updates the file
     updated = mresult.len(
         (
diff --git a/tests/test-graft.t b/tests/test-graft.t
--- a/tests/test-graft.t
+++ b/tests/test-graft.t
@@ -247,9 +247,9 @@  Graft out of order, skipping a merge and
   resolving manifests
    branchmerge: True, force: True, partial: False
    ancestor: 4c60f11aa304, local: 1905859650ec+, remote: 9c233e8e184d
-   preserving e for resolve of e
    d: remote is newer -> g
   getting d
+   preserving e for resolve of e
    e: versions differ -> m (premerge)
   picked tool ':merge' for e (binary False symlink False changedelete False)
   merging e
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
@@ -78,9 +78,9 @@  Criss cross merging
   resolving manifests
    branchmerge: True, force: False, partial: False
    ancestor: 0f6b37dbe527, local: 3b08d01b0ab5+, remote: adfe50279922
-   preserving f2 for resolve of f2
    f1: remote is newer -> g
   getting f1
+   preserving f2 for resolve of f2
    f2: versions differ -> m (premerge)
   picked tool ':dump' for f2 (binary False symlink False changedelete False)
   merging f2
diff --git a/tests/test-rename-merge1.t b/tests/test-rename-merge1.t
--- a/tests/test-rename-merge1.t
+++ b/tests/test-rename-merge1.t
@@ -40,10 +40,10 @@ 
   note: possible conflict - a2 was renamed multiple times to:
    b2
    c2
+   b2: remote created -> g
+  getting b2
    preserving a for resolve of b
   removing a
-   b2: remote created -> g
-  getting b2
    b: remote moved from a -> m (premerge)
   picked tool ':merge' for b (binary False symlink False changedelete False)
   merging a and b to b
diff --git a/tests/test-rename-merge2.t b/tests/test-rename-merge2.t
--- a/tests/test-rename-merge2.t
+++ b/tests/test-rename-merge2.t
@@ -89,6 +89,7 @@  args:
    preserving rev for resolve of rev
   starting 4 threads for background file closing (?)
    b: remote copied from a -> m (premerge)
+  starting 4 threads for background file closing (?)
   picked tool '* ../merge' for b (binary False symlink False changedelete False) (glob)
   merging a and b to b
   my b@e300d1c794ec+ other b@4ce40f5aca24 ancestor a@924404dff337
@@ -124,10 +125,10 @@  args:
   resolving manifests
    branchmerge: True, force: False, partial: False
    ancestor: 924404dff337, local: 86a2aa42fc76+, remote: f4db7e329e71
+   a: remote is newer -> g
+  getting a
    preserving b for resolve of b
    preserving rev for resolve of rev
-   a: remote is newer -> g
-  getting a
    b: local copied/moved from a -> m (premerge)
   picked tool '* ../merge' for b (binary False symlink False changedelete False) (glob)
   merging b and a to b
@@ -241,9 +242,9 @@  args:
   resolving manifests
    branchmerge: True, force: False, partial: False
    ancestor: 924404dff337, local: 94b33a1b7f2d+, remote: 4ce40f5aca24
-   preserving rev for resolve of rev
    b: remote created -> g
   getting b
+   preserving rev for resolve of rev
    rev: versions differ -> m (premerge)
   picked tool '* ../merge' for rev (binary False symlink False changedelete False) (glob)
   merging rev
@@ -306,11 +307,11 @@  args:
   resolving manifests
    branchmerge: True, force: False, partial: False
    ancestor: 924404dff337, local: 94b33a1b7f2d+, remote: bdb19105162a
-   preserving rev for resolve of rev
    a: other deleted -> r
   removing a
    b: remote created -> g
   getting b
+   preserving rev for resolve of rev
    rev: versions differ -> m (premerge)
   picked tool '* ../merge' for rev (binary False symlink False changedelete False) (glob)
   merging rev
@@ -422,9 +423,9 @@  m "um a c" "um x c" "      " "10 do merg
   note: possible conflict - a was renamed multiple times to:
    b
    c
-   preserving rev for resolve of rev
    c: remote created -> g
   getting c
+   preserving rev for resolve of rev
    rev: versions differ -> m (premerge)
   picked tool '* ../merge' for rev (binary False symlink False changedelete False) (glob)
   merging rev
@@ -493,10 +494,10 @@  m "um a c" "um x c" "      " "10 do merg
   resolving manifests
    branchmerge: True, force: False, partial: False
    ancestor: 924404dff337, local: 59318016310c+, remote: bdb19105162a
+   a: other deleted -> r
+  removing a
    preserving b for resolve of b
    preserving rev for resolve of rev
-   a: other deleted -> r
-  removing a
   starting 4 threads for background file closing (?)
    b: both created -> m (premerge)
   picked tool '* ../merge' for b (binary False symlink False changedelete False) (glob)
@@ -534,10 +535,10 @@  m "um a c" "um x c" "      " "10 do merg
   resolving manifests
    branchmerge: True, force: False, partial: False
    ancestor: 924404dff337, local: 86a2aa42fc76+, remote: 8dbce441892a
+   a: remote is newer -> g
+  getting a
    preserving b for resolve of b
    preserving rev for resolve of rev
-   a: remote is newer -> g
-  getting a
    b: both renamed from a -> m (premerge)
   picked tool '* ../merge' for b (binary False symlink False changedelete False) (glob)
   merging b
@@ -571,10 +572,10 @@  m "um a c" "um x c" "      " "10 do merg
   resolving manifests
    branchmerge: True, force: False, partial: False
    ancestor: 924404dff337, local: 59318016310c+, remote: bdb19105162a
+   a: other deleted -> r
+  removing a
    preserving b for resolve of b
    preserving rev for resolve of rev
-   a: other deleted -> r
-  removing a
   starting 4 threads for background file closing (?)
    b: both created -> m (premerge)
   picked tool '* ../merge' for b (binary False symlink False changedelete False) (glob)
@@ -612,10 +613,10 @@  m "um a c" "um x c" "      " "10 do merg
   resolving manifests
    branchmerge: True, force: False, partial: False
    ancestor: 924404dff337, local: 86a2aa42fc76+, remote: 8dbce441892a
+   a: remote is newer -> g
+  getting a
    preserving b for resolve of b
    preserving rev for resolve of rev
-   a: remote is newer -> g
-  getting a
    b: both renamed from a -> m (premerge)
   picked tool '* ../merge' for b (binary False symlink False changedelete False) (glob)
   merging b
@@ -653,6 +654,7 @@  m "um a c" "um x c" "      " "10 do merg
    preserving rev for resolve of rev
   starting 4 threads for background file closing (?)
    b: both renamed from a -> m (premerge)
+  starting 4 threads for background file closing (?)
   picked tool '* ../merge' for b (binary False symlink False changedelete False) (glob)
   merging b
   my b@0b76e65c8289+ other b@4ce40f5aca24 ancestor a@924404dff337
@@ -848,10 +850,10 @@  m "nm a b" "um x a" "      " "22 get a, 
   resolving manifests
    branchmerge: True, force: False, partial: False
    ancestor: 924404dff337, local: 02963e448370+, remote: 2b958612230f
+   c: remote created -> g
+  getting c
    preserving b for resolve of b
    preserving rev for resolve of rev
-   c: remote created -> g
-  getting c
    b: local copied/moved from a -> m (premerge)
   picked tool '* ../merge' for b (binary False symlink False changedelete False) (glob)
   merging b and a to b
diff --git a/tests/test-up-local-change.t b/tests/test-up-local-change.t
--- a/tests/test-up-local-change.t
+++ b/tests/test-up-local-change.t
@@ -43,9 +43,9 @@ 
   resolving manifests
    branchmerge: False, force: False, partial: False
    ancestor: c19d34741b0a, local: c19d34741b0a+, remote: 1e71731e6fbb
-   preserving a for resolve of a
    b: remote created -> g
   getting b
+   preserving a for resolve of a
    a: versions differ -> m (premerge)
   picked tool 'true' for a (binary False symlink False changedelete False)
   merging a
@@ -68,9 +68,9 @@ 
   resolving manifests
    branchmerge: False, force: False, partial: False
    ancestor: 1e71731e6fbb, local: 1e71731e6fbb+, remote: c19d34741b0a
-   preserving a for resolve of a
    b: other deleted -> r
   removing b
+   preserving a for resolve of a
   starting 4 threads for background file closing (?)
    a: versions differ -> m (premerge)
   picked tool 'true' for a (binary False symlink False changedelete False)
@@ -92,9 +92,9 @@ 
   resolving manifests
    branchmerge: False, force: False, partial: False
    ancestor: c19d34741b0a, local: c19d34741b0a+, remote: 1e71731e6fbb
-   preserving a for resolve of a
    b: remote created -> g
   getting b
+   preserving a for resolve of a
    a: versions differ -> m (premerge)
   picked tool 'true' for a (binary False symlink False changedelete False)
   merging a