Re: [PATCH 0/6] Gitweb caching changes v2

[Jakub Narebski <jnareb@gmail.com>]

On Fri, 11 Dec 2009, J.H. (John 'Warthog9' Hawley) wrote:
> Jakub Narebski wrote:
>> "John 'Warthog9' Hawley" <warthog9@kernel.org> writes:

>>> John 'Warthog9' Hawley (6):
>>>   GITWEB - Load Checking
>>>   GITWEB - Missmatching git w/ gitweb
>>>   GITWEB - Add git:// link to summary pages
>>>   GITWEB - Makefile changes
>>>   GITWEB - File based caching layer
>> 
>> This patch didn't made it to git mailing list.  I suspect that you ran
>> afoul vger anti-SPAM filter.
>> 
>> Does this "File based caching layer" have anything common with GSoC
>> 2008 project, available at git://repo.or.cz/git/gitweb-caching.git ?
> 
> Yeah, it does seem that way (like I said eaten by a grue), it 
> *currently* has nothing to do with Lea's GSoC code but it is still my 
> intention, long term, to integrate the two.
> 
> The patch, in all it's glory can be viewed at: 
> http://git.kernel.org/?p=git/warthog9/gitweb.git;a=commitdiff;h=42641b1e3bfae14d5cc2e0150355e89cb87951db
> 
> It is anything but a small patch to gitweb, the patch is 117K and 
> comprises 3539 lines (including git header commit information).  There's 
> not any real good way to break it up as it's a bit of an all or nothing 
> patch.

First, why do you reinvent the wheel instead of using one of existing
caching interfaces like CHI or Cache::Cache (perhaps creating a custom
backend or middle layer which incorporates required features, like being
load-aware)?  This way changing from file-based cache to e.g. mmap based
one or to memcached would be very simple.  And you would avoid pitfals
in doing your own cache management.  perl-Cache-Cache should be available
package in extras repositories.

If module is no available this would simply mean no caching, like in many
(or not so many) other cases with optional features in gitweb.


Second, if you can't use CGI::Cache directly, you can always steal the
idea from it, then the change to gitweb itself would be minimal:

  "Internally, the CGI::Cache module ties the output file descriptor
  (usually STDOUT) to an internal variable to which all output is saved."


P.S. I'll postpone critique of the patch itself for now.  The above issues
are much more important.

-- 
Jakub Narebski
Poland