Patchwork [3,of,3,V3] localrepo: better error message when revlog error and file addition (issue4675)

login
register
mail settings
Submitter Jordi Gutiérrez Hermoso
Date June 2, 2015, 7:07 p.m.
Message ID <6299517509f9a8912ff8.1433272030@Iris>
Download mbox | patch
Permalink /patch/9444/
State Changes Requested
Headers show

Comments

Jordi Gutiérrez Hermoso - June 2, 2015, 7:07 p.m.
# HG changeset patch
# User Jordi Gutiérrez Hermoso <jordigh@octave.org>
# Date 1432239912 14400
#      Thu May 21 16:25:12 2015 -0400
# Node ID 6299517509f9a8912ff83fffa46cecf964ba779c
# Parent  3353bc00fbb14f2930cf0e9b72be312e578d2f6e
localrepo: better error message when revlog error and file addition (issue4675)

At this level we have enough context for saying which filename caused
the revlog error, and we can also add the hint from the revlog
exception, if any.
Pierre-Yves David - June 2, 2015, 7:08 p.m.
On 06/02/2015 12:07 PM, Jordi Gutiérrez Hermoso wrote:
> # HG changeset patch
> # User Jordi Gutiérrez Hermoso <jordigh@octave.org>
> # Date 1432239912 14400
> #      Thu May 21 16:25:12 2015 -0400
> # Node ID 6299517509f9a8912ff83fffa46cecf964ba779c
> # Parent  3353bc00fbb14f2930cf0e9b72be312e578d2f6e
> localrepo: better error message when revlog error and file addition (issue4675)
>
> At this level we have enough context for saying which filename caused
> the revlog error, and we can also add the hint from the revlog
> exception, if any.

Do you think we should get a test for this? (down side would be to 
create a 3Go file during test :-/)
Jordi Gutiérrez Hermoso - June 2, 2015, 7:13 p.m.
On Tue, 2015-06-02 at 12:08 -0700, Pierre-Yves David wrote:
> 
> On 06/02/2015 12:07 PM, Jordi Gutiérrez Hermoso wrote:
> > # HG changeset patch
> > # User Jordi Gutiérrez Hermoso <jordigh@octave.org>
> > # Date 1432239912 14400
> > #      Thu May 21 16:25:12 2015 -0400
> > # Node ID 6299517509f9a8912ff83fffa46cecf964ba779c
> > # Parent  3353bc00fbb14f2930cf0e9b72be312e578d2f6e
> > localrepo: better error message when revlog error and file addition (issue4675)
> >
> > At this level we have enough context for saying which filename caused
> > the revlog error, and we can also add the hint from the revlog
> > exception, if any.
> 
> Do you think we should get a test for this? (down side would be to 
> create a 3Go file during test :-/)

Right, I was thinking about that. Too many gigaoctets.

An alternative could be to monkeypatch so that revlog._maxentrysize is
a lot smaller for the duration of the test?
Pierre-Yves David - June 2, 2015, 8:05 p.m.
On 06/02/2015 12:13 PM, Jordi Gutiérrez Hermoso wrote:
> On Tue, 2015-06-02 at 12:08 -0700, Pierre-Yves David wrote:
>>
>> On 06/02/2015 12:07 PM, Jordi Gutiérrez Hermoso wrote:
>>> # HG changeset patch
>>> # User Jordi Gutiérrez Hermoso <jordigh@octave.org>
>>> # Date 1432239912 14400
>>> #      Thu May 21 16:25:12 2015 -0400
>>> # Node ID 6299517509f9a8912ff83fffa46cecf964ba779c
>>> # Parent  3353bc00fbb14f2930cf0e9b72be312e578d2f6e
>>> localrepo: better error message when revlog error and file addition (issue4675)
>>>
>>> At this level we have enough context for saying which filename caused
>>> the revlog error, and we can also add the hint from the revlog
>>> exception, if any.
>>
>> Do you think we should get a test for this? (down side would be to
>> create a 3Go file during test :-/)
>
> Right, I was thinking about that. Too many gigaoctets.
>
> An alternative could be to monkeypatch so that revlog._maxentrysize is
> a lot smaller for the duration of the test?

Sounds like a plan !
Matt Mackall - June 3, 2015, 8:20 p.m.
On Tue, 2015-06-02 at 15:07 -0400, Jordi Gutiérrez Hermoso wrote:
> # HG changeset patch
> # User Jordi Gutiérrez Hermoso <jordigh@octave.org>
> # Date 1432239912 14400
> #      Thu May 21 16:25:12 2015 -0400
> # Node ID 6299517509f9a8912ff83fffa46cecf964ba779c
> # Parent  3353bc00fbb14f2930cf0e9b72be312e578d2f6e
> localrepo: better error message when revlog error and file addition (issue4675)
> 
> At this level we have enough context for saying which filename caused
> the revlog error, and we can also add the hint from the revlog
> exception, if any.
> 
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -1348,7 +1348,10 @@ class localrepository(object):
>          text = fctx.data()
>          if fparent2 != nullid or flog.cmp(fparent1, text) or meta:
>              changelist.append(fname)
> -            return flog.add(text, meta, tr, linkrev, fparent1, fparent2)
> +            try:
> +                return flog.add(text, meta, tr, linkrev, fparent1, fparent2)
> +            except error.RevlogError as exc:
> +                raise util.Abort("%s: %s" % (exc, fname), hint=exc.hint)
>          # are just the flags changed during merge?
>          elif fname in manifest1 and manifest1.flags(fname) != fctx.flags():
>              changelist.append(fname)

Looks like the wrong layer for this. Either belongs in
revlog._addrevision (where it'll catch changelog and manifest overflows
too) or filelog.add.
Jordi Gutiérrez Hermoso - June 3, 2015, 9:21 p.m.
On Wed, 2015-06-03 at 15:20 -0500, Matt Mackall wrote:
> On Tue, 2015-06-02 at 15:07 -0400, Jordi Gutiérrez Hermoso wrote:
> > # HG changeset patch
> > # User Jordi Gutiérrez Hermoso <jordigh@octave.org>
> > # Date 1432239912 14400
> > #      Thu May 21 16:25:12 2015 -0400
> > # Node ID 6299517509f9a8912ff83fffa46cecf964ba779c
> > # Parent  3353bc00fbb14f2930cf0e9b72be312e578d2f6e
> > localrepo: better error message when revlog error and file addition (issue4675)
> > 
> > At this level we have enough context for saying which filename caused
> > the revlog error, and we can also add the hint from the revlog
> > exception, if any.
> > 
> > diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> > --- a/mercurial/localrepo.py
> > +++ b/mercurial/localrepo.py
> > @@ -1348,7 +1348,10 @@ class localrepository(object):
> >          text = fctx.data()
> >          if fparent2 != nullid or flog.cmp(fparent1, text) or meta:
> >              changelist.append(fname)
> > -            return flog.add(text, meta, tr, linkrev, fparent1, fparent2)
> > +            try:
> > +                return flog.add(text, meta, tr, linkrev, fparent1, fparent2)
> > +            except error.RevlogError as exc:
> > +                raise util.Abort("%s: %s" % (exc, fname), hint=exc.hint)
> >          # are just the flags changed during merge?
> >          elif fname in manifest1 and manifest1.flags(fname) != fctx.flags():
> >              changelist.append(fname)
> 
> Looks like the wrong layer for this. Either belongs in
> revlog._addrevision (where it'll catch changelog and manifest overflows
> too)

I thought it would make most sense to catch manifest overflows
elsewhere, not in revlog.py. We need more context in order to form an
error message about how the problem is about too many files, not a
single large file.

Which makes me think that the largefiles hint should also be added at
localrepo, but then the exception raised by revlogio should be
something more specific than RevlogError. Are we ok with
RevlogOverflow or something like it?

> or filelog.add.

There isn't a filename here to add to the error message, although I
suppose I could compute the filename from the revlog's file name. Is
there a simple way to do that, or do I need to consult the manifest?
Will there already be a manifest at this stage if the file is being
newly added?

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1348,7 +1348,10 @@  class localrepository(object):
         text = fctx.data()
         if fparent2 != nullid or flog.cmp(fparent1, text) or meta:
             changelist.append(fname)
-            return flog.add(text, meta, tr, linkrev, fparent1, fparent2)
+            try:
+                return flog.add(text, meta, tr, linkrev, fparent1, fparent2)
+            except error.RevlogError as exc:
+                raise util.Abort("%s: %s" % (exc, fname), hint=exc.hint)
         # are just the flags changed during merge?
         elif fname in manifest1 and manifest1.flags(fname) != fctx.flags():
             changelist.append(fname)