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

[Martin von Zweigbergk]

On Mon, Jan 11, 2016 at 12:09 PM Gregory Szorc <gregory.szorc at gmail.com>
wrote:

> On Mon, Jan 11, 2016 at 11:33 AM, Martin von Zweigbergk <
> martinvonz at google.com> wrote:
>
>>
>>
>> On Mon, Jan 11, 2016 at 11:00 AM Gregory Szorc <gregory.szorc at gmail.com>
>> wrote:
>>
>>> 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.) However, since tree
>>> manifests aren't a stable feature and are a repo requirement, I think this
>>> change is acceptable since these bundles shouldn't be being produced in the
>>> wild.
>>>
>>
>> If the server uses tree manifests, it will generate a bundle that old
>> clients can not read, whether or not we add this mandatory parameter. I
>> think adding a mandatory parameter seems like a good way of being
>> explicitly about it. Note that it was kind of explicit already in that
>> those, and only those, clients that support cg3 also support tree manifests.
>>
>
> The thing we want to avoid is clients pulling down a bundle via clone
> bundles that they aren't able to read. That's what BUNDLESPEC and
> exchange.filterclonebundleentries() are for.
>
>>
>>
>>>
>>> That being said, we /may/ want to think about adding these types of
>>> requirements to the BUNDLESPEC values.
>>>
>>
>> Oh, I didn't even know about BUNDLESPEC. I spent ~10 minutes reading
>> about it, but I don't know if it's where a treemanifest parameter should
>> go. Could someone who knows better advise?
>>
>
> Since treemanifests are cg3, we want to introduce a "v3" bundle "type" to
> denote them.
>
> While there are other ways to introduce new bundle2 parts and required
> attributes in BUNDLESPEC and clone bundles manifests, it's easier to just
> introduce a whole new bundle type (v1, v2, v3, etc) since our future
> compatibility story around those values is pretty clear.
>
>
Thanks for explaining. I'll look into that soon, but first I'm going to
move treemanifest stuff to cg4 :-P, so we don't have to add two new bundle
types. So please ignore these two patches.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20160111/60c513f1/attachment.html>