Patchwork [6,of,6] merge: reuse batchremove method for 'rm' action

login
register
mail settings
Submitter Martin von Zweigbergk
Date Dec. 3, 2014, 5:17 p.m.
Message ID <0f8f7c4be514c6b979f3.1417627036@martinvonz.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/6982/
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 1416809658 28800
#      Sun Nov 23 22:14:18 2014 -0800
# Node ID 0f8f7c4be514c6b979f3e4710adba9b42bd4c660
# Parent  79d509ee6a41ef5903bf78cc75d9cd0e5213807d
merge: reuse batchremove method for 'rm' action

The batchremove method does what we want for the 'rm' action, so let's
use that method for 'rm' actions as well. We don't care about the
progress for files that have been renamed away, and worker() doesn't
necessarily evauluate the function passed in unless one reads the
progress, we need to force evaluation by iterating over the progress
output.
Mads Kiilerich - Dec. 4, 2014, 3:57 a.m.
On 12/03/2014 06:17 PM, Martin von Zweigbergk wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1416809658 28800
> #      Sun Nov 23 22:14:18 2014 -0800
> # Node ID 0f8f7c4be514c6b979f3e4710adba9b42bd4c660
> # Parent  79d509ee6a41ef5903bf78cc75d9cd0e5213807d
> merge: reuse batchremove method for 'rm' action
>
> The batchremove method does what we want for the 'rm' action, so let's
> use that method for 'rm' actions as well. We don't care about the
> progress for files that have been renamed away, and worker() doesn't
> necessarily evauluate the function passed in unless one reads the
> progress, we need to force evaluation by iterating over the progress
> output.
>
> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- a/mercurial/merge.py
> +++ b/mercurial/merge.py
> @@ -638,7 +638,7 @@
>   
>       return actions
>   
> -def batchremove(repo, actions):
> +def batchremove(repo, actiontype, actions):
>       """apply removes to the working directory
>   
>       yields tuples for progress updates
> @@ -649,7 +649,7 @@
>       audit = repo.wopener.audit
>       i = 0
>       for f, args, msg in actions:
> -        repo.ui.debug(" %s: %s -> r\n" % (f, msg))
> +        repo.ui.debug(" %s: %s -> %s\n" % (f, msg, actiontype))

In the next line we jump through loops to avoid unnecessary % expansion. 
I guess we should do that even more for debug. Or is the next line 
mainly to avoid _() calls?

I guess that is a reminder that we should move ui methods towards a way 
where parameters just are passed and _() and % only are processed if the 
message actually is shown.

>           if verbose:
>               repo.ui.note(_("removing %s\n") % f)
>           audit(f)

Series LGTM - nice cleanups there!

/Mads
Martin von Zweigbergk - Dec. 4, 2014, 5:18 a.m.
On Wed Dec 03 2014 at 7:57:55 PM Mads Kiilerich <mads@kiilerich.com> wrote:

> Series LGTM - nice cleanups there!
>
>
Thanks for reviewing. Unfortunately, I think you missed my other message in
reply to 3/6 saying that I'd like to wait with 3/6-6/6. I just sent you a
V2 with a different goal, but with the first two patches in common. I may
still decide I like 3/6-6/6 enough to polish them up later; I just haven't
made up my mind yet. Thanks.

Patch

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -638,7 +638,7 @@ 
 
     return actions
 
-def batchremove(repo, actions):
+def batchremove(repo, actiontype, actions):
     """apply removes to the working directory
 
     yields tuples for progress updates
@@ -649,7 +649,7 @@ 
     audit = repo.wopener.audit
     i = 0
     for f, args, msg in actions:
-        repo.ui.debug(" %s: %s -> r\n" % (f, msg))
+        repo.ui.debug(" %s: %s -> %s\n" % (f, msg, actiontype))
         if verbose:
             repo.ui.note(_("removing %s\n") % f)
         audit(f)
@@ -724,26 +724,24 @@ 
     _files = _('files')
     progress = repo.ui.progress
 
-    # remove renamed files after safely stored
-    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 not in ('k', 'rm'))
 
     if [a for a in actions['r'] if a[0] == '.hgsubstate']:
         subrepo.submerge(repo, wctx, mctx, wctx, overwrite)
 
+    z = 0
     # remove in parallel (must come first)
-    z = 0
-    prog = worker.worker(repo.ui, 0.001, batchremove, (repo,), actions['r'])
+    prog = worker.worker(repo.ui, 0.001, batchremove, (repo, 'r'), actions['r'])
     for i, item in prog:
         z += i
         progress(_updating, z, item=item, total=numupdates, unit=_files)
     removed = len(actions['r'])
 
+    # remove renamed files after safely stored
+    prog = worker.worker(repo.ui, 0.001, batchremove,
+                         (repo, 'rm'), actions['rm'])
+    list(prog) # wait for completion (or force evaluation)
+
     # get in parallel
     prog = worker.worker(repo.ui, 0.001, batchget, (repo, mctx), actions['g'])
     for i, item in prog:
diff --git a/tests/test-copy-move-merge.t b/tests/test-copy-move-merge.t
--- a/tests/test-copy-move-merge.t
+++ b/tests/test-copy-move-merge.t
@@ -33,6 +33,7 @@ 
    ancestor: b8bf91eeebbc, local: add3f11052fa+, remote: 17c05bb7fcb6
    preserving a for resolve of b
    preserving a for resolve of c
+   a: remote moved away -> rm
   removing a
    b: remote moved from a -> m
   updating: b 1/2 files (50.00%)
diff --git a/tests/test-issue672.t b/tests/test-issue672.t
--- a/tests/test-issue672.t
+++ b/tests/test-issue672.t
@@ -90,6 +90,7 @@ 
    branchmerge: True, force: False, partial: False
    ancestor: c64f439569a9, local: 746e9549ea96+, remote: e327dca35ac8
    preserving 1 for resolve of 1a
+   1: remote moved away -> rm
   removing 1
    1a: remote moved from 1 -> m
   updating: 1a 1/1 files (100.00%)
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
@@ -37,6 +37,7 @@ 
    branchmerge: True, force: False, partial: False
    ancestor: af1939970a1c, local: 044f8520aeeb+, remote: 85c198ef2f6c
    preserving a for resolve of b
+   a: remote moved away -> rm
   removing a
    b2: remote created -> g
   getting b2
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
@@ -162,6 +162,7 @@ 
    ancestor: 924404dff337, local: e300d1c794ec+, remote: bdb19105162a
    preserving a for resolve of b
    preserving rev for resolve of rev
+   a: remote moved away -> rm
   removing a
    b: remote moved from a -> m
   updating: b 1/2 files (50.00%)
@@ -676,6 +677,7 @@ 
    ancestor: 924404dff337, local: e300d1c794ec+, remote: 49b6d8032493
    preserving a for resolve of b
    preserving rev for resolve of rev
+   a: remote moved away -> rm
   removing a
    b: remote moved from a -> m
   updating: b 1/2 files (50.00%)
@@ -846,6 +848,7 @@ 
    preserving 5/g for resolve of 5/g
    preserving 6/g for resolve of 6/g
    preserving 7/f for resolve of 7/f
+   4/f: remote moved away -> rm
   removing 4/f
    8/f: prompt recreating -> g
   getting 8/f