[PATCH 1 of 2] treemanifests: set bundle2 part parameter indicating treemanifest

[Sean Farley]

Gregory Szorc <gregory.szorc at gmail.com> writes:

> On Mon, Jan 11, 2016 at 11:34 AM, Sean Farley <sean at farley.io> wrote:
>
>>
>> Gregory Szorc <gregory.szorc at gmail.com> writes:
>>
>> > On Fri, Jan 8, 2016 at 9:18 PM, Martin von Zweigbergk <
>> martinvonz at google.com
>> >> wrote:
>> >
>> >> # HG changeset patch
>> >> # User Martin von Zweigbergk <martinvonz at google.com>
>> >> # Date 1452316386 28800
>> >> #      Fri Jan 08 21:13:06 2016 -0800
>> >> # Node ID ea75b21a97b7549441941268c317995591230dc5
>> >> # Parent  76fc5ac23473a25216f9cba2f977d1f56aa9df92
>> >> treemanifests: set bundle2 part parameter indicating treemanifest
>> >>
>> >> By adding a mandatory 'treemanifest' parameter in the bundle2 part, we
>> >> make it possible for the recipient to set repo requirements before the
>> >> manifest revlog is accessed.
>> >>
>> >> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
>> >> --- a/mercurial/bundle2.py
>> >> +++ b/mercurial/bundle2.py
>> >> @@ -1262,7 +1262,7 @@
>> >>      obscaps = caps.get('obsmarkers', ())
>> >>      return [int(c[1:]) for c in obscaps if c.startswith('V')]
>> >>
>> >> - at parthandler('changegroup', ('version', 'nbchanges'))
>> >> + at parthandler('changegroup', ('version', 'nbchanges', 'treemanifest'))
>> >>  def handlechangegroup(op, inpart):
>> >>      """apply a changegroup part on the repo
>> >>
>> >> @@ -1284,6 +1284,15 @@
>> >>      nbchangesets = None
>> >>      if 'nbchanges' in inpart.params:
>> >>          nbchangesets = int(inpart.params.get('nbchanges'))
>> >> +    if ('treemanifest' in inpart.params and
>> >> +        'treemanifest' not in op.repo.requirements):
>> >> +        if len(op.repo.changelog) != 0:
>> >> +            raise error.Abort(_(
>> >> +                "bundle contains tree manifests, but local repo is "
>> >> +                "non-empty and does not use tree manifests"))
>> >> +        op.repo.requirements.add('treemanifest')
>> >> +        op.repo._applyopenerreqs()
>> >> +        op.repo._writerequirements()
>> >>      ret = cg.apply(op.repo, 'bundle2', 'bundle2',
>> >> expectedtotal=nbchangesets)
>> >>      op.records.add('changegroup', {'return': ret})
>> >>      if op.reply is not None:
>> >> diff --git a/mercurial/changegroup.py b/mercurial/changegroup.py
>> >> --- a/mercurial/changegroup.py
>> >> +++ b/mercurial/changegroup.py
>> >> @@ -1047,17 +1047,6 @@
>> >>          directory = (f[-1] == '/')
>> >>          if directory:
>> >>              # a directory using treemanifests
>> >> -            # TODO fixup repo requirements safely
>> >> -            if 'treemanifest' not in repo.requirements:
>> >> -                if not wasempty:
>> >> -                    raise error.Abort(_(
>> >> -                        "bundle contains tree manifests, but local repo
>> >> is "
>> >> -                        "non-empty and does not use tree manifests"))
>> >> -                repo.requirements.add('treemanifest')
>> >> -                repo._applyopenerreqs()
>> >> -                repo._writerequirements()
>> >> -                repo.manifest._treeondisk = True
>> >> -                repo.manifest._treeinmem = True
>> >>              fl = repo.manifest.dirlog(f)
>> >>          else:
>> >>              fl = repo.file(f)
>> >> diff --git a/mercurial/exchange.py b/mercurial/exchange.py
>> >> --- a/mercurial/exchange.py
>> >> +++ b/mercurial/exchange.py
>> >> @@ -1519,6 +1519,8 @@
>> >>          if version is not None:
>> >>              part.addparam('version', version)
>> >>          part.addparam('nbchanges', str(len(outgoing.missing)),
>> >> mandatory=False)
>> >> +        if 'treemanifest' in repo.requirements:
>> >> +            part.addparam('treemanifest', '1')
>> >>
>> >>
>> > This is borderline something we shouldn't do because adding new required
>> > params to bundle2 parts can break consumers of clone bundles. (New server
>> > generates bundle which old client can no longer read.)
>>
>> I thought the whole point of bundle2 was that we could add new params
>> without requiring a new bundle protocol?
>>
>
> It doesn't require a whole new protocol. But you need to have content
> negotiation to ensure you don't serve unsuported parts or parameters to
> legacy clients. BUNDLESPEC and clone bundles manifests are effectively
> static content negotiation (as opposed to the client sending its
> capabilities to the server when issuing the getbundle wire proto command).

Ok, I think I see (at least a little better) now.