[PATCH] treemanifest: allow manifestrevlog to take an explicit treemanifest arg

[Ryan McElroy]

On 5/9/17 2:23 AM, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham at fb.com>
> # Date 1494292924 25200
> #      Mon May 08 18:22:04 2017 -0700
> # Node ID 6d02589fd6a76c354aff165503cc72333b14307a
> # Parent  8f1a2b848b52ea7bf3fe2404e3b62924c7aae93f
> treemanifest: allow manifestrevlog to take an explicit treemanifest arg
>
> Previously we relied on the opener options to tell the revlog to be a tree
> manifest. This makes it complicated for extensions to create treemanifests and
> normal manifests at the same time. Let's add a construtor argument to create a
> treemanifest revlog as well.
>
> diff --git a/mercurial/manifest.py b/mercurial/manifest.py
> --- a/mercurial/manifest.py
> +++ b/mercurial/manifest.py
> @@ -1175,25 +1175,29 @@ class manifestrevlog(revlog.revlog):
>       '''A revlog that stores manifest texts. This is responsible for caching the
>       full-text manifest contents.
>       '''
> -    def __init__(self, opener, dir='', dirlogcache=None, indexfile=None):
> +    def __init__(self, opener, dir='', dirlogcache=None, indexfile=None,
> +                 treemanifest=False):
>           """Constructs a new manifest revlog
>   
>           `indexfile` - used by extensions to have two manifests at once, like
>           when transitioning between flatmanifeset and treemanifests.
> +
> +        `treemanifest` - used to indicate this is a tree manifest revlog. If the
> +        treemanifest opener option is set to True, this argument is ignored.

This comment doesn't look quite accurate: from the code, it looks like 
if either treemanifest=True here, or opts.treemanifest is True, then you 
use Treemanifests, so "ignored" isn't quite the right term it seems to 
me. I'd like to see this clarified.

>           """
>           # During normal operations, we expect to deal with not more than four
>           # revs at a time (such as during commit --amend). When rebasing large
>           # stacks of commits, the number can go up, hence the config knob below.
>           cachesize = 4
> -        usetreemanifest = False
> +        optiontreemanifest = False

Not sure why the rename here -- usetreemanifest is used elsewhere in 
this file (down in the manifestlog constructor, around line 1311) and it 
still uses the "old" name and override mechanism, so it seems that both 
should be updated at the same time, or in the same series.

>           usemanifestv2 = False
>           opts = getattr(opener, 'options', None)
>           if opts is not None:
>               cachesize = opts.get('manifestcachesize', cachesize)
> -            usetreemanifest = opts.get('treemanifest', usetreemanifest)
> +            optiontreemanifest = opts.get('treemanifest', False)
>               usemanifestv2 = opts.get('manifestv2', usemanifestv2)
>   
> -        self._treeondisk = usetreemanifest
> +        self._treeondisk = optiontreemanifest or treemanifest
>           self._usemanifestv2 = usemanifestv2
>   
>           self._fulltextcache = util.lrucachedict(cachesize)
> @@ -1232,7 +1236,8 @@ class manifestrevlog(revlog.revlog):
>               assert self._treeondisk
>           if dir not in self._dirlogcache:
>               self._dirlogcache[dir] = manifestrevlog(self.opener, dir,
> -                                                    self._dirlogcache)
> +                                                    self._dirlogcache,
> +                                                    treemanifest=True)

It's not clear to me that this should have been added as True here. What 
am I missing that makes this correct?

>           return self._dirlogcache[dir]
>   
>       def add(self, m, transaction, link, p1, p2, added, removed, readtree=None):
>

I have enough questions about this I'd like to see a follow-up so I'll 
mark as "changes requested" in patchwork.