[PATCH 3 of 3] bundle2: ignore errors seeking a bundle after an exception (issue4784)

[Yuya Nishihara]

On Sun, 16 Apr 2017 12:03:25 -0700, Gregory Szorc wrote:
> On Sun, Apr 16, 2017 at 11:58 AM, Gregory Szorc <gregory.szorc at gmail.com>
> wrote:
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc at gmail.com>
> > # Date 1492368908 25200
> > #      Sun Apr 16 11:55:08 2017 -0700
> > # Node ID 9ab4130a8302b32f7c86171a669e2ea2bfdd496b
> > # Parent  420b7094137e851132849b3cc8ddecf255a09bcb
> > bundle2: ignore errors seeking a bundle after an exception (issue4784)

> > diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
> > --- a/mercurial/bundle2.py
> > +++ b/mercurial/bundle2.py
> > @@ -354,9 +354,19 @@ def processbundle(repo, unbundler, trans
> >          for nbpart, part in iterparts:
> >              _processpart(op, part)
> >      except Exception as exc:
> > -        for nbpart, part in iterparts:
> > -            # consume the bundle content
> > -            part.seek(0, 2)
> > +        # Any exceptions seeking to the end of the bundle at this point
> > are
> > +        # almost certainly related to the underlying stream being bad.
> > +        # And, chances are that the exception we're handling is related to
> > +        # getting in that bad state. So, we swallow the seeking error and
> > +        # re-raise the original error.
> > +        seekerror = False
> > +        try:
> > +            for nbpart, part in iterparts:
> > +                # consume the bundle content
> > +                part.seek(0, 2)
> > +        except Exception:

Nit: I prefer not swallowing Exception which may shadow trivial bugs where
NameError, AttributeError, etc. would be raised.

> > +            seekerror = True
> >
> I'm not super thrilled about swallowing the inner exception. We could raise
> a special exception type that can represent multiple errors and have the
> error format being something like "in addition, an error occurred when
> seeking to the end of the bundle: %s." We could exclude IOError and
> PeerTransportError from reporting the inner exception, possibly only if the
> original error is one of these.

Perhaps.

> I'm not sure what logic is best here. But as this patch stands, we report
> the original error in all cases, which feels strictly better than what we
> did before. The question is whether the inner exception holds any value and
> is worth reporting.

Yeah, I agree.

> > +        # Re-raising from a variable loses the original stack. So only use
> > +        # that form if we need to.
> > +        if seekerror:
> > +            raise exc
> > +        else:
> > +            raise

We could save the original exception by sys.exc_info() and re-raise it, but
Python 3 made that slightly difficult.