[PATCH 5 of 8] manifest: add manifestlog.get to obtain subdirectory instances

[Martin von Zweigbergk]

On Mon, Sep 19, 2016 at 5:09 PM, Durham Goode <durham at fb.com> wrote:
> On 9/15/16 11:07 AM, Martin von Zweigbergk wrote:
>>
>> On Wed, Sep 14, 2016 at 4:04 PM, Durham Goode <durham at fb.com> wrote:
>>>
>>> # HG changeset patch
>>> # User Durham Goode <durham at fb.com>
>>> # Date 1473893509 25200
>>> #      Wed Sep 14 15:51:49 2016 -0700
>>> # Node ID 33a7df42b989c555972280f6b84e2fac38babf7b
>>> # Parent  d41da1522f8efb5bf5aa75a51f0093b1129b6b5a
>>> manifest: add manifestlog.get to obtain subdirectory instances
>>>
>>> Previously manifestlog only allowed obtaining root level manifests.
>>> Future
>>> patches will need direct access to subdirectory manifests as part of
>>> changegroup
>>> creation, so let's add a get() function that knows how to deal with
>>> subdirectories.
>>>
>>> diff --git a/mercurial/manifest.py b/mercurial/manifest.py
>>> --- a/mercurial/manifest.py
>>> +++ b/mercurial/manifest.py
>>> @@ -1040,20 +1040,34 @@ class manifestlog(object):
>>>           """Retrieves the manifest instance for the given node. Throws a
>>> KeyError
>>>           if not found.
>>>           """
>>> -        if node in self._mancache:
>>> -            cachemf = self._mancache[node]
>>> -            # The old manifest may put non-ctx manifests in the cache,
>>> so skip
>>> -            # those since they don't implement the full api.
>>> -            if (isinstance(cachemf, manifestctx) or
>>> -                isinstance(cachemf, treemanifestctx)):
>>> -                return cachemf
>>> +        return self.get('', node)
>>>
>>> -        if self._treeinmem:
>>> -            m = treemanifestctx(self._revlog, '', node)
>>> +    def get(self, dir, node):
>>> +        """Retrieves the manifest instance for the given node. Throws a
>>> KeyError
>>> +        if not found.
>>> +        """
>>> +        if dir:
>>> +            if self._treeinmem:
>>
>> I think this should be checking treeondisk. When using flat manifests
>> on disk and trees in memory, it still doesn't make sense to ask for a
>> specific directory and node.
>>
>> Speaking of that, have you tried setting treeinmem=True a few times
>> during this refactoring? I wouldn't be surprised if it was broken even
>> before you started, so that would be the first thing to check if you
>> haven't already.
>
> I'll switch this to treeondisk.  I've not tried treeinmem=True.  Is that not
> automatically covered by the tests?

Unfortunately not. We could write tests specifically for it if we
added some debug option to enable it, but we don't have any such test
yet. The advantage of setting it manually is that it's easy to enable
it for the entire test suite. There's no reason not to have both,
though. I just haven't done it (well, I also don't know if we have any
existing debug options other than for extra loggin, so I don't know
how to do that).