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

[Martin von Zweigbergk]

On Mon, Nov 14, 2016 at 5:21 PM, Durham Goode <durham at fb.com> wrote:
>
>
> On 11/14/16 4:48 PM, Martin von Zweigbergk wrote:
>>
>> 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 we want to verify all the time, since it's good to fail early if
> someone requests a manifest that doesn't exist.  The tests don't fail
> because the only time verification should fail is if there's a logic error
> in the code.

As we talked about on IRC, we could also never verify. If we did, it
would fail at .read() time instead. I'm fine with either, so I've
queued this. I was initially concerned that it would cause reading of
one level of dirlogs too much, but that was before I understood what
your call with verify=False was about. I now understand that this is
about verifying only the root manifest. Thanks for fixing narrowhg for
us!