On 7 Oct 2010, Thomas Adam wrote: > On 7 October 2010 00:00, Jakub Narebski <jnareb@xxxxxxxxx> 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 -- 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