This is an archive of the discontinued Mercurial Phabricator instance.

bundlerepo: assign bundle attributes in bundle type blocks
ClosedPublic

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

Details

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
Lint Skipped
Unit
Unit Tests Skipped

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
This revision was automatically updated to reflect the committed changes.