Re: [PATCH 0/3] Gitweb caching v7

From: Jakub Narebski <jnareb@gmail.com>
Date: Oct 30, 2010, 08:58 UTC
Message-ID: <m3k4l0girs.fsf@localhost.localdomain>
In-Reply-To: <7vbp6c63gt.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

Show 6 quoted lines

> I am getting this in the gitweb.log:
> 
>     [Fri Oct 29 22:21:12 2010] gitweb.perl: Undefined subroutine 
>     &main::cache_fetch called at .../t/../gitweb/gitweb.perl line 1124.
> 
> which seems to cause t9500 to fail.

This is caused by three issues (bugs) in v7 caching code.

First is the reason why t9500 exhibits this bug. The gitweb caching v7 includes file with subroutines related to caching in the following way:

  do 'cache.pm';

Note that the relative path is used; for t9500 it is relative from somewhere witjin 't/', and not from 'gitweb/', so "do 'cache.pm';" doesn't find it.

In my earlier rewrites I used

  do $cache_pm;

and 't/gitweb-lib.sh' set $cache_pm appriopriately.

Second is why this bug is bad. There is no error checking that "do 'cache.pm';" succeeded. It should be

  do $cache_pm;
  die $@ if $@;

at least. Perhaps even better would be to simply turn off caching support if there is an error in 'cache.pm' (which probably should be called 'cache.pl', as it is not proper Perl module)... though on the other hand side it would could hide the fact that caching is not working.

Third is why this matters. In v7 the cache_fetch() subroutine, defined in 'cache.pm', is run *unconditionally*, and has test if the caching is enabled *inside it*, instead of having gitweb (caller) use it only if caching is disabled.

This coupled with the fact that 'make install-gitweb' would *not* install 'cache.pm' alongside gitweb.cgi means that anybody upgrading gitweb, whether he/she wants caching or not, would have gitweb stop working after upgrade... unless he/she knows that he/she has to copy 'cache.pm' file manually.

On the other hand having test first if caching is enabled would make t9500 tests pass (because they do not even test minimally cache enabled / cache disabled cases, like in my rewrite), but would hide problem when caching is enabled and 'cache.pm' is not installed... (perhaps also in persistent environments; I don't know where pwd is then).

-- 
Jakub Narebski
Poland
ShadeHawk on #git