Patchwork [2,of,4] verify: introduce a notion of "level"

login
register
mail settings
Submitter Pierre-Yves David
Date April 16, 2019, 11:38 p.m.
Message ID <55bd98999c25b10e2204.1555457920@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/39676/
State Superseded
Headers show

Comments

Pierre-Yves David - April 16, 2019, 11:38 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@octobus.net>
# Date 1555456341 -7200
#      Wed Apr 17 01:12:21 2019 +0200
# Node ID 55bd98999c25b10e220477fd4cc446a7c9c1f8ca
# Parent  f233cb63bc077267d8571378350d9563cbabcf3d
# EXP-Topic verify
# Available At https://bitbucket.org/octobus/mercurial-devel/
#              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 55bd98999c25
verify: introduce a notion of "level"

Some checks are slower than others, to help the user to run the checks he needs,
we are about to introduce new flag to select faster vs deeper runs. This put
the scaffolding in place to do this.
Gregory Szorc - April 17, 2019, 1:15 p.m.
> On Apr 17, 2019, at 01:38, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@octobus.net>
> # Date 1555456341 -7200
> #      Wed Apr 17 01:12:21 2019 +0200
> # Node ID 55bd98999c25b10e220477fd4cc446a7c9c1f8ca
> # Parent  f233cb63bc077267d8571378350d9563cbabcf3d
> # EXP-Topic verify
> # Available At https://bitbucket.org/octobus/mercurial-devel/
> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 55bd98999c25
> verify: introduce a notion of "level"
> 
> Some checks are slower than others, to help the user to run the checks he needs,
> we are about to introduce new flag to select faster vs deeper runs. This put
> the scaffolding in place to do this.

I’m in favor of customizing verify behavior: it is an overdue feature IMO.

Experience tells me that shoehorning things into a numeric level will be fragile and won’t scale well. I’m wondering if we should introduce individual “feature flags” / arguments to control what is verified. That will make the code a bit cleaner and easier to separate IMO. If we want to map a number to a set of verify options, we can do that too.

This idea is scope bloat. But something tells me we’ll regret the limitations of numeric levels in the future. I’d rather pass in a set of things to verify. This is also more extensible.

> 
> diff --git a/mercurial/hg.py b/mercurial/hg.py
> --- a/mercurial/hg.py
> +++ b/mercurial/hg.py
> @@ -1092,9 +1092,9 @@ def outgoing(ui, repo, dest, opts):
>     recurse()
>     return 0 # exit code is zero since we found outgoing changes
> 
> -def verify(repo):
> +def verify(repo, level=None):
>     """verify the consistency of a repository"""
> -    ret = verifymod.verify(repo)
> +    ret = verifymod.verify(repo, level=level)
> 
>     # Broken subrepo references in hidden csets don't seem worth worrying about,
>     # since they can't be pushed/pulled, and --hidden can be used if they are a
> diff --git a/mercurial/verify.py b/mercurial/verify.py
> --- a/mercurial/verify.py
> +++ b/mercurial/verify.py
> @@ -22,9 +22,12 @@ from . import (
>     util,
> )
> 
> -def verify(repo):
> +VERIFY_DEFAULT = 0
> +
> +def verify(repo, level=None):
>     with repo.lock():
> -        return verifier(repo).verify()
> +        v = verifier(repo, level)
> +        return v.verify()
> 
> def _normpath(f):
>     # under hg < 2.4, convert didn't sanitize paths properly, so a
> @@ -34,10 +37,13 @@ def _normpath(f):
>     return f
> 
> class verifier(object):
> -    def __init__(self, repo):
> +    def __init__(self, repo, level=None):
>         self.repo = repo.unfiltered()
>         self.ui = repo.ui
>         self.match = repo.narrowmatch()
> +        if level is None:
> +            level = VERIFY_DEFAULT
> +        self._level = level
>         self.badrevs = set()
>         self.errors = 0
>         self.warnings = 0
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
via Mercurial-devel - April 17, 2019, 1:32 p.m.
Heh, that's very much related to my comment on the V2 of this patch.

Pierre-Yves, have you considered sending patches via Phabricator? That
keeps comments on various versions of a patch together in one place.

On Wed, Apr 17, 2019, 06:18 Gregory Szorc <gregory.szorc@gmail.com> wrote:

>
>
> > On Apr 17, 2019, at 01:38, Pierre-Yves David <
> pierre-yves.david@ens-lyon.org> wrote:
> >
> > # HG changeset patch
> > # User Pierre-Yves David <pierre-yves.david@octobus.net>
> > # Date 1555456341 -7200
> > #      Wed Apr 17 01:12:21 2019 +0200
> > # Node ID 55bd98999c25b10e220477fd4cc446a7c9c1f8ca
> > # Parent  f233cb63bc077267d8571378350d9563cbabcf3d
> > # EXP-Topic verify
> > # Available At https://bitbucket.org/octobus/mercurial-devel/
> > #              hg pull https://bitbucket.org/octobus/mercurial-devel/
> -r 55bd98999c25
> > verify: introduce a notion of "level"
> >
> > Some checks are slower than others, to help the user to run the checks
> he needs,
> > we are about to introduce new flag to select faster vs deeper runs. This
> put
> > the scaffolding in place to do this.
>
> I’m in favor of customizing verify behavior: it is an overdue feature IMO.
>
> Experience tells me that shoehorning things into a numeric level will be
> fragile and won’t scale well. I’m wondering if we should introduce
> individual “feature flags” / arguments to control what is verified. That
> will make the code a bit cleaner and easier to separate IMO. If we want to
> map a number to a set of verify options, we can do that too.
>
> This idea is scope bloat. But something tells me we’ll regret the
> limitations of numeric levels in the future. I’d rather pass in a set of
> things to verify. This is also more extensible.
>
> >
> > diff --git a/mercurial/hg.py b/mercurial/hg.py
> > --- a/mercurial/hg.py
> > +++ b/mercurial/hg.py
> > @@ -1092,9 +1092,9 @@ def outgoing(ui, repo, dest, opts):
> >     recurse()
> >     return 0 # exit code is zero since we found outgoing changes
> >
> > -def verify(repo):
> > +def verify(repo, level=None):
> >     """verify the consistency of a repository"""
> > -    ret = verifymod.verify(repo)
> > +    ret = verifymod.verify(repo, level=level)
> >
> >     # Broken subrepo references in hidden csets don't seem worth
> worrying about,
> >     # since they can't be pushed/pulled, and --hidden can be used if
> they are a
> > diff --git a/mercurial/verify.py b/mercurial/verify.py
> > --- a/mercurial/verify.py
> > +++ b/mercurial/verify.py
> > @@ -22,9 +22,12 @@ from . import (
> >     util,
> > )
> >
> > -def verify(repo):
> > +VERIFY_DEFAULT = 0
> > +
> > +def verify(repo, level=None):
> >     with repo.lock():
> > -        return verifier(repo).verify()
> > +        v = verifier(repo, level)
> > +        return v.verify()
> >
> > def _normpath(f):
> >     # under hg < 2.4, convert didn't sanitize paths properly, so a
> > @@ -34,10 +37,13 @@ def _normpath(f):
> >     return f
> >
> > class verifier(object):
> > -    def __init__(self, repo):
> > +    def __init__(self, repo, level=None):
> >         self.repo = repo.unfiltered()
> >         self.ui = repo.ui
> >         self.match = repo.narrowmatch()
> > +        if level is None:
> > +            level = VERIFY_DEFAULT
> > +        self._level = level
> >         self.badrevs = set()
> >         self.errors = 0
> >         self.warnings = 0
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Pierre-Yves David - April 17, 2019, 1:54 p.m.
On 4/17/19 3:15 PM, Gregory Szorc wrote:
> 
> 
>> On Apr 17, 2019, at 01:38, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>>
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@octobus.net>
>> # Date 1555456341 -7200
>> #      Wed Apr 17 01:12:21 2019 +0200
>> # Node ID 55bd98999c25b10e220477fd4cc446a7c9c1f8ca
>> # Parent  f233cb63bc077267d8571378350d9563cbabcf3d
>> # EXP-Topic verify
>> # Available At https://bitbucket.org/octobus/mercurial-devel/
>> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 55bd98999c25
>> verify: introduce a notion of "level"
>>
>> Some checks are slower than others, to help the user to run the checks he needs,
>> we are about to introduce new flag to select faster vs deeper runs. This put
>> the scaffolding in place to do this.
> 
> I’m in favor of customizing verify behavior: it is an overdue feature IMO.
> 
> Experience tells me that shoehorning things into a numeric level will be fragile and won’t scale well. I’m wondering if we should introduce individual “feature flags” / arguments to control what is verified. That will make the code a bit cleaner and easier to separate IMO. If we want to map a number to a set of verify options, we can do that too.
> 
> This idea is scope bloat. But something tells me we’ll regret the limitations of numeric levels in the future. I’d rather pass in a set of things to verify. This is also more extensible.

I agree with you on the general principle (being able to select what we 
check for. I also agree that it is outside the scope of this small 
series. (I documented the checks earlier this cycle, so we will have 
something to start from).

I am not sure if you are requesting changes here or not. The internal 
implementation can be changed later and the flag is experimental, so 
everything in this series can be adjusted later.

Do you have specific changes in mind to get this series in ?


> 
>>
>> diff --git a/mercurial/hg.py b/mercurial/hg.py
>> --- a/mercurial/hg.py
>> +++ b/mercurial/hg.py
>> @@ -1092,9 +1092,9 @@ def outgoing(ui, repo, dest, opts):
>>      recurse()
>>      return 0 # exit code is zero since we found outgoing changes
>>
>> -def verify(repo):
>> +def verify(repo, level=None):
>>      """verify the consistency of a repository"""
>> -    ret = verifymod.verify(repo)
>> +    ret = verifymod.verify(repo, level=level)
>>
>>      # Broken subrepo references in hidden csets don't seem worth worrying about,
>>      # since they can't be pushed/pulled, and --hidden can be used if they are a
>> diff --git a/mercurial/verify.py b/mercurial/verify.py
>> --- a/mercurial/verify.py
>> +++ b/mercurial/verify.py
>> @@ -22,9 +22,12 @@ from . import (
>>      util,
>> )
>>
>> -def verify(repo):
>> +VERIFY_DEFAULT = 0
>> +
>> +def verify(repo, level=None):
>>      with repo.lock():
>> -        return verifier(repo).verify()
>> +        v = verifier(repo, level)
>> +        return v.verify()
>>
>> def _normpath(f):
>>      # under hg < 2.4, convert didn't sanitize paths properly, so a
>> @@ -34,10 +37,13 @@ def _normpath(f):
>>      return f
>>
>> class verifier(object):
>> -    def __init__(self, repo):
>> +    def __init__(self, repo, level=None):
>>          self.repo = repo.unfiltered()
>>          self.ui = repo.ui
>>          self.match = repo.narrowmatch()
>> +        if level is None:
>> +            level = VERIFY_DEFAULT
>> +        self._level = level
>>          self.badrevs = set()
>>          self.errors = 0
>>          self.warnings = 0
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Gregory Szorc - April 17, 2019, 9:42 p.m.
On Wed, Apr 17, 2019 at 7:54 AM Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 4/17/19 3:15 PM, Gregory Szorc wrote:
> >
> >
> >> On Apr 17, 2019, at 01:38, Pierre-Yves David <
> pierre-yves.david@ens-lyon.org> wrote:
> >>
> >> # HG changeset patch
> >> # User Pierre-Yves David <pierre-yves.david@octobus.net>
> >> # Date 1555456341 -7200
> >> #      Wed Apr 17 01:12:21 2019 +0200
> >> # Node ID 55bd98999c25b10e220477fd4cc446a7c9c1f8ca
> >> # Parent  f233cb63bc077267d8571378350d9563cbabcf3d
> >> # EXP-Topic verify
> >> # Available At https://bitbucket.org/octobus/mercurial-devel/
> >> #              hg pull https://bitbucket.org/octobus/mercurial-devel/
> -r 55bd98999c25
> >> verify: introduce a notion of "level"
> >>
> >> Some checks are slower than others, to help the user to run the checks
> he needs,
> >> we are about to introduce new flag to select faster vs deeper runs.
> This put
> >> the scaffolding in place to do this.
> >
> > I’m in favor of customizing verify behavior: it is an overdue feature
> IMO.
> >
> > Experience tells me that shoehorning things into a numeric level will be
> fragile and won’t scale well. I’m wondering if we should introduce
> individual “feature flags” / arguments to control what is verified. That
> will make the code a bit cleaner and easier to separate IMO. If we want to
> map a number to a set of verify options, we can do that too.
> >
> > This idea is scope bloat. But something tells me we’ll regret the
> limitations of numeric levels in the future. I’d rather pass in a set of
> things to verify. This is also more extensible.
>
> I agree with you on the general principle (being able to select what we
> check for. I also agree that it is outside the scope of this small
> series. (I documented the checks earlier this cycle, so we will have
> something to start from).
>
> I am not sure if you are requesting changes here or not. The internal
> implementation can be changed later and the flag is experimental, so
> everything in this series can be adjusted later.
>
> Do you have specific changes in mind to get this series in ?
>

I have no major objections to using a numeric level for now. But if any
more complexity is added, it might be best to do something more flexible
sooner rather than later.


>
>
> >
> >>
> >> diff --git a/mercurial/hg.py b/mercurial/hg.py
> >> --- a/mercurial/hg.py
> >> +++ b/mercurial/hg.py
> >> @@ -1092,9 +1092,9 @@ def outgoing(ui, repo, dest, opts):
> >>      recurse()
> >>      return 0 # exit code is zero since we found outgoing changes
> >>
> >> -def verify(repo):
> >> +def verify(repo, level=None):
> >>      """verify the consistency of a repository"""
> >> -    ret = verifymod.verify(repo)
> >> +    ret = verifymod.verify(repo, level=level)
> >>
> >>      # Broken subrepo references in hidden csets don't seem worth
> worrying about,
> >>      # since they can't be pushed/pulled, and --hidden can be used if
> they are a
> >> diff --git a/mercurial/verify.py b/mercurial/verify.py
> >> --- a/mercurial/verify.py
> >> +++ b/mercurial/verify.py
> >> @@ -22,9 +22,12 @@ from . import (
> >>      util,
> >> )
> >>
> >> -def verify(repo):
> >> +VERIFY_DEFAULT = 0
> >> +
> >> +def verify(repo, level=None):
> >>      with repo.lock():
> >> -        return verifier(repo).verify()
> >> +        v = verifier(repo, level)
> >> +        return v.verify()
> >>
> >> def _normpath(f):
> >>      # under hg < 2.4, convert didn't sanitize paths properly, so a
> >> @@ -34,10 +37,13 @@ def _normpath(f):
> >>      return f
> >>
> >> class verifier(object):
> >> -    def __init__(self, repo):
> >> +    def __init__(self, repo, level=None):
> >>          self.repo = repo.unfiltered()
> >>          self.ui = repo.ui
> >>          self.match = repo.narrowmatch()
> >> +        if level is None:
> >> +            level = VERIFY_DEFAULT
> >> +        self._level = level
> >>          self.badrevs = set()
> >>          self.errors = 0
> >>          self.warnings = 0
> >> _______________________________________________
> >> Mercurial-devel mailing list
> >> Mercurial-devel@mercurial-scm.org
> >> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
> --
> Pierre-Yves David
>

Patch

diff --git a/mercurial/hg.py b/mercurial/hg.py
--- a/mercurial/hg.py
+++ b/mercurial/hg.py
@@ -1092,9 +1092,9 @@  def outgoing(ui, repo, dest, opts):
     recurse()
     return 0 # exit code is zero since we found outgoing changes
 
-def verify(repo):
+def verify(repo, level=None):
     """verify the consistency of a repository"""
-    ret = verifymod.verify(repo)
+    ret = verifymod.verify(repo, level=level)
 
     # Broken subrepo references in hidden csets don't seem worth worrying about,
     # since they can't be pushed/pulled, and --hidden can be used if they are a
diff --git a/mercurial/verify.py b/mercurial/verify.py
--- a/mercurial/verify.py
+++ b/mercurial/verify.py
@@ -22,9 +22,12 @@  from . import (
     util,
 )
 
-def verify(repo):
+VERIFY_DEFAULT = 0
+
+def verify(repo, level=None):
     with repo.lock():
-        return verifier(repo).verify()
+        v = verifier(repo, level)
+        return v.verify()
 
 def _normpath(f):
     # under hg < 2.4, convert didn't sanitize paths properly, so a
@@ -34,10 +37,13 @@  def _normpath(f):
     return f
 
 class verifier(object):
-    def __init__(self, repo):
+    def __init__(self, repo, level=None):
         self.repo = repo.unfiltered()
         self.ui = repo.ui
         self.match = repo.narrowmatch()
+        if level is None:
+            level = VERIFY_DEFAULT
+        self._level = level
         self.badrevs = set()
         self.errors = 0
         self.warnings = 0