| 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
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 >
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)
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)