| 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
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?
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.
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.
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