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

Hi --

On 7 October 2010 00:00, Jakub Narebski [off-list ref] 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.

quoted

quoted

+       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.   :)

quoted

quoted

+               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.

quoted

quoted

+       # 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.

quoted

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.

quoted

{
        ....
}

Wouldn't it be easier to eval{} this and check $@?

So the answer is no.

Fantastic!   :)

-- Thomas Adam