[PATCH 4 of 4] verify: also check full manifest validity during verify runs

Martin von Zweigbergk martinvonz at google.com
Wed Apr 17 10:27:09 EDT 2019


On Wed, Apr 17, 2019, 06:52 Pierre-Yves David <
pierre-yves.david at ens-lyon.org> wrote:

>
>
> On 4/17/19 3:49 PM, Martin von Zweigbergk wrote:
> >
> >
> > On Wed, Apr 17, 2019, 06:47 Pierre-Yves David
> > <pierre-yves.david at ens-lyon.org <mailto:pierre-yves.david at ens-lyon.org>>
>
> > wrote:
> >
> >
> >
> >     On 4/17/19 3:06 PM, Martin von Zweigbergk wrote:
> >      >
> >      >
> >      > On Wed, Apr 17, 2019, 03:34 Pierre-Yves David
> >      > <pierre-yves.david at ens-lyon.org
> >     <mailto:pierre-yves.david at ens-lyon.org>
> >     <mailto:pierre-yves.david at ens-lyon.org
> >     <mailto:pierre-yves.david at ens-lyon.org>>>
> >      > wrote:
> >      >
> >      >     On 4/17/19 4:59 AM, Martin von Zweigbergk wrote:
> >      >      >
> >      >      >
> >      >      > On Tue, Apr 16, 2019, 16:46 Pierre-Yves David
> >      >      > <pierre-yves.david at ens-lyon.org
> >     <mailto:pierre-yves.david at ens-lyon.org>
> >      >     <mailto:pierre-yves.david at ens-lyon.org
> >     <mailto:pierre-yves.david at ens-lyon.org>>
> >      >     <mailto:pierre-yves.david at ens-lyon.org
> >     <mailto:pierre-yves.david at ens-lyon.org>
> >      >     <mailto:pierre-yves.david at ens-lyon.org
> >     <mailto:pierre-yves.david at ens-lyon.org>>>>
> >      >      > wrote:
> >      >      >
> >      >      >     # HG changeset patch
> >      >      >     # User Pierre-Yves David
> >     <pierre-yves.david at octobus.net <mailto:pierre-yves.david at octobus.net
> >
> >      >     <mailto:pierre-yves.david at octobus.net
> >     <mailto:pierre-yves.david at octobus.net>>
> >      >      >     <mailto:pierre-yves.david at octobus.net
> >     <mailto:pierre-yves.david at octobus.net>
> >      >     <mailto:pierre-yves.david at octobus.net
> >     <mailto:pierre-yves.david at octobus.net>>>>
> >      >      >     # Date 1551881213 -3600
> >      >      >     #      Wed Mar 06 15:06:53 2019 +0100
> >      >      >     # Node ID ed796867a06764cd78a57b2ed0249353f5809319
> >      >      >     # Parent  9bec7491e9b4cabdfa4d264e5213b1f416ec2607
> >      >      >     # EXP-Topic verify
> >      >      >     # Available At
> >     https://bitbucket.org/octobus/mercurial-devel/
> >      >      >     #              hg pull
> >      >      > https://bitbucket.org/octobus/mercurial-devel/ -r
> ed796867a067
> >      >      >     verify: also check full manifest validity during
> >     verify runs
> >      >      >
> >      >      >     Before this changes, `hg verify` only checked if a
> >     manifest
> >      >     revision
> >      >      >     existed and
> >      >      >     referenced the proper files. However it never checked
> the
> >      >     manifest
> >      >      >     revision
> >      >      >     content itself.
> >      >      >
> >      >      >     Mercurial is expecting manifest entries to be sorted
> >     and will
> >      >     crash
> >      >      >     otherwise.
> >      >      >     Since `hg verify` did not attempted a full restoration
> of
> >      >     manifest
> >      >      >     entry, it
> >      >      >     could ignore this kind of corruption.
> >      >      >
> >      >      >     This new check significantly increases the cost of a
> >     `hg verify`
> >      >      >     run. This
> >      >      >     especially affects large repository not using
> >      >     `sparse-revlog`. For
> >      >      >     now, this is
> >      >      >     hidden behind the `--full` experimental flag.
> >      >      >
> >      >      >     diff --git a/mercurial/verify.py b/mercurial/verify.py
> >      >      >     --- a/mercurial/verify.py
> >      >      >     +++ b/mercurial/verify.py
> >      >      >     @@ -337,6 +337,16 @@ class verifier(object):
> >      >      >
>  filenodes.setdefault(fullpath,
> >      >      >     {}).setdefault(fn, lr)
> >      >      >                   except Exception as inst:
> >      >      >                       self._exc(lr, _("reading delta %s")
> >     % short(n),
> >      >      >     inst, label)
> >      >      >     +            if not dir and self._level >= VERIFY_FULL:
> >      >      >
> >      >      >
> >      >      > What does the "not dir" mean? I guess it's to do this
> >     check only
> >      >     for the
> >      >      > root directory when using tree manifests. Should we do it
> >     for all
> >      >      > directories?
> >      >
> >      >     That is used earlier in the same function to denote "the root
> >      >     manifest".
> >      >     I think check the root manifest will trigger checks of the
> >     sub manifest
> >      >     but I am not sure, I am not too familiar tree manifest.
> >      >
> >      >
> >      > I'm pretty sure it's about tree manifests. I asked just to make
> sure
> >      > since it surprised me that you used that as part of the condition
> >     here.
> >
> >     I know it is about tree manifest, I am reusing a pattern used by
> >     earlier
> >     code.
> >
> >      >     Can we double
> >      >     check/fix this as a follow up ?
> >      >
> >      >
> >      > It should be easy to check (just add a print statement and run
> >     tests,
> >      > for example), so I don't see much reason to fix such a simple
> >     thing in a
> >      > follow-up.
> >
> >     The question here is "Does reading the top level manifest will
> >     validated
> >     the content of the sub manifest too". I don't know how reliably to
> >     check
> >     it in a reasonable amount of time. I am not exposed to any tree
> >     manifest
> >     user myself.
> >
> >
> > Ah, no, it won't read sub manifests. You're simply skipping sub
> > manifests by including the "not dir" check.
>
> so should we just drop the "not dir" ?
>

Yes, that's what I would have expected.


> --
> Pierre-Yves David
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20190417/936dd874/attachment.html>


More information about the Mercurial-devel mailing list