Patchwork [03,of,11] bundle2: use new compression engine API for compression

login
register
mail settings
Submitter Gregory Szorc
Date Nov. 2, 2016, 12:08 a.m.
Message ID <5ebb7422896689850acb.1478045316@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/17246/
State Superseded
Headers show

Comments

Gregory Szorc - Nov. 2, 2016, 12:08 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1477157519 25200
#      Sat Oct 22 10:31:59 2016 -0700
# Node ID 5ebb7422896689850acba2017f61c182050956d0
# Parent  4015d575d311cd7ebc923d1320e55a76c655c485
bundle2: use new compression engine API for compression

Now that we have a new API to define compression engines, let's put it
to use!

The new code stores a reference to the compression engine instead of
a low-level compressor object. This will allow us to use alternate
compression APIs in the future if we ever need to. For example, if
the engine exposed an API to compress a generator of chunks, we could
use that.

As part of this, we change the registration in bundletypes to use 'UN'
instead of None. Previously, util.compressors had the no-op compressor
registered under both the 'UN' and None keys. Since we're switching to
a new API, I don't see the point in carrying this dual registration
forward.
Pierre-Yves David - Nov. 7, 2016, 1:59 p.m.
On 11/02/2016 01:08 AM, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1477157519 25200
> #      Sat Oct 22 10:31:59 2016 -0700
> # Node ID 5ebb7422896689850acba2017f61c182050956d0
> # Parent  4015d575d311cd7ebc923d1320e55a76c655c485
> bundle2: use new compression engine API for compression
>
> Now that we have a new API to define compression engines, let's put it
> to use!
>
> The new code stores a reference to the compression engine instead of
> a low-level compressor object. This will allow us to use alternate
> compression APIs in the future if we ever need to. For example, if
> the engine exposed an API to compress a generator of chunks, we could
> use that.
>
> As part of this, we change the registration in bundletypes to use 'UN'
> instead of None. Previously, util.compressors had the no-op compressor
> registered under both the 'UN' and None keys. Since we're switching to
> a new API, I don't see the point in carrying this dual registration
> forward.

This change looks good to me (but is being patch 2 which has feedback).

However this patch raise an extra feedback about the previous patches, 
see below.

> […]
> @@ -506,25 +506,25 @@ class bundle20(object):
>
>      _magicstring = 'HG20'
>
>      def __init__(self, ui, capabilities=()):
>          self.ui = ui
>          self._params = []
>          self._parts = []
>          self.capabilities = dict(capabilities)
> -        self._compressor = util.compressors[None]()
> +        self._compengine = util.compressionengines.forbundletype('UN')

"util.compressionengines" is bit long, we could find a name a bit more 
compact?

Cheers,

Patch

diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
--- a/mercurial/bundle2.py
+++ b/mercurial/bundle2.py
@@ -480,21 +480,21 @@  def encodecaps(caps):
         ca = urlreq.quote(ca)
         vals = [urlreq.quote(v) for v in vals]
         if vals:
             ca = "%s=%s" % (ca, ','.join(vals))
         chunks.append(ca)
     return '\n'.join(chunks)
 
 bundletypes = {
-    "": ("", None),       # only when using unbundle on ssh and old http servers
+    "": ("", 'UN'),       # only when using unbundle on ssh and old http servers
                           # since the unification ssh accepts a header but there
                           # is no capability signaling it.
     "HG20": (), # special-cased below
-    "HG10UN": ("HG10UN", None),
+    "HG10UN": ("HG10UN", 'UN'),
     "HG10BZ": ("HG10", 'BZ'),
     "HG10GZ": ("HG10GZ", 'GZ'),
 }
 
 # hgweb uses this list to communicate its preferred type
 bundlepriority = ['HG10GZ', 'HG10BZ', 'HG10UN']
 
 class bundle20(object):
@@ -506,25 +506,25 @@  class bundle20(object):
 
     _magicstring = 'HG20'
 
     def __init__(self, ui, capabilities=()):
         self.ui = ui
         self._params = []
         self._parts = []
         self.capabilities = dict(capabilities)
-        self._compressor = util.compressors[None]()
+        self._compengine = util.compressionengines.forbundletype('UN')
 
     def setcompression(self, alg):
         """setup core part compression to <alg>"""
         if alg is None:
             return
         assert not any(n.lower() == 'Compression' for n, v in self._params)
         self.addparam('Compression', alg)
-        self._compressor = util.compressors[alg]()
+        self._compengine = util.compressionengines.forbundletype(alg)
 
     @property
     def nbparts(self):
         """total number of parts added to the bundler"""
         return len(self._parts)
 
     # methods used to defines the bundle2 content
     def addparam(self, name, value=None):
@@ -567,21 +567,22 @@  class bundle20(object):
         outdebug(self.ui, 'start emission of %s stream' % self._magicstring)
         yield self._magicstring
         param = self._paramchunk()
         outdebug(self.ui, 'bundle parameter: %s' % param)
         yield _pack(_fstreamparamsize, len(param))
         if param:
             yield param
         # starting compression
+        compressor = self._compengine.compressorobj()
         for chunk in self._getcorechunk():
-            data = self._compressor.compress(chunk)
+            data = compressor.compress(chunk)
             if data:
                 yield data
-        yield self._compressor.flush()
+        yield compressor.flush()
 
     def _paramchunk(self):
         """return a encoded version of all stream parameters"""
         blocks = []
         for par, value in self._params:
             par = urlreq.quote(par)
             if value is not None:
                 value = urlreq.quote(value)
@@ -1313,28 +1314,29 @@  def writebundle(ui, cg, filename, bundle
         chunkiter = bundle.getchunks()
     else:
         # compression argument is only for the bundle2 case
         assert compression is None
         if cg.version != '01':
             raise error.Abort(_('old bundle types only supports v1 '
                                 'changegroups'))
         header, comp = bundletypes[bundletype]
-        if comp not in util.compressors:
+        if comp not in util.compressionengines.supportedbundletypes:
             raise error.Abort(_('unknown stream compression type: %s')
                               % comp)
-        z = util.compressors[comp]()
+        compengine = util.compressionengines.forbundletype(comp)
+        compressor = compengine.compressorobj()
         subchunkiter = cg.getchunks()
         def chunkiter():
             yield header
             for chunk in subchunkiter:
-                data = z.compress(chunk)
+                data = compressor.compress(chunk)
                 if data:
                     yield data
-            yield z.flush()
+            yield compressor.flush()
         chunkiter = chunkiter()
 
     # parse the changegroup data, otherwise we will block
     # in case of sshrepo because we don't know the end of the stream
     return changegroup.writechunks(ui, chunkiter, filename, vfs=vfs)
 
 @parthandler('changegroup', ('version', 'nbchanges', 'treemanifest'))
 def handlechangegroup(op, inpart):