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

Durham Goode durham at fb.com
Mon Nov 14 20:21:38 EST 2016



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.


More information about the Mercurial-devel mailing list