Re: [RFC PATCHv3 00/10] gitweb: Simple file based output caching

["J.H." <warthog9@eaglescrag.net>]

> Shortlog:
> ~~~~~~~~~
> Jakub Narebski (10):
>   gitweb: href(..., -path_info => 0|1)

This still looks fine to me.

>   gitweb/cache.pm - Very simple file based cache

This looks fine to me, and I'm much more keen on using the CHI interface.

>   gitweb/cache.pm - Stat-based cache expiration

- I think having expires time at -1 (never) is a dangerous default.  I
think having it as an option is fine, but setting this to something like
6 months may be a better option.  I wouldn't expect this to be an issue
in reality, but I can just see someone screwing up and having a cache
system that *never* expires.  I think I'd rather see something long vs.
never.  But that's just my opinion.

- This does have the problem (though this gets fixed later on, again)
that it would return invalid if there's a process already updating the
cache.  This might need to be fixed later to understand the locking
structures on what is/isn't valid (this is mostly a note for me while I
read through the patches)

>   gitweb: Use Cache::Cache compatibile (get, set) output caching

- It might be worth (in a later patch) enabling the PerlIO layers as
there was a significant speed improvement in using them, despite their
non-standardness.

- What if you want to use a different caching library but don't want
cache.pm ?  That first bit of "if ($caching_enabled) {" might need to be
wrapped to figure out if we are using our inbuilt caching or an external
caching system. (just thinking out loud)

Looks fine to me.

>   gitweb/cache.pm - Adaptive cache expiration time

is the commented:

#return &{$self->{'check_load'}}();

intended in check_load() ?

otherwise this looks fine to me.

>   gitweb: Use CHI compatibile (compute method) caching

Looks fine to me.  I think it's fine where it's at in the order myself.

>   gitweb/cache.pm - Use locking to avoid 'cache miss stampede' problem

Looks fine to me.

>   gitweb/cache.pm - Serve stale data when waiting for filling cache

Looks fine to me.

>   gitweb/cache.pm - Regenerate (refresh) cache in background

Looks good to me.

>   gitweb: Show appropriate "Generating..." page when regenerating cache

I've got a couple of things that will need to be added on top of this
(mainly to try and guess if the client is dumb or smart) so that if it
won't support generating... as expected it doesn't get it.  But that can
come in a later patch.

This looks fine to me.

> 
> Diffstat:
> ~~~~~~~~~
>  gitweb/README                          |   70 ++++
>  gitweb/cache.pm                        |  655 ++++++++++++++++++++++++++++++++
>  gitweb/gitweb.perl                     |  321 +++++++++++++++-
>  t/gitweb-lib.sh                        |    2 +
>  t/t9500-gitweb-standalone-no-errors.sh |   19 +
>  t/t9503-gitweb-caching.sh              |   32 ++
>  t/t9503/test_cache_interface.pl        |  404 ++++++++++++++++++++
>  t/test-lib.sh                          |    3 +
>  8 files changed, 1486 insertions(+), 20 deletions(-)
>  create mode 100644 gitweb/cache.pm
>  create mode 100755 t/t9503-gitweb-caching.sh
>  create mode 100755 t/t9503/test_cache_interface.pl
>