[PATCH] util: refactor compression engine capabilities declaration

Gregory Szorc gregory.szorc at gmail.com
Wed Jan 18 08:48:04 UTC 2017


# HG changeset patch
# User Gregory Szorc <gregory.szorc at gmail.com>
# Date 1484729224 28800
#      Wed Jan 18 00:47:04 2017 -0800
# Node ID 016baf3f6299e23b98c205b6506bb6203dba8fd6
# Parent  923336cf8b8afdb41746ecef8a39d773bd5538bf
util: refactor compression engine capabilities declaration

I was asked to refactor the various methods for declaring compression
engine capabilities to be less abusive of methods. I did so by
consolidating everything into a single method returning a dict.

Having written this, I'm not convinced it is objectively better. I'll
let a reviewer decide whether to accept, reject flat out, or push
in a different direction, such as using attributes to declare
capabilities.

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -1962,9 +1962,9 @@ def debuginstall(ui, **opts):
     wirecompengines = util.compengines.supportedwireengines(util.SERVERROLE)
     fm.write('compenginesserver', _('checking available compression engines '
                                     'for wire protocol (%s)\n'),
              fm.formatlist([e.name() for e in wirecompengines
-                            if e.wireprotosupport()],
+                            if e.capabilities().get('wireprotoname')],
                            name='compengine', fmt='%s', sep=', '))
 
     # templates
     p = templater.templatepaths()
diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -151,9 +151,9 @@ def parsebundlespec(repo, spec, strict=T
                       ', '.join(sorted(missingreqs)))
 
     if not externalnames:
         engine = util.compengines.forbundlename(compression)
-        compression = engine.bundletype()[1]
+        compression = engine.capabilities()['bundleid']
         version = _bundlespeccgversions[version]
     return compression, version, params
 
 def readbundle(ui, fh, fname, vfs=None):
@@ -191,9 +191,10 @@ def getbundlespec(ui, fh):
     restored.
     """
     def speccompression(alg):
         try:
-            return util.compengines.forbundletype(alg).bundletype()[0]
+            engine = util.compengines.forbundletype(alg)
+            return engine.capabilities()['bundlename']
         except KeyError:
             return None
 
     b = readbundle(ui, fh, None)
diff --git a/mercurial/hgweb/protocol.py b/mercurial/hgweb/protocol.py
--- a/mercurial/hgweb/protocol.py
+++ b/mercurial/hgweb/protocol.py
@@ -119,9 +119,9 @@ class webproto(wireproto.abstractserverp
 
             # Now find an agreed upon compression format.
             for engine in wireproto.supportedcompengines(self.ui, self,
                                                          util.SERVERROLE):
-                if engine.wireprotosupport().name in compformats:
+                if engine.capabilities()['wireprotoname'] in compformats:
                     opts = {}
                     level = self.ui.configint('server',
                                               '%slevel' % engine.name())
                     if level is not None:
@@ -146,9 +146,9 @@ def call(repo, req, cmd):
 
     def genversion2(gen, compress, engine, engineopts):
         # application/mercurial-0.2 always sends a payload header
         # identifying the compression engine.
-        name = engine.wireprotosupport().name
+        name = engine.capabilities()['wireprotoname']
         assert 0 < len(name) < 256
         yield struct.pack('B', len(name))
         yield name
 
diff --git a/mercurial/httppeer.py b/mercurial/httppeer.py
--- a/mercurial/httppeer.py
+++ b/mercurial/httppeer.py
@@ -193,9 +193,9 @@ class httppeer(wireproto.wirepeer):
         if '0.2tx' in mediatypes and self.capable('compression'):
             # We /could/ compare supported compression formats and prune
             # non-mutually supported or error if nothing is mutually supported.
             # For now, send the full list to the server and have it error.
-            comps = [e.wireprotosupport().name for e in
+            comps = [e.capabilities()['wireprotoname'] for e in
                      util.compengines.supportedwireengines(util.CLIENTROLE)]
             protoparams.append('comp=%s' % ','.join(comps))
 
         if protoparams:
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -286,9 +286,9 @@ class localrepository(object):
 
         # Add compression engines.
         for name in util.compengines:
             engine = util.compengines[name]
-            if engine.revlogheader():
+            if engine.capabilities().get('revlogheader'):
                 self.supported.add('exp-compression-%s' % name)
 
         if not self.vfs.isdir():
             if create:
diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -2983,12 +2983,8 @@ class ctxmanager(object):
 
 SERVERROLE = 'server'
 CLIENTROLE = 'client'
 
-compewireprotosupport = collections.namedtuple(u'compenginewireprotosupport',
-                                               (u'name', u'serverpriority',
-                                                u'clientpriority'))
-
 class compressormanager(object):
     """Holds registrations of various compression engines.
 
     This class essentially abstracts the differences between compression
@@ -3031,41 +3027,42 @@ class compressormanager(object):
         if name in self._engines:
             raise error.Abort(_('compression engine %s already registered') %
                               name)
 
-        bundleinfo = engine.bundletype()
-        if bundleinfo:
-            bundlename, bundletype = bundleinfo
-
-            if bundlename in self._bundlenames:
+        caps = engine.capabilities()
+
+        bundlename = caps.get('bundlename')
+        bundletype = caps.get('bundleid')
+        if bundletype:
+            if bundlename and bundlename in self._bundlenames:
                 raise error.Abort(_('bundle name %s already registered') %
                                   bundlename)
             if bundletype in self._bundletypes:
                 raise error.Abort(_('bundle type %s already registered by %s') %
                                   (bundletype, self._bundletypes[bundletype]))
 
-            # No external facing name declared.
             if bundlename:
                 self._bundlenames[bundlename] = name
 
             self._bundletypes[bundletype] = name
 
-        wiresupport = engine.wireprotosupport()
-        if wiresupport:
-            wiretype = wiresupport.name
+        wiretype = caps.get('wireprotoname')
+        if wiretype:
             if wiretype in self._wiretypes:
                 raise error.Abort(_('wire protocol compression %s already '
                                     'registered by %s') %
                                   (wiretype, self._wiretypes[wiretype]))
 
             self._wiretypes[wiretype] = name
 
-        revlogheader = engine.revlogheader()
-        if revlogheader and revlogheader in self._revlogheaders:
-            raise error.Abort(_('revlog header %s already registered by %s') %
-                              (revlogheader, self._revlogheaders[revlogheader]))
-
+        revlogheader = caps.get('revlogheader')
         if revlogheader:
+            if revlogheader in self._revlogheaders:
+                raise error.Abort(_('revlog header %s already registered by '
+                                    '%s') % (
+                                  revlogheader,
+                                  self._revlogheaders[revlogheader]))
+
             self._revlogheaders[revlogheader] = name
 
         self._engines[name] = engine
 
@@ -3112,9 +3109,10 @@ class compressormanager(object):
         loaded.
         """
         assert role in (SERVERROLE, CLIENTROLE)
 
-        attr = 'serverpriority' if role == SERVERROLE else 'clientpriority'
+        key = 'serverpriority' if role == SERVERROLE else 'clientpriority'
+        key = 'wireproto%s' % key
 
         engines = [self._engines[e] for e in self._wiretypes.values()]
         if onlyavailable:
             engines = [e for e in engines if e.available()]
@@ -3122,10 +3120,10 @@ class compressormanager(object):
         def getkey(e):
             # Sort first by priority, highest first. In case of tie, sort
             # alphabetically. This is arbitrary, but ensures output is
             # stable.
-            w = e.wireprotosupport()
-            return -1 * getattr(w, attr), w.name
+            caps = e.capabilities()
+            return -1 * caps[key], caps['wireprotoname']
 
         return list(sorted(engines, key=getkey))
 
     def forwiretype(self, wiretype):
@@ -3166,57 +3164,49 @@ class compressionengine(object):
         on C extensions that may not be present).
         """
         return True
 
-    def bundletype(self):
-        """Describes bundle identifiers for this engine.
-
-        If this compression engine isn't supported for bundles, returns None.
-
-        If this engine can be used for bundles, returns a 2-tuple of strings of
-        the user-facing "bundle spec" compression name and an internal
-        identifier used to denote the compression format within bundles. To
-        exclude the name from external usage, set the first element to ``None``.
-
-        If bundle compression is supported, the class must also implement
-        ``compressstream`` and `decompressorreader``.
+    def capabilities(self):
+        """Describe the capabilities and properties of this engine.
+
+        Returns a dict describing the engine in more detail. This dict is
+        consumed at engine registration time to make the engines manager aware
+        of all engines and their abilities.
+
+        The returned dict may have the following keys:
+
+        bundlename
+           String used to identify this engine in the user-facing "bundle
+           specification" string (as used by `hg bundle`). Omit to not register
+           this engine externally.
+
+        bundleid
+           String used to identify this compression format in bundles. Omit
+           to not expose this engine to bundles.
+
+        wireprotoname
+           String that identifies this compression engine in the wire protocol.
+
+        wireprotoserverpriority
+           Integer priority to advertise this engine at on the server.
+           Highest value is most preferred. The priority values are somewhat
+           arbitrary and are only used for default ordering.
+
+        wireprotoclientpriority
+           Integer priority to advertise this engine at on the client.
+
+        revlogheader
+           Byte sequence used to identify chunks in revlogs as compressed with
+           this engine. If this is not defined, the engine does not support
+           revlog compression.
+
+        If ``bundlename`` or ``bundleid`` are set, the class must implement
+        ``compressstream`` and ``decompressorreader``.
+
+        If ``wireprotoname`` is set, the class must implement ``compressstream``
+        and ``decompressorreader``.
         """
-        return None
-
-    def wireprotosupport(self):
-        """Declare support for this compression format on the wire protocol.
-
-        If this compression engine isn't supported for compressing wire
-        protocol payloads, returns None.
-
-        Otherwise, returns ``compenginewireprotosupport`` with the following
-        fields:
-
-        * String format identifier
-        * Integer priority for the server
-        * Integer priority for the client
-
-        The integer priorities are used to order the advertisement of format
-        support by server and client. The highest integer is advertised
-        first. Integers with non-positive values aren't advertised.
-
-        The priority values are somewhat arbitrary and only used for default
-        ordering. The relative order can be changed via config options.
-
-        If wire protocol compression is supported, the class must also implement
-        ``compressstream`` and ``decompressorreader``.
-        """
-        return None
-
-    def revlogheader(self):
-        """Header added to revlog chunks that identifies this engine.
-
-        If this engine can be used to compress revlogs, this method should
-        return the bytes used to identify chunks compressed with this engine.
-        Else, the method should return ``None`` to indicate it does not
-        participate in revlog compression.
-        """
-        return None
+        return {}
 
     def compressstream(self, it, opts=None):
         """Compress an iterator of chunks.
 
@@ -3261,16 +3251,17 @@ class compressionengine(object):
 class _zlibengine(compressionengine):
     def name(self):
         return 'zlib'
 
-    def bundletype(self):
-        return 'gzip', 'GZ'
-
-    def wireprotosupport(self):
-        return compewireprotosupport('zlib', 20, 20)
-
-    def revlogheader(self):
-        return 'x'
+    def capabilities(self):
+        return {
+            'bundlename': 'gzip',
+            'bundleid': 'GZ',
+            'wireprotoname': 'zlib',
+            'wireprotoserverpriority': 20,
+            'wireprotoclientpriority': 20,
+            'revlogheader': 'x',
+        }
 
     def compressstream(self, it, opts=None):
         opts = opts or {}
 
@@ -3342,15 +3333,18 @@ compengines.register(_zlibengine())
 class _bz2engine(compressionengine):
     def name(self):
         return 'bz2'
 
-    def bundletype(self):
-        return 'bzip2', 'BZ'
-
-    # We declare a protocol name but don't advertise by default because
-    # it is slow.
-    def wireprotosupport(self):
-        return compewireprotosupport('bzip2', 0, 0)
+    def capabilities(self):
+        return {
+            'bundlename': 'bzip2',
+            'bundleid': 'BZ',
+            # We declare a protocol name but don't advertise by default
+            # because it is slow.
+            'wireprotoname': 'bzip2',
+            'wireprotoserverpriority': 0,
+            'wireprotoclientpriority': 0,
+        }
 
     def compressstream(self, it, opts=None):
         opts = opts or {}
         z = bz2.BZ2Compressor(opts.get('level', 9))
@@ -3374,10 +3368,12 @@ compengines.register(_bz2engine())
 class _truncatedbz2engine(compressionengine):
     def name(self):
         return 'bz2truncated'
 
-    def bundletype(self):
-        return None, '_truncatedBZ'
+    def capabilities(self):
+        return {
+            'bundleid': '_truncatedBZ',
+        }
 
     # We don't implement compressstream because it is hackily handled elsewhere.
 
     def decompressorreader(self, fh):
@@ -3395,19 +3391,21 @@ compengines.register(_truncatedbz2engine
 class _noopengine(compressionengine):
     def name(self):
         return 'none'
 
-    def bundletype(self):
-        return 'none', 'UN'
-
-    # Clients always support uncompressed payloads. Servers don't because
-    # unless you are on a fast network, uncompressed payloads can easily
-    # saturate your network pipe.
-    def wireprotosupport(self):
-        return compewireprotosupport('none', 0, 10)
-
-    # We don't implement revlogheader because it is handled specially
-    # in the revlog class.
+    def capabilities(self):
+        return {
+            'bundlename': 'none',
+            'bundleid': 'UN',
+            'wireprotoname': 'none',
+            # Clients always support uncompressed payloads. Servers don't
+            # because unless you are on a fast network, uncompressed
+            # payloads can easily saturate your network pipe.
+            'wireprotoserverpriority': 0,
+            'wireprotoclientpriority': 10,
+            # We don't declare the revlog header because it is handled
+            # specially in the revlog class.
+        }
 
     def compressstream(self, it, opts=None):
         return it
 
@@ -3441,16 +3439,17 @@ class _zstdengine(compressionengine):
 
     def available(self):
         return bool(self._module)
 
-    def bundletype(self):
-        return 'zstd', 'ZS'
-
-    def wireprotosupport(self):
-        return compewireprotosupport('zstd', 50, 50)
-
-    def revlogheader(self):
-        return '\x28'
+    def capabilities(self):
+        return {
+            'bundlename': 'zstd',
+            'bundleid': 'ZS',
+            'wireprotoname': 'zstd',
+            'wireprotoserverpriority': 50,
+            'wireprotoclientpriority': 50,
+            'revlogheader': '\x28',
+        }
 
     def compressstream(self, it, opts=None):
         opts = opts or {}
         # zstd level 3 is almost always significantly faster than zlib
diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py
--- a/mercurial/wireproto.py
+++ b/mercurial/wireproto.py
@@ -629,11 +629,12 @@ def supportedcompengines(ui, proto, role
 
     # No explicit config. Filter out the ones that aren't supposed to be
     # advertised and return default ordering.
     if not configengines:
-        attr = 'serverpriority' if role == util.SERVERROLE else 'clientpriority'
+        key = 'serverpriority' if role == util.SERVERROLE else 'clientpriority'
+        key = 'wireproto%s' % key
         return [e for e in compengines
-                if getattr(e.wireprotosupport(), attr) > 0]
+                if e.capabilities().get(key, 0) > 0]
 
     # If compression engines are listed in the config, assume there is a good
     # reason for it (like server operators wanting to achieve specific
     # performance characteristics). So fail fast if the config references
@@ -777,9 +778,9 @@ def _capabilities(repo, proto):
         caps.append('httpmediatype=0.1rx,0.1tx,0.2tx')
 
         compengines = supportedcompengines(repo.ui, proto, util.SERVERROLE)
         if compengines:
-            comptypes = ','.join(urlreq.quote(e.wireprotosupport().name)
+            comptypes = ','.join(urlreq.quote(e.capabilities()['wireprotoname'])
                                  for e in compengines)
             caps.append('compression=%s' % comptypes)
 
     return caps


More information about the Mercurial-devel mailing list