Re: [PATCH 3/3] gitweb: use new Git::Repo API, and add optional caching

[Lea Wiemann <lewiemann@gmail.com>]

Jakub Narebski wrote:
> It was suggested to split this into separate commit

Yup; I'll probably send updated patches tomorrow night (also for patch 2/3).

>> - gitweb will check for parameter correctness more aggressively,
> 
> I understand that this change deals with treating invalid specifiers,
> which point to either object that do not exists, are ambiguous, or point 
> to object of invalid type.

Yes, that's right.  (I don't believe we have any point where ambiguity
might come up though.)

>> - Empty projects: [...]
> 
> Good.  The only thing that *might* be controversial is putting empty
> projects at the bottom of sorted by age (by last change) projects list, 
> instead of at top.

Yup; let's see if anyone objects though.  If I sort the list by "Last
Change", I usually want to see projects with recent activity, not dead
project, at the top, which is why I changed it (since I was touching
that line anyway).

>> - For HTML pages, remove the "Expires" HTTP response header, and add
>>   "Cache-Control: no-cache" instead.  This is because pages can
>>   contain dynamic content (like the subject of the latest commit)
> 
> I don't think it is a good change.

Hm; I thought transient titles could slip in (e.g. try opening the tree
of some commit and remove the hb parameter; the URL will seem cacheable,
but the page contains the title of the HEAD commit), but I can't find
any URL right now where mainline actually sets a wrong Expires header.
I'll look into it; if you don't see me posting about it again I'll
re-add the Expires header.

> Note that if caching is enabled, you can set expires to either
> time-to-expire of cache entries (simpler), or time left to live to
> invalidation of item in cache (better, but more complicated)

Gitweb's cache is actually never out-of-date, and cache invalidation
happens automatically.  It uses some (long) expiry times to guard
against non-standard modification of the repository, but it's nothing
the HTTP client should be concerned with.

>> $cache = Cache::Memcached->new( { servers => ['localhost:11211'],
> 
> IIRC you can use any Cache::Cache compatibile cache here;
> IMVHO it would be nice if this info would be also in commit message.

I'll add that.

>> $large_cache_root = '/home/lewiemann/gitweb-cache';
> 
> Errr... I understand that it is your _private_ configuration, just 
> copied here verbatim, but I don't think '/home/lewiemann/gitweb-cache'
> is a good example: '/tmp/gitweb-cache' perhaps, that I can understand.

Yup. ;-)  Or /var/cache/gitweb.

>> # Invalidate cache on changes to gitweb without version number bump;
>> # useful for development.
>> $cache_key = (stat '/home/lewiemann/gitweb')[9] . 
>>      (stat '/home/lewiemann/gitweb/gitweb.cgi')[9]; 
> 
> What should be used in production? "$cache_key = $version;"?

No, nothing.  $version is used automatically as a cache key; I'll add
that to the documentation for $cache_key.

> You can always use $ENV{'SCRIPT_FILENAME'}, or dirname of it.

That one doesn't exist with my thttpd, or any other environment variable
that'd be usable.  It's just a hack anyway, so hardcoded paths are OK.
:)  I don't think gitweb should check its own mtime by default.

>> # Display detailed cache info at the bottom of each page.
>> $page_info = 2;
> 
> Errr... what does "$page_info = <n>;" mean?

Display no (0) / short (1) / detailed (2) page (cache) info at the
bottom of each page.  It's documented in gitweb.perl.

> [Comments on patch itself in separate email, later]

Thanks!