Patchwork util: refactor compression engine capabilities declaration

login
register
mail settings
Submitter Gregory Szorc
Date Jan. 18, 2017, 8:48 a.m.
Message ID <016baf3f6299e23b98c2.1484729284@gps-mbp.local>
Download mbox | patch
Permalink /patch/18243/
State Deferred
Headers show

Comments

Gregory Szorc - Jan. 18, 2017, 8:48 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@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.
Augie Fackler - Jan. 18, 2017, 4:07 p.m.
On Wed, Jan 18, 2017 at 12:48:04AM -0800, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1484729224 28800
> #      Wed Jan 18 00:47:04 2017 -0800
> # Node ID 016baf3f6299e23b98c205b6506bb6203dba8fd6
> # Parent  923336cf8b8afdb41746ecef8a39d773bd5538bf
> util: refactor compression engine capabilities declaration

This doesn't really thrill me, but it's all internal code so I'm not
going to worry about wether it lands for 4.1 or not. Maybe other
people have opinions?

(Don't want to take the time now to think about this API problem and
get a better idea, but my knee-jerk reaction is a namedtuple instead
of a dict, without looking at the circumstances in detail.)

>
> 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
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

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