Re: [PATCH 1/1] Don't obscure errors with custom errors only

[Guillem Jover <guillem@debian.org>]

Hi Thomas!

On Mon, 2012-03-19 at 15:33:28 +0000, Thomas Adam wrote:
> On 19 March 2012 15:28, Jonathan Nieder <jrnieder@gmail.com> wrote:
> > Thomas Adam wrote:
> >> When coming out of eval blocks and reporting on errors, make sure $@ is
> >> included as part of the textual output so that the real underlying error is
> >> reported.
> >> ---
> >>  I was recently bitten by this:
> >>
> >>  dpkg-source: error: source package format `3.0 (native)' is not supported
> >>  (Perl module Dpkg::Source::Package::V3::native is required)
> >>
> >>  Of course, that file is being required just fine, the problem turned out
> >>  to be missing File::Temp, but without including $@, I would never have
> >>  know this, due to a custom error message completely hiding this detail.
> >
> > Small nit: in this example, I think the text after the three dashes
> > makes a much better commit message than the text before them.
> 
> I can re-roll if you prefer?

Given the note below, that'd be perfect, yes.

> > [...]
> >> --- a/scripts/Dpkg/Source/Package.pm
> >> +++ b/scripts/Dpkg/Source/Package.pm
> >> @@ -233,7 +233,8 @@ sub upgrade_object_type {
> >>              $self->{'fields'}{'Format'} .= " ($variant)" if defined $variant;
> >>          }
> >>          if ($@) {
> >> -         error(_g("source package format `%s' is not supported (Perl module %s is required)"), $format, $module);
> >> +         error(_g("source package format `%s' is not supported (Perl module %s is required): %s"),
> >> +                     $format, $module, $@);
> >
> > If the Dpkg::Source::Package::V3::foo module is missing, will $@
> > mention so?  I am asking because I wonder if
> >
> >        _g("source package format '%s' is not supported: %s')
> >
> > might not be a shorter way to convey the same information.
> 
> It should do, but it depends entirely on the intent of the original
> error message, which is why I left that intact and merely appended the
> extra content from $@.

Although the current message might seem more clear, if it ends up giving
confusing information it does not serve its purpose well, so could you
rework it with the one suggested by Jonathan (notice the use of single
quotes, though), please?

thanks,
guillem