[PATCH 2 of 3] unbundle20: retrieve unbundler instances through a factory function

[Martin von Zweigbergk]

On Tue, Apr 7, 2015 at 11:06 AM Pierre-Yves David <
pierre-yves.david at ens-lyon.org> wrote:

>
>
> On 04/07/2015 08:00 AM, Martin von Zweigbergk wrote:
> >
> >
> > On Mon, Apr 6, 2015 at 11:32 PM Pierre-Yves David
> > <pierre-yves.david at ens-lyon.org <mailto:pierre-yves.david at ens-lyon.org>>
> > wrote:
> >
> >     # HG changeset patch
> >     # User Pierre-Yves David <pierre-yves.david at fb.com
> >     <mailto:pierre-yves.david at fb.com>>
> >     # Date 1428361473 25200
> >     #      Mon Apr 06 16:04:33 2015 -0700
> >     # Node ID 37b1e89e2ffcbd8a8851cb7c84404d__47c6dc8e56
> >     # Parent  3aea190962e2c2286d70a251b37595__7e1ab60c01
> >     unbundle20: retrieve unbundler instances through a factory function
> >
> >     To support multiple bundle2 formats, we will need a function
> >     returning the
> >     proper unbundler according to the header. We introduce such function
> >     and change
> >     the usage in the code base. The function will get smarter in later
> >     changesets.
> >
> >     This is somewhat similar to the dispatching we do for 'HG10' and
> 'HG11'.
> >
> >     The main target is to allow HG2Y support in an extension to ease
> >     transition of
> >     companies using the experimental protocol in production (yeah...)
> >     But I've no
> >     doubt this will be useful when playing with a future HG21.
> >
> >     diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
> >     --- a/mercurial/bundle2.py
> >     +++ b/mercurial/bundle2.py
> >     @@ -519,10 +519,14 @@ class unpackermixin(object):
> >           def close(self):
> >               """close underlying file"""
> >               if util.safehasattr(self._fp, 'close'):
> >                   return self._fp.close()
> >
> >     +def getunbundler(ui, fp, header=None):
> >     +    """return a valid unbundler object for a given header"""
> >     +    return unbundle20(ui, fp, header)
> >     +
> >       class unbundle20(unpackermixin):
> >           """interpret a bundle2 stream
> >
> >           This class is fed with a binary stream and yields parts
> >     through its
> >           `iterparts` methods."""
> >     diff --git a/mercurial/exchange.py b/mercurial/exchange.py
> >     --- a/mercurial/exchange.py
> >     +++ b/mercurial/exchange.py
> >     @@ -31,11 +31,11 @@ def readbundle(ui, fh, fname, vfs=None):
> >           if version == '10':
> >               if alg is None:
> >                   alg = changegroup.readexactly(fh, 2)
> >               return changegroup.cg1unpacker(fh, alg)
> >           elif version == '2Y':
> >     -        return bundle2.unbundle20(ui, fh, header=magic + version)
> >     +        return bundle2.getunbundler(ui, fh, header=magic + version)
> >
> >
> > So "header" here is what's called "magicstring" in patch 1/3. Would it
> > makes, as a followup, to rename "magicstring" to "header" in that
> > context too? Sure, it's a small thing, but it confused me.
>
> It is called "magicstring" in bundle20 and "header" in unbundle20. This
> is not very consistent and we should probably fix it. However, Lets
> stick as close as possible from the original code for these code
> movement patches. We'll rename the variable in an independent patch.
> Does this make sense to you?
>

Yep, makes sense (and I did say "as a followup").

These patches (all three) look good to me. I'll push them to the
clowncopter.


>
> --
> Pierre-Yves David
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20150407/61fa2298/attachment.html>