Patchwork verify: check raw revision size

login
register
mail settings
Submitter Jun Wu
Date March 28, 2017, 9:27 p.m.
Message ID <ce672245b46749351a71.1490736458@localhost.localdomain>
Download mbox | patch
Permalink /patch/19797/
State Superseded
Headers show

Comments

Jun Wu - March 28, 2017, 9:27 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1490736238 25200
#      Tue Mar 28 14:23:58 2017 -0700
# Node ID ce672245b46749351a71701d2fa6c786d9b2eebb
# Parent  331cc4433efe0d897bb16ad4ff08a3fbe850869b
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r ce672245b467
verify: check raw revision size

The code is assuming revlog.size(rev) == len(revlog.revision(rev)). Since
revlog.size(rev) returns the raw size, before running any flag processor,
the code should be updated to fetch the revision with raw=True.
Gregory Szorc - March 29, 2017, 2:15 a.m.
On Tue, Mar 28, 2017 at 2:27 PM, Jun Wu <quark@fb.com> wrote:

> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1490736238 25200
> #      Tue Mar 28 14:23:58 2017 -0700
> # Node ID ce672245b46749351a71701d2fa6c786d9b2eebb
> # Parent  331cc4433efe0d897bb16ad4ff08a3fbe850869b
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r
> ce672245b467
> verify: check raw revision size
>
> The code is assuming revlog.size(rev) == len(revlog.revision(rev)). Since
> revlog.size(rev) returns the raw size, before running any flag processor,
> the code should be updated to fetch the revision with raw=True.
>

LGTM. A test would be nice. But my understanding is the flags processing
code is a bit under-tested as is `hg verify`. You didn't walk into that
trap, so I won't bloat scope on you.


>
> diff --git a/mercurial/verify.py b/mercurial/verify.py
> --- a/mercurial/verify.py
> +++ b/mercurial/verify.py
> @@ -385,5 +385,5 @@ class verifier(object):
>                      rp = fl.renamed(n)
>                      if l != fl.size(i):
> -                        if len(fl.revision(n)) != fl.size(i):
> +                        if len(fl.revision(n, raw=True)) != fl.size(i):
>                              self.err(lr, _("unpacked size is %s, %s
> expected") %
>                                       (l, fl.size(i)), f)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Jun Wu - March 29, 2017, 2:46 a.m.
Excerpts from Gregory Szorc's message of 2017-03-28 19:15:21 -0700:
> On Tue, Mar 28, 2017 at 2:27 PM, Jun Wu <quark@fb.com> wrote:
> 
> > # HG changeset patch
> > # User Jun Wu <quark@fb.com>
> > # Date 1490736238 25200
> > #      Tue Mar 28 14:23:58 2017 -0700
> > # Node ID ce672245b46749351a71701d2fa6c786d9b2eebb
> > # Parent  331cc4433efe0d897bb16ad4ff08a3fbe850869b
> > # Available At https://bitbucket.org/quark-zju/hg-draft 
> > #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r
> > ce672245b467
> > verify: check raw revision size
> >
> > The code is assuming revlog.size(rev) == len(revlog.revision(rev)). Since
> > revlog.size(rev) returns the raw size, before running any flag processor,
> > the code should be updated to fetch the revision with raw=True.
> >
> 
> LGTM. A test would be nice. But my understanding is the flags processing
> code is a bit under-tested as is `hg verify`. You didn't walk into that
> trap, so I won't bloat scope on you.

The flag processing code is terribly broken now, and will be better tested
after [1] and its friends. I can try to add a test for verify after the
revlog fixes.

[1]: https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-March/095754.html

> >
> > diff --git a/mercurial/verify.py b/mercurial/verify.py
> > --- a/mercurial/verify.py
> > +++ b/mercurial/verify.py
> > @@ -385,5 +385,5 @@ class verifier(object):
> >                      rp = fl.renamed(n)
> >                      if l != fl.size(i):
> > -                        if len(fl.revision(n)) != fl.size(i):
> > +                        if len(fl.revision(n, raw=True)) != fl.size(i):
> >                              self.err(lr, _("unpacked size is %s, %s
> > expected") %
> >                                       (l, fl.size(i)), f)
Jun Wu - March 29, 2017, 7:50 p.m.
Actually, there are more issues with "verify". I'll send an updated version,
probably with a test.

Excerpts from Jun Wu's message of 2017-03-28 14:27:38 -0700:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1490736238 25200
> #      Tue Mar 28 14:23:58 2017 -0700
> # Node ID ce672245b46749351a71701d2fa6c786d9b2eebb
> # Parent  331cc4433efe0d897bb16ad4ff08a3fbe850869b
> # Available At https://bitbucket.org/quark-zju/hg-draft 
> #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r ce672245b467
> verify: check raw revision size
> 
> The code is assuming revlog.size(rev) == len(revlog.revision(rev)). Since
> revlog.size(rev) returns the raw size, before running any flag processor,
> the code should be updated to fetch the revision with raw=True.
> 
> diff --git a/mercurial/verify.py b/mercurial/verify.py
> --- a/mercurial/verify.py
> +++ b/mercurial/verify.py
> @@ -385,5 +385,5 @@ class verifier(object):
>                      rp = fl.renamed(n)
>                      if l != fl.size(i):
> -                        if len(fl.revision(n)) != fl.size(i):
> +                        if len(fl.revision(n, raw=True)) != fl.size(i):
>                              self.err(lr, _("unpacked size is %s, %s expected") %
>                                       (l, fl.size(i)), f)

Patch

diff --git a/mercurial/verify.py b/mercurial/verify.py
--- a/mercurial/verify.py
+++ b/mercurial/verify.py
@@ -385,5 +385,5 @@  class verifier(object):
                     rp = fl.renamed(n)
                     if l != fl.size(i):
-                        if len(fl.revision(n)) != fl.size(i):
+                        if len(fl.revision(n, raw=True)) != fl.size(i):
                             self.err(lr, _("unpacked size is %s, %s expected") %
                                      (l, fl.size(i)), f)