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

login
register
mail settings
Submitter Pierre-Yves David
Date April 7, 2015, 6:30 a.m.
Message ID <37b1e89e2ffcbd8a8851.1428388253@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/8528/
State Accepted
Commit 60fecc5b14a45de49af875087c0a0e3b2820c7af
Headers show

Comments

Pierre-Yves David - April 7, 2015, 6:30 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1428361473 25200
#      Mon Apr 06 16:04:33 2015 -0700
# Node ID 37b1e89e2ffcbd8a8851cb7c84404d47c6dc8e56
# Parent  3aea190962e2c2286d70a251b375957e1ab60c01
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.
Martin von Zweigbergk - April 7, 2015, 3 p.m.
On Mon, Apr 6, 2015 at 11:32 PM Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1428361473 25200
> #      Mon Apr 06 16:04:33 2015 -0700
> # Node ID 37b1e89e2ffcbd8a8851cb7c84404d47c6dc8e56
> # Parent  3aea190962e2c2286d70a251b375957e1ab60c01
> 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.


>      else:
>          raise util.Abort(_('%s: unknown bundle version %s') % (fname,
> version))
>
>  def buildobsmarkerspart(bundler, markers):
>      """add an obsmarker part to the bundler with <markers>
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -112,11 +112,11 @@ class localpeer(peer.peerrepository):
>                                  common=common, bundlecaps=bundlecaps,
> **kwargs)
>          if bundlecaps is not None and 'HG2Y' in bundlecaps:
>              # When requesting a bundle2, getbundle returns a stream to
> make the
>              # wire level function happier. We need to build a proper
> object
>              # from it in local peer.
> -            cg = bundle2.unbundle20(self.ui, cg)
> +            cg = bundle2.getunbundler(self.ui, cg)
>          return cg
>
>      # TODO We might want to move the next two calls into legacypeer and
> add
>      # unbundle instead.
>
> @@ -130,11 +130,11 @@ class localpeer(peer.peerrepository):
>              if util.safehasattr(ret, 'getchunks'):
>                  # This is a bundle20 object, turn it into an unbundler.
>                  # This little dance should be dropped eventually when the
> API
>                  # is finally improved.
>                  stream = util.chunkbuffer(ret.getchunks())
> -                ret = bundle2.unbundle20(self.ui, stream)
> +                ret = bundle2.getunbundler(self.ui, stream)
>              return ret
>          except error.PushRaced, exc:
>              raise error.ResponseError(_('push failed:'), str(exc))
>
>      def lock(self):
> diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py
> --- a/mercurial/wireproto.py
> +++ b/mercurial/wireproto.py
> @@ -362,11 +362,11 @@ class wirepeer(peer.peerrepository):
>                                 % keytype)
>              opts[key] = value
>          f = self._callcompressable("getbundle", **opts)
>          bundlecaps = kwargs.get('bundlecaps')
>          if bundlecaps is not None and 'HG2Y' in bundlecaps:
> -            return bundle2.unbundle20(self.ui, f)
> +            return bundle2.getunbundler(self.ui, f)
>          else:
>              return changegroupmod.cg1unpacker(f, 'UN')
>
>      def unbundle(self, cg, heads, source):
>          '''Send cg (a readable file-like object representing the
> @@ -399,11 +399,11 @@ class wirepeer(peer.peerrepository):
>              for l in output.splitlines(True):
>                  self.ui.status(_('remote: '), l)
>          else:
>              # bundle2 push. Send a stream, fetch a stream.
>              stream = self._calltwowaystream('unbundle', cg, heads=heads)
> -            ret = bundle2.unbundle20(self.ui, stream)
> +            ret = bundle2.getunbundler(self.ui, stream)
>          return ret
>
>      def debugwireargs(self, one, two, three=None, four=None, five=None):
>          # don't pass optional arguments left at their default value
>          opts = {}
> diff --git a/tests/test-bundle2-format.t b/tests/test-bundle2-format.t
> --- a/tests/test-bundle2-format.t
> +++ b/tests/test-bundle2-format.t
> @@ -155,11 +155,11 @@ Create an extension to test bundle2 API
>    >     try:
>    >         tr = None
>    >         lock = repo.lock()
>    >         tr = repo.transaction('processbundle')
>    >         try:
> -  >             unbundler = bundle2.unbundle20(ui, sys.stdin)
> +  >             unbundler = bundle2.getunbundler(ui, sys.stdin)
>    >             op = bundle2.processbundle(repo, unbundler, lambda: tr)
>    >             tr.close()
>    >         except error.BundleValueError, exc:
>    >             raise util.Abort('missing support for %s' % exc)
>    >         except error.PushRaced, exc:
> @@ -181,11 +181,11 @@ Create an extension to test bundle2 API
>    >             file.write(chunk)
>    >
>    > @command('statbundle2', [], '')
>    > def cmdstatbundle2(ui, repo):
>    >     """print statistic on the bundle2 container read from stdin"""
> -  >     unbundler = bundle2.unbundle20(ui, sys.stdin)
> +  >     unbundler = bundle2.getunbundler(ui, sys.stdin)
>    >     try:
>    >         params = unbundler.params
>    >     except error.BundleValueError, exc:
>    >        raise util.Abort('unknown parameters: %s' % exc)
>    >     ui.write('options count: %i\n' % len(params))
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
Pierre-Yves David - April 7, 2015, 6:05 p.m.
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@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
> wrote:
>
>     # HG changeset patch
>     # User Pierre-Yves David <pierre-yves.david@fb.com
>     <mailto:pierre-yves.david@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?
Martin von Zweigbergk - April 7, 2015, 8:25 p.m.
On Tue, Apr 7, 2015 at 11:06 AM Pierre-Yves David <
pierre-yves.david@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@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
> > wrote:
> >
> >     # HG changeset patch
> >     # User Pierre-Yves David <pierre-yves.david@fb.com
> >     <mailto:pierre-yves.david@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
>

Patch

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)
     else:
         raise util.Abort(_('%s: unknown bundle version %s') % (fname, version))
 
 def buildobsmarkerspart(bundler, markers):
     """add an obsmarker part to the bundler with <markers>
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -112,11 +112,11 @@  class localpeer(peer.peerrepository):
                                 common=common, bundlecaps=bundlecaps, **kwargs)
         if bundlecaps is not None and 'HG2Y' in bundlecaps:
             # When requesting a bundle2, getbundle returns a stream to make the
             # wire level function happier. We need to build a proper object
             # from it in local peer.
-            cg = bundle2.unbundle20(self.ui, cg)
+            cg = bundle2.getunbundler(self.ui, cg)
         return cg
 
     # TODO We might want to move the next two calls into legacypeer and add
     # unbundle instead.
 
@@ -130,11 +130,11 @@  class localpeer(peer.peerrepository):
             if util.safehasattr(ret, 'getchunks'):
                 # This is a bundle20 object, turn it into an unbundler.
                 # This little dance should be dropped eventually when the API
                 # is finally improved.
                 stream = util.chunkbuffer(ret.getchunks())
-                ret = bundle2.unbundle20(self.ui, stream)
+                ret = bundle2.getunbundler(self.ui, stream)
             return ret
         except error.PushRaced, exc:
             raise error.ResponseError(_('push failed:'), str(exc))
 
     def lock(self):
diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py
--- a/mercurial/wireproto.py
+++ b/mercurial/wireproto.py
@@ -362,11 +362,11 @@  class wirepeer(peer.peerrepository):
                                % keytype)
             opts[key] = value
         f = self._callcompressable("getbundle", **opts)
         bundlecaps = kwargs.get('bundlecaps')
         if bundlecaps is not None and 'HG2Y' in bundlecaps:
-            return bundle2.unbundle20(self.ui, f)
+            return bundle2.getunbundler(self.ui, f)
         else:
             return changegroupmod.cg1unpacker(f, 'UN')
 
     def unbundle(self, cg, heads, source):
         '''Send cg (a readable file-like object representing the
@@ -399,11 +399,11 @@  class wirepeer(peer.peerrepository):
             for l in output.splitlines(True):
                 self.ui.status(_('remote: '), l)
         else:
             # bundle2 push. Send a stream, fetch a stream.
             stream = self._calltwowaystream('unbundle', cg, heads=heads)
-            ret = bundle2.unbundle20(self.ui, stream)
+            ret = bundle2.getunbundler(self.ui, stream)
         return ret
 
     def debugwireargs(self, one, two, three=None, four=None, five=None):
         # don't pass optional arguments left at their default value
         opts = {}
diff --git a/tests/test-bundle2-format.t b/tests/test-bundle2-format.t
--- a/tests/test-bundle2-format.t
+++ b/tests/test-bundle2-format.t
@@ -155,11 +155,11 @@  Create an extension to test bundle2 API
   >     try:
   >         tr = None
   >         lock = repo.lock()
   >         tr = repo.transaction('processbundle')
   >         try:
-  >             unbundler = bundle2.unbundle20(ui, sys.stdin)
+  >             unbundler = bundle2.getunbundler(ui, sys.stdin)
   >             op = bundle2.processbundle(repo, unbundler, lambda: tr)
   >             tr.close()
   >         except error.BundleValueError, exc:
   >             raise util.Abort('missing support for %s' % exc)
   >         except error.PushRaced, exc:
@@ -181,11 +181,11 @@  Create an extension to test bundle2 API
   >             file.write(chunk)
   > 
   > @command('statbundle2', [], '')
   > def cmdstatbundle2(ui, repo):
   >     """print statistic on the bundle2 container read from stdin"""
-  >     unbundler = bundle2.unbundle20(ui, sys.stdin)
+  >     unbundler = bundle2.getunbundler(ui, sys.stdin)
   >     try:
   >         params = unbundler.params
   >     except error.BundleValueError, exc:
   >        raise util.Abort('unknown parameters: %s' % exc)
   >     ui.write('options count: %i\n' % len(params))