From: Jakub Narebski <jnareb@gmail.com>
Date: Wed, 06 Oct 2010 23:32:11 GMT
Subject: Re: [PATCHv5 03/17] gitweb/lib - Very simple file based cache
Message-ID: <201010070132.12196.jnareb@gmail.com>
In-Reply-To: <AANLkTikVmHgk=jLCs11tJ7yhUL4H2gnM_pKkXOJSxYq7@mail.gmail.com>

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 ;-)

>>>> +               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);'.
 
>>> 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