Patchwork [2,of,2,V2] verify: fix length check

login
register
mail settings
Submitter Jun Wu
Date March 30, 2017, 9:51 p.m.
Message ID <35902a0e3f38c766666a.1490910675@x1c>
Download mbox | patch
Permalink /patch/19852/
State Accepted
Delegated to: Yuya Nishihara
Headers show

Comments

Jun Wu - March 30, 2017, 9:51 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1490824154 25200
#      Wed Mar 29 14:49:14 2017 -0700
# Node ID 35902a0e3f38c766666a66d0dfbf76ec72091832
# Parent  17b41390f4912a4c18538d778837bc2cf4a1be92
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 35902a0e3f38
verify: fix length check

According to the document added above, we should check L1 == L2, and the
only way to get L1 in all cases is to call "rawsize()", and the only way to
get L2 is to call "revision(raw=True)". Therefore the fix.

Meanwhile there are still a lot of things about flagprocessor broken in
revlog.py. Tests will be added after revlog.py gets fixed.
Ryan McElroy - March 31, 2017, 10:13 a.m.
On 3/30/17 10:51 PM, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1490824154 25200
> #      Wed Mar 29 14:49:14 2017 -0700
> # Node ID 35902a0e3f38c766666a66d0dfbf76ec72091832
> # Parent  17b41390f4912a4c18538d778837bc2cf4a1be92
> verify: fix length check
This series looks good to me. Marked as pre-reviewed in patchwork.

>
> According to the document added above, we should check L1 == L2, and the
> only way to get L1 in all cases is to call "rawsize()", and the only way to
> get L2 is to call "revision(raw=True)". Therefore the fix.
>
> Meanwhile there are still a lot of things about flagprocessor broken in
> revlog.py. Tests will be added after revlog.py gets fixed.
>
> diff --git a/mercurial/verify.py b/mercurial/verify.py
> --- a/mercurial/verify.py
> +++ b/mercurial/verify.py
> @@ -431,5 +431,6 @@ class verifier(object):
>                       rp = fl.renamed(n)
>                       if l != fl.size(i):
> -                        if len(fl.revision(n)) != fl.size(i):
> +                        # the "L1 == L2" check
> +                        if len(fl.revision(n, raw=True)) != fl.rawsize(i):
>                               self.err(lr, _("unpacked size is %s, %s expected") %
>                                        (l, fl.size(i)), f)
>
Yuya Nishihara - April 2, 2017, 7:33 a.m.
On Thu, 30 Mar 2017 14:51:15 -0700, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1490824154 25200
> #      Wed Mar 29 14:49:14 2017 -0700
> # Node ID 35902a0e3f38c766666a66d0dfbf76ec72091832
> # Parent  17b41390f4912a4c18538d778837bc2cf4a1be92
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 35902a0e3f38
> verify: fix length check

Looks good. I'll queue it.

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

Perhaps we can get rid of 'l != fl.size(i)', which just checks API-level
inconsistency. This was introduced at e79a8f36c2a5, which said "check
unpacked size field." And IIUC, rawsize() is the unpacked size field.
Jun Wu - April 3, 2017, 12:24 a.m.
Excerpts from Yuya Nishihara's message of 2017-04-02 16:33:38 +0900:
> On Thu, 30 Mar 2017 14:51:15 -0700, Jun Wu wrote:
> > # HG changeset patch
> > # User Jun Wu <quark@fb.com>
> > # Date 1490824154 25200
> > #      Wed Mar 29 14:49:14 2017 -0700
> > # Node ID 35902a0e3f38c766666a66d0dfbf76ec72091832
> > # Parent  17b41390f4912a4c18538d778837bc2cf4a1be92
> > # Available At https://bitbucket.org/quark-zju/hg-draft 
> > #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r 35902a0e3f38
> > verify: fix length check
> 
> Looks good. I'll queue it.
> 
> > diff --git a/mercurial/verify.py b/mercurial/verify.py
> > --- a/mercurial/verify.py
> > +++ b/mercurial/verify.py
> > @@ -431,5 +431,6 @@ class verifier(object):
> >                      rp = fl.renamed(n)
> >                      if l != fl.size(i):
> > -                        if len(fl.revision(n)) != fl.size(i):
> > +                        # the "L1 == L2" check
> > +                        if len(fl.revision(n, raw=True)) != fl.rawsize(i):
> >                              self.err(lr, _("unpacked size is %s, %s expected") %
> >                                       (l, fl.size(i)), f)
> 
> Perhaps we can get rid of 'l != fl.size(i)', which just checks API-level
> inconsistency. This was introduced at e79a8f36c2a5, which said "check
> unpacked size field." And IIUC, rawsize() is the unpacked size field.

I was aware of the condition is wrong. I'm thinking about a new "raw" flag
to control whether to verify non-raw contents. So people can choose a full
verify which requires downloading all large files, or just a shallow verify
skipping them.

I limited flag processor patches to a minimal to get the most critical bugs
fixed first. Now I can send nice-to-have follow-ups. Thanks!

Patch

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