Re: [PATCHv5 03/17] gitweb/lib - Very simple file based cache

From: Thomas Adam <thomas@xteddy.org>
Date: Oct 6, 2010, 23:12 UTC
Message-ID: <AANLkTikVmHgk=jLCs11tJ7yhUL4H2gnM_pKkXOJSxYq7@mail.gmail.com>
In-Reply-To: <201010070100.33478.jnareb@gmail.com>

Hi --

On 7 October 2010 00:00, Jakub Narebski <jnareb@gmail.com> wrote:

> Thank you very much for those comments on code.

Sure.

> On the other hand this way scope where "no strict 'refs';" is active
> is limited... but I guess having "no strict 'refs';" outside loop would
> be better.

Definitely.

Show 10 quoted lines

>>
>> > +       my $file = $self->path_to_key($key);
>> > +       return undef unless (defined $file && -f $file);
>>
>> PBP (Perl Best Practises) will tell you that explicitly returning
>> undef is discouraged -- "undef" should be reserved for those errors
>> you cannot handle, not ones you don't want to.
>
> Well, Perl Best Practices are practices; sometimes there is good reason
> to not take them into account (though probably not in this case).

Do not ever underestimate Conway's book, Jakub -- seriously. It's not a Bible, sure, but to ignore it or otherwise eschew its advise is to shoot yourself in the foot. :)

> I should really have run gitweb, caching modules and tests through
> perlcritic...

And your other patches.   :)

Show 15 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.

Show 11 quoted lines

>> > +       # ensure that directory leading to cache file exists
>> > +       if (!-d $dir) {
>> > +               eval { mkpath($dir, 0, 0777); 1 }
>> > +                       or die "Couldn't create leading directory '$dir' (mkpath): $!";
>> > +       }
>>
>> Why is this eval()ed?  It will still return false and set $! appropriately.
>
> IIRC mkpath *dies on error*, rather than returning false.  For better
> error handling we would need to use make_path, but File::Path 2.0+
> is in [stable] core only since Perl 5.10.

Umm, maybe.   I've not verified this, but we shouldn't rely on this either.

> I don't want to code too defensively, but perhaps check for this is in
> order... though what we should do if $code is not code reference?

confess() ahem, or otherwise die() since it's a complete fail here. Appropriate logging is necessary here.

Show 5 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.

Show 7 quoted lines

>> {
>>         ....
>> }
>>
>> Wouldn't it be easier to eval{} this and check $@?
>
> So the answer is no.

Fantastic!   :)

-- Thomas Adam