On Thu, 23 Dec 2010, Jonathan Nieder wrote:
> Jakub Narebski wrote:
>
> > Gitweb assumes here that exceptions thrown by Perl would be simple
> > strings; die_error() throws hash reference (if not for minimal
> > external dependencies, it would be probable object of Class::Exception
> > or Throwable class thrown).
>
> Hmm, why not throw an object of new type Gitweb::Exception?
First, 'gitweb: Prepare for splitting gitweb' commit is only later in
series... ;-) but that of course is not a serious issue.
Second, more important is that I'd rather gitweb doesn't go "reinvent
the wheel" route. I'd rather (re)use Exception::Class (like e.g.
SVN::Web does it) if we go the OO exception handling route.
But if we are going to use Exception::Class, then we can also use
Try::Tiny, I think.
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -1045,21 +1045,6 @@ sub configure_gitweb_features {
> > }
> > }
> >
> > -# custom error handler: 'die <message>' is Internal Server Error
> > -sub handle_errors_html {
> > - my $msg = shift; # it is already HTML escaped
> > -
> > - # to avoid infinite loop where error occurs in die_error,
> > - # change handler to default handler, disabling handle_errors_html
> > - set_message("Error occured when inside die_error:\n$msg");
> > -
> > - # you cannot jump out of die_error when called as error handler;
> > - # the subroutine set via CGI::Carp::set_message is called _after_
> > - # HTTP headers are already written, so it cannot write them itself
> > - die_error(undef, undef, $msg, -error_handler => 1, -no_http_header => 1);
> > -}
> > -set_message(\&handle_errors_html);
> > -
>
> Hoorah!
Yeah, that is very nice.
> > # dispatch
> > sub dispatch {
> > if (!defined $action) {
> > @@ -1167,7 +1152,11 @@ sub run {
> > $pre_dispatch_hook->()
> > if $pre_dispatch_hook;
> >
> > - run_request();
> > + eval { run_request() };
> > + if (defined $@ && !ref($@)) {
Ooops, it should be 'if ($@ ...)', not 'if (defined $@ ...)'.
> > + # some Perl error, but not one thrown by die_error
> > + die_error(undef, undef, $@, -error_handler => 1);
> > + }
>
> The !ref($@) seems overzealous, which is why I am wondering if it
> would be possible to use bless() for a finer-grained check.
You meant Scalar::Util::blessed here, isn't it? Fortunately Scalar::Util
is core Perl module.
By 'overzealous' do you mean here possibility of catching what we
shouldn't, i.e. non-gitweb error (not thrown by die_error)? We can
narrow it to "ref($@) eq 'HASH'", but I don't think it would be ever
necessary: Perl throws string exceptions.
> >
> > DONE_REQUEST:
> > $post_dispatch_hook->()
> > @@ -3768,7 +3757,8 @@ EOF
> > print "</div>\n";
> >
> > git_footer_html();
> > - goto DONE_REQUEST
> > +
> > + die {'status' => $status, 'error' => $error}
> > unless ($opts{'-error_handler'});
>
> Is the DONE_REQUEST label still needed?
No it isn't.
> Thanks, I am happy to see the semantics becoming less thorny.
Now I should check if this doesn't affect gitweb performance too badly.
IIRC I have chosen 'goto DONE_GITWEB' because I didn't know about
ModPerl::Registry redefining 'exit' (why it was done), and because of
some microbenchmark showing that it performs better than die/eval (why
this specific solution)...
But I think that the performance hit would be negligible in practice;
making gitweb more maintainable is I think worth the cost.
--
Jakub Narebski
Poland
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html