[PATCH 1 of 2] manifest: make revlog verification optional

[Martin von Zweigbergk]

On Mon, Nov 14, 2016 at 4:26 PM, Durham Goode <durham at fb.com> wrote:
> On 11/14/16 4:23 PM, Martin von Zweigbergk wrote:
>>
>> On Mon, Nov 14, 2016 at 3:27 PM, Durham Goode <durham at fb.com> wrote:
>>>
>>> # HG changeset patch
>>> # User Durham Goode <durham at fb.com>
>>> # Date 1479165447 28800
>>> #      Mon Nov 14 15:17:27 2016 -0800
>>> # Node ID 27209d52a5865422c5ef4ba05cedb28ce32919ed
>>> # Parent  046a7e828ea63ec940ffae1089a33fae7954da2e
>>> manifest: make revlog verification optional
>>>
>>> This patches adds an parameter to manifestlog.get() to disable hash
>>> checking.
>>> This will be used in an upcoming patch to support treemanifestctx reading
>>> sub-trees without loading them from the revlog. (This is already
>>> supported but
>>> does not go through the manifestlog.get() code path)
>>>
>>> diff --git a/mercurial/manifest.py b/mercurial/manifest.py
>>> --- a/mercurial/manifest.py
>>> +++ b/mercurial/manifest.py
>>> @@ -1278,9 +1278,12 @@ class manifestlog(object):
>>>           """
>>>           return self.get('', node)
>>>
>>> -    def get(self, dir, node):
>>> +    def get(self, dir, node, verify=True):
>>>           """Retrieves the manifest instance for the given node. Throws a
>>>           LookupError if not found.
>>> +
>>> +        `verify` - if True an exception will be thrown if the node is
>>> not in
>>> +                   the revlog
>>
>> Patch 2/2 says "Set verify to False since we need to be able to create
>> subtrees for trees that don't exist on disk yet.", which makes it
>> sounds like it's only about reading empty revlogs, which should be
>> safe anyway. What am I missing?
>
> "yet" was probably the wrong word there.  This logic is to support the test
> case where the test moves some of the revlogs out of the way and expects the
> treemanifest to still work because those revlogs are outside the narrow
> spec. So this verification needs to be turned off so the ctx can be created
> (and it just so happens that the contents of the ctx are never accessed).

Ah, I see. Another question: When do we want to verify? Tests seem to
pass even if I always turn of verification (defaulted it to False).

>
> I think it's a bit weird that readsubtree() is called for trees outside the
> narrow spec (which is why we need to handle this case), but that would
> require a deeper refactoring.