Patchwork D1378: bundlerepo: assign bundle attributes in bundle type blocks

login
register
mail settings
Submitter phabricator
Date Nov. 12, 2017, 2:52 a.m.
Message ID <differential-rev-PHID-DREV-xes63khwnoformzs3egk-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/25495/
State Superseded
Headers show

Comments

phabricator - Nov. 12, 2017, 2:52 a.m.
indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  It is a bit wonky to assign the same object to multiple
  attributes and then possibly overwrite them later.
  
  Refactor the code to use a local variable and defer attribute
  assignment until the final values are ready.
  
  This required passing the bundle instance to _handlebundle2part().
  The only use of this method I could find is Facebook's
  treemanifest extension. Since it is a private method, I don't
  think it warrants an API callout.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/bundlerepo.py

CHANGE DETAILS




To: indygreg, #hg-reviewers
Cc: mercurial-devel
phabricator - Nov. 13, 2017, 8:05 a.m.
dlax added inline comments.

INLINE COMMENTS

> bundlerepo.py:298
>  
> -                self._handlebundle2part(part)
> +                self._handlebundle2part(bundle, part)
>  

Within `_handlebundle2part`, there's still a `self._bundle = changegroup.getunbundler(...)` statement. Perhaps, it'd be even clearer if this method would `return changegroup.getunbundler(...)` and let the caller eventually assign this as an attribute.

REPOSITORY
  rHG Mercurial

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

To: indygreg, #hg-reviewers, dlax
Cc: mercurial-devel
phabricator - Nov. 13, 2017, 5:06 p.m.
indygreg added inline comments.

INLINE COMMENTS

> dlax wrote in bundlerepo.py:298
> Within `_handlebundle2part`, there's still a `self._bundle = changegroup.getunbundler(...)` statement. Perhaps, it'd be even clearer if this method would `return changegroup.getunbundler(...)` and let the caller eventually assign this as an attribute.

Yes, this API would be cleaner.

The reason I hesitated is `_handlebundle2part()` is generic. It feels weird to return a changegroup unpacker from such a generic method.

Since I'm incurring an API break in this series anyway and since treemanifests is the only thing extending this code AFAICT, I may just refactor things further.

REPOSITORY
  rHG Mercurial

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

To: indygreg, #hg-reviewers, dlax
Cc: mercurial-devel

Patch

diff --git a/mercurial/bundlerepo.py b/mercurial/bundlerepo.py
--- a/mercurial/bundlerepo.py
+++ b/mercurial/bundlerepo.py
@@ -281,47 +281,52 @@ 
 
         self.tempfile = None
         f = util.posixfile(bundlepath, "rb")
-        self._bundlefile = self._bundle = exchange.readbundle(ui, f, bundlepath)
+        bundle = exchange.readbundle(ui, f, bundlepath)
 
-        if isinstance(self._bundle, bundle2.unbundle20):
+        if isinstance(bundle, bundle2.unbundle20):
+            self._bundlefile = bundle
+            self._bundle = None
+
             hadchangegroup = False
-            for part in self._bundle.iterparts():
+            for part in bundle.iterparts():
                 if part.type == 'changegroup':
                     if hadchangegroup:
                         raise NotImplementedError("can't process "
                                                   "multiple changegroups")
                     hadchangegroup = True
 
-                self._handlebundle2part(part)
+                self._handlebundle2part(bundle, part)
 
             if not hadchangegroup:
                 raise error.Abort(_("No changegroups found"))
-        elif isinstance(self._bundle, changegroup.cg1unpacker):
-            if self._bundle.compressed():
-                f = self._writetempbundle(self._bundle.read, '.hg10un',
+        elif isinstance(bundle, changegroup.cg1unpacker):
+            if bundle.compressed():
+                f = self._writetempbundle(bundle.read, '.hg10un',
                                           header='HG10UN')
-                self._bundlefile = self._bundle = exchange.readbundle(
-                    ui, f, bundlepath, self.vfs)
+                bundle = exchange.readbundle(ui, f, bundlepath, self.vfs)
+
+            self._bundlefile = bundle
+            self._bundle = bundle
         else:
             raise error.Abort(_('bundle type %s cannot be read') %
-                              type(self._bundle))
+                              type(bundle))
 
         # dict with the mapping 'filename' -> position in the bundle
         self.bundlefilespos = {}
 
         self.firstnewrev = self.changelog.repotiprev + 1
         phases.retractboundary(self, None, phases.draft,
                                [ctx.node() for ctx in self[self.firstnewrev:]])
 
-    def _handlebundle2part(self, part):
+    def _handlebundle2part(self, bundle, part):
         if part.type == 'changegroup':
             cgstream = part
             version = part.params.get('version', '01')
             legalcgvers = changegroup.supportedincomingversions(self)
             if version not in legalcgvers:
                 msg = _('Unsupported changegroup version: %s')
                 raise error.Abort(msg % version)
-            if self._bundle.compressed():
+            if bundle.compressed():
                 cgstream = self._writetempbundle(part.read,
                                                  ".cg%sun" % version)