Patchwork [1,of,2,v3] util: add progress callback support to copyfiles

login
register
mail settings
Submitter Augie Fackler
Date March 24, 2015, 8:45 p.m.
Message ID <9bff84a5d07004cfe7ac.1427229928@augie-macbookair2.roam.corp.google.com>
Download mbox | patch
Permalink /patch/8242/
State Accepted
Commit 2ddfac2f163e1f287c65732273936d121ab694b2
Headers show

Comments

Augie Fackler - March 24, 2015, 8:45 p.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1426775062 14400
#      Thu Mar 19 10:24:22 2015 -0400
# Node ID 9bff84a5d07004cfe7ac2b1dfbdfb0f59ca37e18
# Parent  5b85a5bc5bbb9d8365953609d98e4dce7110e9b0
util: add progress callback support to copyfiles
Adrian Buehlmann - March 24, 2015, 9:08 p.m.
On 2015-03-24 21:45, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1426775062 14400
> #      Thu Mar 19 10:24:22 2015 -0400
> # Node ID 9bff84a5d07004cfe7ac2b1dfbdfb0f59ca37e18
> # Parent  5b85a5bc5bbb9d8365953609d98e4dce7110e9b0
> util: add progress callback support to copyfiles
> 
> diff --git a/mercurial/hg.py b/mercurial/hg.py
> --- a/mercurial/hg.py
> +++ b/mercurial/hg.py
> @@ -258,8 +258,8 @@ def copystore(ui, srcrepo, destpath):
>                      lockfile = os.path.join(dstbase, "lock")
>                      # lock to avoid premature writing to the target
>                      destlock = lock.lock(dstvfs, lockfile)
> -                hardlink, n = util.copyfiles(srcvfs.join(f), dstvfs.join(f),
> -                                             hardlink)
> +                hardlink, n = util.copyfiles(
> +                    srcvfs.join(f), dstvfs.join(f), hardlink)
>                  num += n
>          if hardlink:
>              ui.debug("linked %d files\n" % num)

This looks like a simple reformatting. Was that intended?
Augie Fackler - March 24, 2015, 9:13 p.m.
On Tue, Mar 24, 2015 at 5:08 PM, Adrian Buehlmann <adrian@cadifra.com> wrote:
>> diff --git a/mercurial/hg.py b/mercurial/hg.py
>> --- a/mercurial/hg.py
>> +++ b/mercurial/hg.py
>> @@ -258,8 +258,8 @@ def copystore(ui, srcrepo, destpath):
>>                      lockfile = os.path.join(dstbase, "lock")
>>                      # lock to avoid premature writing to the target
>>                      destlock = lock.lock(dstvfs, lockfile)
>> -                hardlink, n = util.copyfiles(srcvfs.join(f), dstvfs.join(f),
>> -                                             hardlink)
>> +                hardlink, n = util.copyfiles(
>> +                    srcvfs.join(f), dstvfs.join(f), hardlink)
>>                  num += n
>>          if hardlink:
>>              ui.debug("linked %d files\n" % num)
>
> This looks like a simple reformatting. Was that intended?


Ugh, no. I'll roll a v4. Good catch.
Martin von Zweigbergk - March 24, 2015, 9:19 p.m.
On Tue, Mar 24, 2015 at 2:13 PM Augie Fackler <raf@durin42.com> wrote:

> On Tue, Mar 24, 2015 at 5:08 PM, Adrian Buehlmann <adrian@cadifra.com>
> wrote:
> >> diff --git a/mercurial/hg.py b/mercurial/hg.py
> >> --- a/mercurial/hg.py
> >> +++ b/mercurial/hg.py
> >> @@ -258,8 +258,8 @@ def copystore(ui, srcrepo, destpath):
> >>                      lockfile = os.path.join(dstbase, "lock")
> >>                      # lock to avoid premature writing to the target
> >>                      destlock = lock.lock(dstvfs, lockfile)
> >> -                hardlink, n = util.copyfiles(srcvfs.join(f),
> dstvfs.join(f),
> >> -                                             hardlink)
> >> +                hardlink, n = util.copyfiles(
> >> +                    srcvfs.join(f), dstvfs.join(f), hardlink)
> >>                  num += n
> >>          if hardlink:
> >>              ui.debug("linked %d files\n" % num)
> >
> > This looks like a simple reformatting. Was that intended?
>
>
> Ugh, no. I'll roll a v4. Good catch.
>

If you're rolling a v4 anyway:

* The 'num' didn't seem to need to move, so revert that?
* Sorry I didn't realize before, but wouldn't it be better to pass just a
boolean value (True for hardlink) to the progress callback and keep all the
debug messages out of copyfiles()?
* Does it make sense to move the "linked %d files\n" messages to the "if
pos is None" case? inside the callback? I'm not sure, just a thought. That
would remove the need for the 'closetopic' container.
Martin von Zweigbergk - March 24, 2015, 9:31 p.m.
On Tue, Mar 24, 2015 at 2:19 PM Martin von Zweigbergk <martinvonz@google.com>
wrote:

> On Tue, Mar 24, 2015 at 2:13 PM Augie Fackler <raf@durin42.com> wrote:
>
>> On Tue, Mar 24, 2015 at 5:08 PM, Adrian Buehlmann <adrian@cadifra.com>
>> wrote:
>> >> diff --git a/mercurial/hg.py b/mercurial/hg.py
>> >> --- a/mercurial/hg.py
>> >> +++ b/mercurial/hg.py
>> >> @@ -258,8 +258,8 @@ def copystore(ui, srcrepo, destpath):
>> >>                      lockfile = os.path.join(dstbase, "lock")
>> >>                      # lock to avoid premature writing to the target
>> >>                      destlock = lock.lock(dstvfs, lockfile)
>> >> -                hardlink, n = util.copyfiles(srcvfs.join(f),
>> dstvfs.join(f),
>> >> -                                             hardlink)
>> >> +                hardlink, n = util.copyfiles(
>> >> +                    srcvfs.join(f), dstvfs.join(f), hardlink)
>> >>                  num += n
>> >>          if hardlink:
>> >>              ui.debug("linked %d files\n" % num)
>> >
>> > This looks like a simple reformatting. Was that intended?
>>
>>
>> Ugh, no. I'll roll a v4. Good catch.
>>
>
> If you're rolling a v4 anyway:
>
> * The 'num' didn't seem to need to move, so revert that?
> * Sorry I didn't realize before, but wouldn't it be better to pass just a
> boolean value (True for hardlink) to the progress callback and keep all the
> debug messages out of copyfiles()?
> * Does it make sense to move the "linked %d files\n" messages to the "if
> pos is None" case? inside the callback? I'm not sure, just a thought. That
> would remove the need for the 'closetopic' container.
>
>
Never mind that last comment. I didn't realize  copyfiles() is called
several times in sequence.

Patch

diff --git a/mercurial/hg.py b/mercurial/hg.py
--- a/mercurial/hg.py
+++ b/mercurial/hg.py
@@ -258,8 +258,8 @@  def copystore(ui, srcrepo, destpath):
                     lockfile = os.path.join(dstbase, "lock")
                     # lock to avoid premature writing to the target
                     destlock = lock.lock(dstvfs, lockfile)
-                hardlink, n = util.copyfiles(srcvfs.join(f), dstvfs.join(f),
-                                             hardlink)
+                hardlink, n = util.copyfiles(
+                    srcvfs.join(f), dstvfs.join(f), hardlink)
                 num += n
         if hardlink:
             ui.debug("linked %d files\n" % num)
diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -739,20 +739,27 @@  def copyfile(src, dest, hardlink=False):
         except shutil.Error, inst:
             raise Abort(str(inst))
 
-def copyfiles(src, dst, hardlink=None):
-    """Copy a directory tree using hardlinks if possible"""
+def copyfiles(src, dst, hardlink=None, progress=lambda t, pos: None):
+    """Copy a directory tree using hardlinks if possible."""
+    num = 0
 
     if hardlink is None:
         hardlink = (os.stat(src).st_dev ==
                     os.stat(os.path.dirname(dst)).st_dev)
+    if hardlink:
+        topic = _('linking')
+    else:
+        topic = _('copying')
 
-    num = 0
     if os.path.isdir(src):
         os.mkdir(dst)
         for name, kind in osutil.listdir(src):
             srcname = os.path.join(src, name)
             dstname = os.path.join(dst, name)
-            hardlink, n = copyfiles(srcname, dstname, hardlink)
+            def nprog(t, pos):
+                if pos is not None:
+                    return progress(t, pos + num)
+            hardlink, n = copyfiles(srcname, dstname, hardlink, progress=nprog)
             num += n
     else:
         if hardlink:
@@ -764,6 +771,8 @@  def copyfiles(src, dst, hardlink=None):
         else:
             shutil.copy(src, dst)
         num += 1
+        progress(topic, num)
+    progress(topic, None)
 
     return hardlink, num