Re: [PATCH 0/3] Gitweb caching v7

[Jakub Narebski <jnareb@gmail.com>]

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

> 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