[patch] show unexpected tracebacks to stderr

[Martin Pool]

On 15 Jun 2006, John Arbash Meinel <john at arbash-meinel.com> wrote:
> Martin Pool wrote:
> > bzr.log was a nice idea, but it's a bit unobvious.

> I can see a lot of nice cleanup happening.

Thanks

> Why did you get rid of BzrOptionError in favor of BzrCommandError?

I was trying to clean up exceptions that are constructed with different
meanings depending just on the format string provided to them.  Perhaps
this was taking it too far.  

> v- PEP8 here
> 
> > -class BzrCommandError(BzrError):
> > +class BzrCommandError(BzrNewError):
> > +    """Error from user command"""
> > +    is_user_error = True
> >      # Error from malformed user command
> >      # This is being misused as a generic exception
> >      # pleae subclass. RBC 20051030
> > @@ -172,15 +195,14 @@
> >      # are not intended to be caught anyway.  UI code need not subclass
> >      # BzrCommandError, and non-UI code should not throw a subclass of
> >      # BzrCommandError.  ADHB 20051211
> 
> ...
> 

> The '%r' was in case there was a Unicode path that could not be
> displayed in the current encoding. You can chose how you want to handle
> it. But is PathError a user_error or not?
> These seem like hidden internal classes, that if one escapes, it should
> be a bug.

OK, I'll put them back to not user_error and %r.

> 
> >  
> >  class NoSuchFile(PathError):
> > -    """No such file: %(path)r%(extra)s"""
> > +    """No such file: %(path)s%(extra)s"""
> >  
> >  
> >  class FileExists(PathError):
> > -    """File exists: %(path)r%(extra)s"""
> > +    """File exists: %(path)s%(extra)s"""
> >  
> >  
> >  class DirectoryNotEmpty(PathError):
> > -    """Directory not empty: %(path)r%(extra)s"""
> > +    """Directory not empty: %(path)s%(extra)s"""
> >  
> >  
> >  class ResourceBusy(PathError):
> > -    """Device or resource busy: %(path)r%(extra)s"""
> > +    """Device or resource busy: %(path)s%(extra)s"""
> >  
> >  
> >  class PermissionDenied(PathError):
> > -    """Permission denied: %(path)r%(extra)s"""
> > +    """Permission denied: %(path)s%(extra)s"""
> >  
> >  
> >  class PathNotChild(BzrNewError):
> > -    """Path %(path)r is not a child of path %(base)r%(extra)s"""
> > +    """Path %(path)s is not a child of path %(base)s%(extra)s"""
> > +
> > +    is_user_error = False
> > +
> >      def __init__(self, path, base, extra=None):
> >          BzrNewError.__init__(self)
> >          self.path = path
> > @@ -244,7 +269,7 @@
> >  
> 
> v- PEP8 here as well
> >  class ObjectNotLocked(LockError):
> > -    """%(obj)r is not locked"""
> > +    """%(obj)s is not locked"""
> > +    is_user_error = False
> >      # this can indicate that any particular object is not locked; see also
> >      # LockNotHeld which means that a particular *lock* object is not held by
> >      # the caller -- perhaps they should be unified.
> > @@ -366,7 +388,7 @@
> >  
> 
> 

> v- Here you switch r => s, but then left another %r

OK, should probably revert that.

> >  class LockBreakMismatch(LockError):
> > -    """Lock was released and re-acquired before being broken: %(lock)s: held by %(holder)r, wanted to break %(target)r"""
> > +    """Lock was released and re-acquired before being broken: %(lock)s: held by %(holder)s, wanted to break %(target)r"""
> >      def __init__(self, lock, holder, target):
> >          self.lock = lock
> >          self.holder = holder
> > @@ -425,42 +447,38 @@
> >      """Commit refused because there are unknowns in the tree."""
> >  
> >  
> 
> 
> > === modified file 'bzrlib/tests/test_source.py'
> > --- bzrlib/tests/test_source.py	
> > +++ bzrlib/tests/test_source.py	
> > @@ -72,4 +72,4 @@
> >          # written. Note that this is an exact equality so that when the number
> >          # drops, it is not given a buffer but rather this test updated
> >          # immediately.
> > -        self.assertEqual(4, occurences)
> > +        self.assertEqual(3, occurences)
> 
> Good for us.
> 
> 
> v- I thought we weren't doing 'Authors' statements.

I'll remove it.

> 
> > === modified file 'bzrlib/tests/test_trace.py'
> > --- bzrlib/tests/test_trace.py	
> > +++ bzrlib/tests/test_trace.py	
> 
> 
> ...
> These seem like API changes that would need to be deprecated first.

I suppose they are.  It's satisfying to just pull it out but I suppose
we can have the deferred satisfaction of knowing it will be deleted in
the future.
 
> > -def log_exception(msg=None):
> > -    """Log the last exception to stderr and the trace file.
> > -
> > -    The exception string representation is used as the error
> > -    summary, unless msg is given.
> > -    """
> > -    if msg:
> > -        error(msg)
> > -    else:
> > -        exc_str = format_exception_short(sys.exc_info())
> > -        error(exc_str)
> > -    log_exception_quietly()
> > -
> > -
> >  def log_exception_quietly():
> >      """Log the last exception to the trace file only.
> >  
> 
> Does this handle the case of commands that shouldn't show anything on
> EPIPE? Probably that exception is caught in the decorator, so isn't
> important here.

Yes, I think they should catch it below this level.

> > +def report_exception(exc_info, err_file):
> > +    exc_type, exc_object, exc_tb = exc_info
> > +    if (isinstance(exc_object, IOError)
> > +        and getattr(exc_object, 'errno', None) == errno.EPIPE):
> > +        print >>err_file, "bzr: broken pipe"
> > +    elif isinstance(exc_object, KeyboardInterrupt):
> > +        print >>err_file, "bzr: interrupted"
> > +    elif getattr(exc_object, 'is_user_error', False):
> > +        report_user_error(exc_info, err_file)
> > +    elif isinstance(exc_object, (OSError, IOError)):
> > +        # Might be nice to catch all of these and show them as something more
> > +        # specific, but there are too many cases at the moment.
> > +        report_user_error(exc_info, err_file)
> > +    else:
> > +        report_bug(exc_info, err_file)
> 
> 
> A couple small things, but +1 overall.

Thankyou for the review comments.  I'll fix them up and merge it in.

-- 
Martin