Re: [PATCHv5 03/17] gitweb/lib - Very simple file based cache
On 7 Oct 2010, Thomas Adam wrote:
> On 7 October 2010 00:00, Jakub Narebski <jnareb@gmail.com> wrote:
>> I should really have run gitweb, caching modules and tests through
>> perlcritic...
>
> And your other patches. :)
Well, you can't run (as far as I know) *patches* though perlcritic ;-)
Show 18 quoted lines
>>>> + last if $read_cnt == 0;
>>>> + $size_left -= $read_cnt;
>>>> + #last if $size_left <= 0;
>>>> + }
>>>> +
>>>> + close $read_fh
>>>> + or die "Couldn't close file '$file' opened for reading: $!";
>>>> + return $buf;
>>>> +}
>>>
>>> "use Carp;" would be more useful here, and hence croak() and confess().
>>
>> For a web application we usually do not want to have too detailed error
>> message present to client (to web browser) to avoid leaking of sensitive
>> information.
>
> Unless we use fatalstobrowser, it will still end up in Apache's error
> log. I don't see a problem here.
The problem is that we do equivalent of fatalsToBrowser, i.e. set up
error handler with 'set_message(\&handle_errors_html);'.
Show 7 quoted lines
>>> unless( defined $code and ref $code eq 'CODE' )
>>
>> "ref($code) eq 'CODE'" would be enough; 'undef' is not reference, and
>> ref(undef) returns "".
>
> Sure, but it still assumes $code is a reference, and there's no
> guarantee of that in the code path.
Yes, but if $code is not reference, then ref($code) evaluates to empty
string, and "ref($code) eq 'CODE'" comparison would fail... without any
warnings. You don't need to check upfront for "defined $code"; it is
the only thing I wanted to nitpick.
--
Jakub Narebski
Poland
Jakub Narebski <jnareb@gmail.com>