Re: [PATCH/RFC] gitweb: Fix actionless dispatch for non-existent objects

From: Jakub Narebski <jnareb@gmail.com>
Date: Jan 9, 2012, 22:05 UTC
Message-ID: <201201092305.10517.jnareb@gmail.com>
In-Reply-To: <7v8vlgef7o.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:

Show 10 quoted lines

> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > When gitweb URL does not provide action explicitly, e.g.
> >
> >   http://git.example.org/repo.git/branch
> >
> > dispatch() tries to guess action (view to be used) based on remaining
> > parameters.  Among others it is based on the type of requested object,
> > which gave problems when asking for non-existent branch or file (for
> > example misspelt name)

                          , because git_get_type() returns undef when
requested object does not exist, and $action was left undefined.

This resulted in Perl generating the "Use of unitialized value" warnings,
which made it in error.log.  Additionally gitweb returned "400 Bad Request"
error instead of more informative "404 Not Found".

Show 10 quoted lines

> 
> Ok. "gave problems" is a bit unclear to see why explicitly calling
> die_error() is an improvement, though. What is the nature of the
> "problems"? Giving a server error 500 because later codepaths tried to
> call an undefined subroutine?
> 
> > Now undefined $action from dispatch() should not result in problems.
> 
> Again, unspecified "problems" here. I'd like this sentence to end with
> "should not result in X but gives an explicit '404 not found' error".

This is about second chunk of change to gitweb/gitweb.perl, which is responsible about silencing this warning:

   gitweb.perl: Use of uninitialized value in pattern match (m//) at ../gitweb.perl line 2397.

It was present even with the '$action or die_error(404,...)' short-circut, as this was in the part responsible for generating page header, which is the same for normal page and for error page.

Perhaps better solution would be to set action to 'object' if requested object is not found (a valid action in itself). Hmmm...

-- 
Jakub Narebski
Poland