Patchwork [3,of,6] merge: split move-merge into two actions

login
register
mail settings
Submitter Martin von Zweigbergk
Date Dec. 3, 2014, 5:17 p.m.
Message ID <d7c59ced01f5bc3ddc34.1417627033@martinvonz.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/6979/
State Superseded
Headers show

Comments

Martin von Zweigbergk - Dec. 3, 2014, 5:17 p.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1416786380 28800
#      Sun Nov 23 15:46:20 2014 -0800
# Node ID d7c59ced01f5bc3ddc34c3b597b8915740ed79c0
# Parent  17f0fda2398ba2463589a3a59efe78b095b5d593
merge: split move-merge into two actions

The merge action for merges ('m') carries a 'move' argument, which
tells whether to also remove the copy source. The code can be
simplified by removing that action argument and producing a second
action for the removal instead. Since the removal doesn't behave
exactly like an 'r' action (specifically, it doesn't count as
'removed' file in the stats), we need to create new type of action.
Martin von Zweigbergk - Dec. 3, 2014, 8:41 p.m.
Sorry, let me think a bit more about this before you review it. It might
make it somewhat simpler, but my end goal was to simplify
largefiles' overridecalculateupdates() and maybe bidmerge. I still think
the first two patches are good, so please review those.

On Wed Dec 03 2014 at 9:17:18 AM Martin von Zweigbergk <
martinvonz@google.com> wrote:

> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1416786380 28800
> #      Sun Nov 23 15:46:20 2014 -0800
> # Node ID d7c59ced01f5bc3ddc34c3b597b8915740ed79c0
> # Parent  17f0fda2398ba2463589a3a59efe78b095b5d593
> merge: split move-merge into two actions
>
> The merge action for merges ('m') carries a 'move' argument, which
> tells whether to also remove the copy source. The code can be
> simplified by removing that action argument and producing a second
> action for the removal instead. Since the removal doesn't behave
> exactly like an 'r' action (specifically, it doesn't count as
> 'removed' file in the stats), we need to create new type of action.
>
> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- a/mercurial/merge.py
> +++ b/mercurial/merge.py
> @@ -343,8 +343,9 @@
>          for m in 'a', 'f', 'g', 'cd', 'dc':
>              for f, args, msg in actions[m]:
>                  pmmf.add(f)
> -        for f, args, msg in actions['r']:
> -            pmmf.discard(f)
> +        for m in 'r', 'rm':
> +            for f, args, msg in actions[m]:
> +                pmmf.discard(f)
>          for f, args, msg in actions['dm']:
>              f2, flags = args
>              pmmf.discard(f2)
> @@ -352,9 +353,6 @@
>          for f, args, msg in actions['dg']:
>              pmmf.add(f)
>          for f, args, msg in actions['m']:
> -            f1, f2, fa, move, anc = args
> -            if move:
> -                pmmf.discard(f1)
>              pmmf.add(f)
>
>      # check case-folding collision in provisional merged manifest
> @@ -376,7 +374,8 @@
>      acceptremote = accept the incoming changes without prompting
>      """
>
> -    actions = dict((m, []) for m in 'a f g cd dc r dm dg m dr e rd
> k'.split())
> +    actions = dict((m, []) for m in
> +                   'a f g cd dc r rm dm dg m dr e rd k'.split())
>      copy, movewithdir = {}, {}
>
>      # manifests fetched in order are going to be faster, so prime the
> caches
> @@ -418,10 +417,10 @@
>              if f not in ma:
>                  fa = copy.get(f, None)
>                  if fa is not None:
> -                    actions['m'].append((f, (f, f, fa, False, pa.node()),
> +                    actions['m'].append((f, (f, f, fa, pa.node()),
>                                     "both renamed from " + fa))
>                  else:
> -                    actions['m'].append((f, (f, f, None, False,
> pa.node()),
> +                    actions['m'].append((f, (f, f, None, pa.node()),
>                                     "both created"))
>              else:
>                  a = ma[f]
> @@ -439,7 +438,7 @@
>                  elif nol and n1 == a: # local only changed 'x'
>                      actions['g'].append((f, (fl1,), "remote is newer"))
>                  else: # both changed something
> -                    actions['m'].append((f, (f, f, f, False, pa.node()),
> +                    actions['m'].append((f, (f, f, f, pa.node()),
>                                     "versions differ"))
>          elif n1: # file exists only on local side
>              if f in copied:
> @@ -450,7 +449,7 @@
>                                  "remote directory rename - move from " +
> f))
>              elif f in copy:
>                  f2 = copy[f]
> -                actions['m'].append((f, (f, f2, f2, False, pa.node()),
> +                actions['m'].append((f, (f, f2, f2, pa.node()),
>                                  "local copied/moved from " + f2))
>              elif f in ma: # clean, a different, no remote
>                  if n1 != ma[f]:
> @@ -476,10 +475,12 @@
>              elif f in copy:
>                  f2 = copy[f]
>                  if f2 in m2:
> -                    actions['m'].append((f, (f2, f, f2, False, pa.node()),
> +                    actions['m'].append((f, (f2, f, f2, pa.node()),
>                                      "remote copied from " + f2))
>                  else:
> -                    actions['m'].append((f, (f2, f, f2, True, pa.node()),
> +                    actions['rm'].append((f2, None,
> +                                    "remote moved to " + f))
> +                    actions['m'].append((f, (f2, f, f2, pa.node()),
>                                      "remote moved from " + f2))
>              elif f not in ma:
>                  # local unknown, remote created: the logic is described
> by the
> @@ -499,7 +500,7 @@
>                  else:
>                      different = _checkunknownfile(repo, wctx, p2, f)
>                      if force and branchmerge and different:
> -                        actions['m'].append((f, (f, f, None, False,
> pa.node()),
> +                        actions['m'].append((f, (f, f, None, pa.node()),
>                                          "remote differs from untracked
> local"))
>                      elif not force and different:
>                          aborts.append((f, 'ud'))
> @@ -702,13 +703,12 @@
>      updated, merged, removed, unresolved = 0, 0, 0, 0
>      ms = mergestate(repo)
>      ms.reset(wctx.p1().node(), mctx.node())
> -    moves = []
>      for m, l in actions.items():
>          l.sort()
>
>      # prescan for merges
>      for f, args, msg in actions['m']:
> -        f1, f2, fa, move, anc = args
> +        f1, f2, fa, anc = args
>          if f == '.hgsubstate': # merged internally
>              continue
>          repo.ui.debug(" preserving %s for resolve of %s\n" % (f1, f))
> @@ -720,8 +720,6 @@
>          else:
>              fca = repo.filectx(f1, fileid=nullrev)
>          ms.add(fcl, fco, fca, f)
> -        if f1 != f and move:
> -            moves.append(f1)
>
>      audit = repo.wopener.audit
>      _updating = _('updating')
> @@ -729,13 +727,13 @@
>      progress = repo.ui.progress
>
>      # remove renamed files after safely stored
> -    for f in moves:
> +    for f, args, msg in actions['rm']:
>          if os.path.lexists(repo.wjoin(f)):
>              repo.ui.debug("removing %s\n" % f)
>              audit(f)
>              util.unlinkpath(repo.wjoin(f))
>
> -    numupdates = sum(len(l) for m, l in actions.items() if m != 'k')
> +    numupdates = sum(len(l) for m, l in actions.items() if m not in ('k',
> 'rm'))
>
>      if [a for a in actions['r'] if a[0] == '.hgsubstate']:
>          subrepo.submerge(repo, wctx, mctx, wctx, overwrite)
> @@ -888,14 +886,12 @@
>
>      # merge
>      for f, args, msg in actions['m']:
> -        f1, f2, fa, move, anc = args
> +        f1, f2, fa, anc = args
>          if branchmerge:
>              # We've done a branch merge, mark this file as merged
>              # so that we properly record the merger later
>              repo.dirstate.merge(f)
>              if f1 != f2: # copy/rename
> -                if move:
> -                    repo.dirstate.remove(f1)
>                  if f1 != f:
>                      repo.dirstate.copy(f1, f)
>                  else:
> @@ -908,8 +904,13 @@
>              # modification.
>              if f2 == f: # file not locally copied/moved
>                  repo.dirstate.normallookup(f)
> -            if move:
> -                repo.dirstate.drop(f1)
> +
> +    # remove file that moved away
> +    for f, args, msg in actions['rm']:
> +        if branchmerge:
> +            repo.dirstate.remove(f)
> +        else:
> +            repo.dirstate.drop(f)
>
>      # directory rename, move local
>      for f, args, msg in actions['dm']:
>
Mads Kiilerich - Dec. 4, 2014, 3:56 a.m.
On 12/03/2014 06:17 PM, Martin von Zweigbergk wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1416786380 28800
> #      Sun Nov 23 15:46:20 2014 -0800
> # Node ID d7c59ced01f5bc3ddc34c3b597b8915740ed79c0
> # Parent  17f0fda2398ba2463589a3a59efe78b095b5d593
> merge: split move-merge into two actions
>
> The merge action for merges ('m') carries a 'move' argument, which
> tells whether to also remove the copy source. The code can be
> simplified by removing that action argument and producing a second
> action for the removal instead. Since the removal doesn't behave
> exactly like an 'r' action (specifically, it doesn't count as
> 'removed' file in the stats), we need to create new type of action.
>
> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- a/mercurial/merge.py
> +++ b/mercurial/merge.py
> @@ -343,8 +343,9 @@
>           for m in 'a', 'f', 'g', 'cd', 'dc':
>               for f, args, msg in actions[m]:
>                   pmmf.add(f)
> -        for f, args, msg in actions['r']:
> -            pmmf.discard(f)
> +        for m in 'r', 'rm':

I would expect () around the tuple here. But well ... it seems to work 
without it. But still ...
I would perhaps prefer actions['r']+actions['m'] in the next line ...
(and same pattern in later patches)
but perhaps not ...

> +            for f, args, msg in actions[m]:
> +                pmmf.discard(f)
>           for f, args, msg in actions['dm']:
>               f2, flags = args
>               pmmf.discard(f2)
> @@ -352,9 +353,6 @@
>           for f, args, msg in actions['dg']:
>               pmmf.add(f)
>           for f, args, msg in actions['m']:
> -            f1, f2, fa, move, anc = args
> -            if move:
> -                pmmf.discard(f1)
>               pmmf.add(f)
>   
>       # check case-folding collision in provisional merged manifest
> @@ -376,7 +374,8 @@
>       acceptremote = accept the incoming changes without prompting
>       """
>   
> -    actions = dict((m, []) for m in 'a f g cd dc r dm dg m dr e rd k'.split())
> +    actions = dict((m, []) for m in
> +                   'a f g cd dc r rm dm dg m dr e rd k'.split())

I wonder if this should be turned into a named dict too ;-)

/Mads

Patch

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -343,8 +343,9 @@ 
         for m in 'a', 'f', 'g', 'cd', 'dc':
             for f, args, msg in actions[m]:
                 pmmf.add(f)
-        for f, args, msg in actions['r']:
-            pmmf.discard(f)
+        for m in 'r', 'rm':
+            for f, args, msg in actions[m]:
+                pmmf.discard(f)
         for f, args, msg in actions['dm']:
             f2, flags = args
             pmmf.discard(f2)
@@ -352,9 +353,6 @@ 
         for f, args, msg in actions['dg']:
             pmmf.add(f)
         for f, args, msg in actions['m']:
-            f1, f2, fa, move, anc = args
-            if move:
-                pmmf.discard(f1)
             pmmf.add(f)
 
     # check case-folding collision in provisional merged manifest
@@ -376,7 +374,8 @@ 
     acceptremote = accept the incoming changes without prompting
     """
 
-    actions = dict((m, []) for m in 'a f g cd dc r dm dg m dr e rd k'.split())
+    actions = dict((m, []) for m in
+                   'a f g cd dc r rm dm dg m dr e rd k'.split())
     copy, movewithdir = {}, {}
 
     # manifests fetched in order are going to be faster, so prime the caches
@@ -418,10 +417,10 @@ 
             if f not in ma:
                 fa = copy.get(f, None)
                 if fa is not None:
-                    actions['m'].append((f, (f, f, fa, False, pa.node()),
+                    actions['m'].append((f, (f, f, fa, pa.node()),
                                    "both renamed from " + fa))
                 else:
-                    actions['m'].append((f, (f, f, None, False, pa.node()),
+                    actions['m'].append((f, (f, f, None, pa.node()),
                                    "both created"))
             else:
                 a = ma[f]
@@ -439,7 +438,7 @@ 
                 elif nol and n1 == a: # local only changed 'x'
                     actions['g'].append((f, (fl1,), "remote is newer"))
                 else: # both changed something
-                    actions['m'].append((f, (f, f, f, False, pa.node()),
+                    actions['m'].append((f, (f, f, f, pa.node()),
                                    "versions differ"))
         elif n1: # file exists only on local side
             if f in copied:
@@ -450,7 +449,7 @@ 
                                 "remote directory rename - move from " + f))
             elif f in copy:
                 f2 = copy[f]
-                actions['m'].append((f, (f, f2, f2, False, pa.node()),
+                actions['m'].append((f, (f, f2, f2, pa.node()),
                                 "local copied/moved from " + f2))
             elif f in ma: # clean, a different, no remote
                 if n1 != ma[f]:
@@ -476,10 +475,12 @@ 
             elif f in copy:
                 f2 = copy[f]
                 if f2 in m2:
-                    actions['m'].append((f, (f2, f, f2, False, pa.node()),
+                    actions['m'].append((f, (f2, f, f2, pa.node()),
                                     "remote copied from " + f2))
                 else:
-                    actions['m'].append((f, (f2, f, f2, True, pa.node()),
+                    actions['rm'].append((f2, None,
+                                    "remote moved to " + f))
+                    actions['m'].append((f, (f2, f, f2, pa.node()),
                                     "remote moved from " + f2))
             elif f not in ma:
                 # local unknown, remote created: the logic is described by the
@@ -499,7 +500,7 @@ 
                 else:
                     different = _checkunknownfile(repo, wctx, p2, f)
                     if force and branchmerge and different:
-                        actions['m'].append((f, (f, f, None, False, pa.node()),
+                        actions['m'].append((f, (f, f, None, pa.node()),
                                         "remote differs from untracked local"))
                     elif not force and different:
                         aborts.append((f, 'ud'))
@@ -702,13 +703,12 @@ 
     updated, merged, removed, unresolved = 0, 0, 0, 0
     ms = mergestate(repo)
     ms.reset(wctx.p1().node(), mctx.node())
-    moves = []
     for m, l in actions.items():
         l.sort()
 
     # prescan for merges
     for f, args, msg in actions['m']:
-        f1, f2, fa, move, anc = args
+        f1, f2, fa, anc = args
         if f == '.hgsubstate': # merged internally
             continue
         repo.ui.debug(" preserving %s for resolve of %s\n" % (f1, f))
@@ -720,8 +720,6 @@ 
         else:
             fca = repo.filectx(f1, fileid=nullrev)
         ms.add(fcl, fco, fca, f)
-        if f1 != f and move:
-            moves.append(f1)
 
     audit = repo.wopener.audit
     _updating = _('updating')
@@ -729,13 +727,13 @@ 
     progress = repo.ui.progress
 
     # remove renamed files after safely stored
-    for f in moves:
+    for f, args, msg in actions['rm']:
         if os.path.lexists(repo.wjoin(f)):
             repo.ui.debug("removing %s\n" % f)
             audit(f)
             util.unlinkpath(repo.wjoin(f))
 
-    numupdates = sum(len(l) for m, l in actions.items() if m != 'k')
+    numupdates = sum(len(l) for m, l in actions.items() if m not in ('k', 'rm'))
 
     if [a for a in actions['r'] if a[0] == '.hgsubstate']:
         subrepo.submerge(repo, wctx, mctx, wctx, overwrite)
@@ -888,14 +886,12 @@ 
 
     # merge
     for f, args, msg in actions['m']:
-        f1, f2, fa, move, anc = args
+        f1, f2, fa, anc = args
         if branchmerge:
             # We've done a branch merge, mark this file as merged
             # so that we properly record the merger later
             repo.dirstate.merge(f)
             if f1 != f2: # copy/rename
-                if move:
-                    repo.dirstate.remove(f1)
                 if f1 != f:
                     repo.dirstate.copy(f1, f)
                 else:
@@ -908,8 +904,13 @@ 
             # modification.
             if f2 == f: # file not locally copied/moved
                 repo.dirstate.normallookup(f)
-            if move:
-                repo.dirstate.drop(f1)
+
+    # remove file that moved away
+    for f, args, msg in actions['rm']:
+        if branchmerge:
+            repo.dirstate.remove(f)
+        else:
+            repo.dirstate.drop(f)
 
     # directory rename, move local
     for f, args, msg in actions['dm']: