[PATCH 1 of 2] changegroup: hide packermap behind methods

Gregory Szorc gregory.szorc at gmail.com
Wed Jan 13 00:27:43 CST 2016


On Tue, Jan 12, 2016 at 10:06 PM, Martin von Zweigbergk <
martinvonz at google.com> wrote:

> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz at google.com>
> # Date 1452661266 28800
> #      Tue Jan 12 21:01:06 2016 -0800
> # Node ID 3be4c174944f1e0e9e3b0d414a90a983d018ea9d
> # Parent  edd2615ad226c14f6904fc1738c3dc36431db223
> changegroup: hide packermap behind methods
>

This series LGTM. I only glanced at the test changes. I assume most of it
is benign. I approve of hiding cg3 behind an experimental flag until tree
manifests stabilize. The one downside of this is we lose exposing revision
flags to changegroups, which means no censor flags. I don't think anyone is
using that feature quite yet, so I don't think it will be missed.

I love the cleanup in this patch that establishes a saner API for
retrieving bundlers and unbundlers. Next step is a common interface for
these objects so we don't have to do isinstance() checks. But that's for
another day.


>
> This is to prepare for hiding changegroup3 behind a config option.
>
> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
> --- a/mercurial/bundle2.py
> +++ b/mercurial/bundle2.py
> @@ -1240,7 +1240,7 @@
>      Exists to allow extensions (like evolution) to mutate the
> capabilities.
>      """
>      caps = capabilities.copy()
> -    caps['changegroup'] = tuple(sorted(changegroup.packermap.keys()))
> +    caps['changegroup'] =
> tuple(sorted(changegroup.supportedversions(repo)))
>      if obsolete.isenabled(repo, obsolete.exchangeopt):
>          supportedformat = tuple('V%i' % v for v in obsolete.formats)
>          caps['obsmarkers'] = supportedformat
> @@ -1277,8 +1277,7 @@
>      op.gettransaction()
>      unpackerversion = inpart.params.get('version', '01')
>      # We should raise an appropriate exception here
> -    unpacker = changegroup.packermap[unpackerversion][1]
> -    cg = unpacker(inpart, None)
> +    cg = changegroup.getunbundler(unpackerversion, inpart, None)
>      # the source and url passed here are overwritten by the one contained
> in
>      # the transaction.hookargs argument. So 'bundle2' is a placeholder
>      nbchangesets = None
> diff --git a/mercurial/bundlerepo.py b/mercurial/bundlerepo.py
> --- a/mercurial/bundlerepo.py
> +++ b/mercurial/bundlerepo.py
> @@ -285,7 +285,7 @@
>                                                    "multiple changegroups")
>                      cgstream = part
>                      version = part.params.get('version', '01')
> -                    if version not in changegroup.packermap:
> +                    if version not in changegroup.supportedversions(self):
>                          msg = _('Unsupported changegroup version: %s')
>                          raise error.Abort(msg % version)
>                      if self.bundle.compressed():
> @@ -296,7 +296,7 @@
>                  raise error.Abort('No changegroups found')
>              cgstream.seek(0)
>
> -            self.bundle = changegroup.packermap[version][1](cgstream,
> 'UN')
> +            self.bundle = changegroup.getunbundler(version, cgstream,
> 'UN')
>
>          elif self.bundle.compressed():
>              f = _writetempbundle(self.bundle.read, '.hg10un',
> header='HG10UN')
> diff --git a/mercurial/changegroup.py b/mercurial/changegroup.py
> --- a/mercurial/changegroup.py
> +++ b/mercurial/changegroup.py
> @@ -912,13 +912,23 @@
>          return struct.pack(
>              self.deltaheader, node, p1n, p2n, basenode, linknode, flags)
>
> -packermap = {'01': (cg1packer, cg1unpacker),
> +_packermap = {'01': (cg1packer, cg1unpacker),
>               # cg2 adds support for exchanging generaldelta
>               '02': (cg2packer, cg2unpacker),
>               # cg3 adds support for exchanging treemanifests
>               '03': (cg3packer, cg3unpacker),
>  }
>
> +def supportedversions(repo):
> +    return _packermap.keys()
> +
> +def getbundler(version, repo, bundlecaps=None):
> +    assert version in supportedversions(repo)
> +    return _packermap[version][0](repo, bundlecaps)
> +
> +def getunbundler(version, fh, alg):
> +    return _packermap[version][1](fh, alg)
> +
>  def _changegroupinfo(repo, nodes, source):
>      if repo.ui.verbose or source == 'bundle':
>          repo.ui.status(_("%d changesets found\n") % len(nodes))
> @@ -945,7 +955,7 @@
>
>  def getsubset(repo, outgoing, bundler, source, fastpath=False):
>      gengroup = getsubsetraw(repo, outgoing, bundler, source, fastpath)
> -    return packermap[bundler.version][1](util.chunkbuffer(gengroup), None)
> +    return getunbundler(bundler.version, util.chunkbuffer(gengroup), None)
>
>  def changegroupsubset(repo, roots, heads, source, version='01'):
>      """Compute a changegroup consisting of all the nodes that are
> @@ -971,7 +981,7 @@
>      included = set(csets)
>      discbases = [n for n in discbases if n not in included]
>      outgoing = discovery.outgoing(cl, discbases, heads)
> -    bundler = packermap[version][0](repo)
> +    bundler = getbundler(version, repo)
>      return getsubset(repo, outgoing, bundler, source)
>
>  def getlocalchangegroupraw(repo, source, outgoing, bundlecaps=None,
> @@ -982,7 +992,7 @@
>      precomputed sets in outgoing. Returns a raw changegroup generator."""
>      if not outgoing.missing:
>          return None
> -    bundler = packermap[version][0](repo, bundlecaps)
> +    bundler = getbundler(version, repo, bundlecaps)
>      return getsubsetraw(repo, outgoing, bundler, source)
>
>  def getlocalchangegroup(repo, source, outgoing, bundlecaps=None,
> @@ -993,7 +1003,7 @@
>      precomputed sets in outgoing."""
>      if not outgoing.missing:
>          return None
> -    bundler = packermap[version][0](repo, bundlecaps)
> +    bundler = getbundler(version, repo, bundlecaps)
>      return getsubset(repo, outgoing, bundler, source)
>
>  def computeoutgoing(repo, heads, common):
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -2078,7 +2078,7 @@
>          ui.write('%s -- %r\n' % (part.type, repr(part.params)))
>          if part.type == 'changegroup':
>              version = part.params.get('version', '01')
> -            cg = changegroup.packermap[version][1](part, 'UN')
> +            cg = changegroup.getunbundler(version, part, 'UN')
>              chunkdata = cg.changelogheader()
>              chain = None
>              while True:
> diff --git a/mercurial/exchange.py b/mercurial/exchange.py
> --- a/mercurial/exchange.py
> +++ b/mercurial/exchange.py
> @@ -653,7 +653,8 @@
>          cg = changegroup.getlocalchangegroupraw(pushop.repo, 'push',
>                                                  pushop.outgoing)
>      else:
> -        cgversions = [v for v in cgversions if v in changegroup.packermap]
> +        cgversions = [v for v in cgversions
> +                      if v in changegroup.supportedversions(pushop.repo)]
>          if not cgversions:
>              raise ValueError(_('no common changegroup version'))
>          version = max(cgversions)
> @@ -1505,7 +1506,8 @@
>          cgversions = b2caps.get('changegroup')
>          getcgkwargs = {}
>          if cgversions:  # 3.1 and 3.2 ship with an empty value
> -            cgversions = [v for v in cgversions if v in
> changegroup.packermap]
> +            cgversions = [v for v in cgversions
> +                          if v in changegroup.supportedversions(repo)]
>              if not cgversions:
>                  raise ValueError(_('no common changegroup version'))
>              version = getcgkwargs['version'] = max(cgversions)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20160112/f85346a6/attachment-0001.html>


More information about the Mercurial-devel mailing list