Patchwork D288: bundlerepo: move temp bundle creation to a separate function

login
register
mail settings
Submitter phabricator
Date Aug. 9, 2017, 12:05 a.m.
Message ID <differential-rev-PHID-DREV-3tnmsqav2vnbtidfgwzh-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/22774/
State Superseded
Headers show

Comments

phabricator - Aug. 9, 2017, 12:05 a.m.
durham created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  A future patch will refactor certain parts of bundlerepo initiatlization such
  that we need to create temp bundles from another function. Let's move this to
  another function to support that.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D288

AFFECTED FILES
  mercurial/bundlerepo.py

CHANGE DETAILS




To: durham, #hg-reviewers
Cc: mercurial-devel
phabricator - Aug. 9, 2017, 12:39 p.m.
lothiraldan added a comment.


  LGTM

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D288

To: durham, #hg-reviewers
Cc: lothiraldan, mercurial-devel
phabricator - Aug. 15, 2017, 1:40 a.m.
indygreg requested changes to this revision.
indygreg added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> bundlerepo.py:325-326
> +
> +        This is closure because we need to make sure this tracked by
> +        self.tempfile for cleanup purposes."""
> +        fdtemp, temp = self.vfs.mkstemp(prefix="hg-bundle-",

Please update this to reflect that it is no longer a closure.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D288

To: durham, #hg-reviewers, indygreg
Cc: indygreg, lothiraldan, mercurial-devel
phabricator - Aug. 23, 2017, 5:48 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> bundlerepo.py:322
>  
> +    def _writetempbundle(self, read, suffix, header=''):
> +        """Write a temporary file to disk

I think we usually use a "fn" prefix for function arguments. I think "readfn" would be clearer.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D288

To: durham, #hg-reviewers, indygreg
Cc: martinvonz, indygreg, lothiraldan, mercurial-devel
phabricator - Aug. 23, 2017, 6:30 p.m.
krbullock added inline comments.

INLINE COMMENTS

> martinvonz wrote in bundlerepo.py:322
> I think we usually use a "fn" prefix for function arguments. I think "readfn" would be clearer.

I agree, but as it is, this is simple code movement. Usually we like renames to be done in separate changes. Updating the docstring as @indygreg suggests is appropriate, though.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D288

To: durham, #hg-reviewers, indygreg
Cc: krbullock, martinvonz, indygreg, lothiraldan, mercurial-devel
phabricator - Aug. 23, 2017, 7:40 p.m.
durham added a comment.


  I fixed the comment and renamed read to readfn. I know we usually do renames as separate from code moves, but this was a tiny rename (only one use).

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D288

To: durham, #hg-reviewers, indygreg
Cc: krbullock, martinvonz, indygreg, lothiraldan, mercurial-devel

Patch

diff --git a/mercurial/bundlerepo.py b/mercurial/bundlerepo.py
--- a/mercurial/bundlerepo.py
+++ b/mercurial/bundlerepo.py
@@ -264,24 +264,6 @@ 
 
 class bundlerepository(localrepo.localrepository):
     def __init__(self, ui, path, bundlename):
-        def _writetempbundle(read, suffix, header=''):
-            """Write a temporary file to disk
-
-            This is closure because we need to make sure this tracked by
-            self.tempfile for cleanup purposes."""
-            fdtemp, temp = self.vfs.mkstemp(prefix="hg-bundle-",
-                                            suffix=".hg10un")
-            self.tempfile = temp
-
-            with os.fdopen(fdtemp, pycompat.sysstr('wb')) as fptemp:
-                fptemp.write(header)
-                while True:
-                    chunk = read(2**18)
-                    if not chunk:
-                        break
-                    fptemp.write(chunk)
-
-            return self.vfs.open(self.tempfile, mode="rb")
         self._tempparent = None
         try:
             localrepo.localrepository.__init__(self, ui, path)
@@ -314,17 +296,18 @@ 
                         msg = _('Unsupported changegroup version: %s')
                         raise error.Abort(msg % version)
                     if self.bundle.compressed():
-                        cgstream = _writetempbundle(part.read,
-                                                    ".cg%sun" % version)
+                        cgstream = self._writetempbundle(part.read,
+                                                         ".cg%sun" % version)
 
             if cgstream is None:
                 raise error.Abort(_('No changegroups found'))
             cgstream.seek(0)
 
             self.bundle = changegroup.getunbundler(version, cgstream, 'UN')
 
         elif self.bundle.compressed():
-            f = _writetempbundle(self.bundle.read, '.hg10un', header='HG10UN')
+            f = self._writetempbundle(self.bundle.read, '.hg10un',
+                                      header='HG10UN')
             self.bundlefile = self.bundle = exchange.readbundle(ui, f,
                                                                 bundlename,
                                                                 self.vfs)
@@ -336,6 +319,25 @@ 
         phases.retractboundary(self, None, phases.draft,
                                [ctx.node() for ctx in self[self.firstnewrev:]])
 
+    def _writetempbundle(self, read, suffix, header=''):
+        """Write a temporary file to disk
+
+        This is closure because we need to make sure this tracked by
+        self.tempfile for cleanup purposes."""
+        fdtemp, temp = self.vfs.mkstemp(prefix="hg-bundle-",
+                                        suffix=".hg10un")
+        self.tempfile = temp
+
+        with os.fdopen(fdtemp, pycompat.sysstr('wb')) as fptemp:
+            fptemp.write(header)
+            while True:
+                chunk = read(2**18)
+                if not chunk:
+                    break
+                fptemp.write(chunk)
+
+        return self.vfs.open(self.tempfile, mode="rb")
+
     @localrepo.unfilteredpropertycache
     def _phasecache(self):
         return bundlephasecache(self, self._phasedefaults)