Re: [PATCH 0/3] Gitweb caching v7

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 31, 2010, 04:24 UTC
Message-ID: <7v1v773s7e.fsf@alter.siamese.dyndns.org>
In-Reply-To: <m3k4l0girs.fsf@localhost.localdomain>

Jakub Narebski <jnareb@gmail.com> writes:

Show 18 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.

John, where should cache.pm go in the installed system? Does it typically go next to gitweb.perl?

I think "do 'that-file'" honors path specified by the -I option, so I do not think "do $cache_pm" is necessary. My preference is to run gitweb tests with appropriate -I pointing at the cache.pm in the directory.

Show 6 quoted lines

> ...  It should be
>
>   do $cache_pm;
>   die $@ if $@;
>
> at least.

Catching failure is a good thing to do.

> Perhaps even better would be to simply turn off caching
> support if there is an error in 'cache.pm'

That can come later.

Jakub, can we have an absolute minimum fix-up, so that we can give this wider exposure? I think there are only four issues:

 (1) exclude Ajax-y stuff from caching;
 (2) install cache.pm the same way gitweb.perl is installed via
     the Makefile;
 (3) running tests with appropriate -I so that cache.pm is found; and
 (4) die if 'cache.pm' cannot be "done").

I think the change in gitweb-cache v7 is small and safe enough that we should fast-track it to give usability to the real world sites. It may be a low-risk "obviously correct" approach that is quick-and-dirty, but that is exactly why this should be fast-tracked. It does not touch the logic or formatting in any way, just bypasses the page generation altogether when it can clearly do so when it can tell the output cannot possibly be incorrect (albeit sometimes it might be stale if in certain cases, e.g. it is relative to HEAD).

I know you and others were aiming to split things up, but I think the amount of the effort that is needed for that line of work on top of the current codebase is not much different from what is needed on top of gitweb-cache v7.