This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1378

bundlerepo: assign bundle attributes in bundle type blocks
ClosedPublic

Authored by indygreg on Nov 11 2017, 9:52 PM.

Details

Reviewers
dlax
durin42
Group Reviewers
hg-reviewers
Commits
rHG495fcff10124: bundlerepo: assign bundle attributes in bundle type blocks
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.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

indygreg created this revision.Nov 11 2017, 9:52 PM
dlax accepted this revision.Nov 13 2017, 2:58 AM
dlax added inline comments.Nov 13 2017, 3:05 AM
mercurial/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.

indygreg added inline comments.Nov 13 2017, 12:06 PM
mercurial/bundlerepo.py
298

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.

durin42 accepted this revision.Nov 13 2017, 5:20 PM
This revision is now accepted and ready to land.Nov 13 2017, 5:20 PM
Closed by commit rHG495fcff10124: bundlerepo: assign bundle attributes in bundle type blocks (authored by indygreg). · Explain WhyNov 13 2017, 6:16 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/bundlerepo.py (29 lines)

Diff 3456

mercurial/bundlerepo.py

if repopath: if repopath:
self._url = 'bundle:' + util.expandpath(repopath) + '+' + bundlepath self._url = 'bundle:' + util.expandpath(repopath) + '+' + bundlepath
else: else:
self._url = 'bundle:' + bundlepath self._url = 'bundle:' + bundlepath
self.tempfile = None self.tempfile = None
f = util.posixfile(bundlepath, "rb") f = util.posixfile(bundlepath, "rb")
self._bundlefile = self._bundle = exchange.readbundle(ui, f, bundlepath) bundle = exchange.readbundle(ui, f, bundlepath)
if isinstance(bundle, bundle2.unbundle20):
self._bundlefile = bundle
self._bundle = None
if isinstance(self._bundle, bundle2.unbundle20):
hadchangegroup = False hadchangegroup = False
for part in self._bundle.iterparts(): for part in bundle.iterparts():
if part.type == 'changegroup': if part.type == 'changegroup':
if hadchangegroup: if hadchangegroup:
raise NotImplementedError("can't process " raise NotImplementedError("can't process "
"multiple changegroups") "multiple changegroups")
hadchangegroup = True hadchangegroup = True
self._handlebundle2part(part) self._handlebundle2part(bundle, part)
dlaxUnsubmitted
Not Done

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.

dlax: Within `_handlebundle2part`, there's still a `self._bundle = changegroup.getunbundler(...)`…
indygregAuthorUnsubmitted
Not Done

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.

indygreg: Yes, this API would be cleaner. The reason I hesitated is `_handlebundle2part()` is generic.
if not hadchangegroup: if not hadchangegroup:
raise error.Abort(_("No changegroups found")) raise error.Abort(_("No changegroups found"))
elif isinstance(self._bundle, changegroup.cg1unpacker): elif isinstance(bundle, changegroup.cg1unpacker):
if self._bundle.compressed(): if bundle.compressed():
f = self._writetempbundle(self._bundle.read, '.hg10un', f = self._writetempbundle(bundle.read, '.hg10un',
header='HG10UN') header='HG10UN')
self._bundlefile = self._bundle = exchange.readbundle( bundle = exchange.readbundle(ui, f, bundlepath, self.vfs)
ui, f, bundlepath, self.vfs)
self._bundlefile = bundle
self._bundle = bundle
else: else:
raise error.Abort(_('bundle type %s cannot be read') % raise error.Abort(_('bundle type %s cannot be read') %
type(self._bundle)) type(bundle))
# dict with the mapping 'filename' -> position in the bundle # dict with the mapping 'filename' -> position in the bundle
self.bundlefilespos = {} self.bundlefilespos = {}
self.firstnewrev = self.changelog.repotiprev + 1 self.firstnewrev = self.changelog.repotiprev + 1
phases.retractboundary(self, None, phases.draft, phases.retractboundary(self, None, phases.draft,
[ctx.node() for ctx in self[self.firstnewrev:]]) [ctx.node() for ctx in self[self.firstnewrev:]])
def _handlebundle2part(self, part): def _handlebundle2part(self, bundle, part):
if part.type == 'changegroup': if part.type == 'changegroup':
cgstream = part cgstream = part
version = part.params.get('version', '01') version = part.params.get('version', '01')
legalcgvers = changegroup.supportedincomingversions(self) legalcgvers = changegroup.supportedincomingversions(self)
if version not in legalcgvers: if version not in legalcgvers:
msg = _('Unsupported changegroup version: %s') msg = _('Unsupported changegroup version: %s')
raise error.Abort(msg % version) raise error.Abort(msg % version)
if self._bundle.compressed(): if bundle.compressed():
cgstream = self._writetempbundle(part.read, cgstream = self._writetempbundle(part.read,
".cg%sun" % version) ".cg%sun" % version)
self._bundle = changegroup.getunbundler(version, cgstream, 'UN') self._bundle = changegroup.getunbundler(version, cgstream, 'UN')
def _writetempbundle(self, readfn, suffix, header=''): def _writetempbundle(self, readfn, suffix, header=''):
"""Write a temporary file to disk """Write a temporary file to disk
""" """