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

From: Lea Wiemann <lewiemann@gmail.com>
Date: Jul 14, 2008, 23:03 UTC
Message-ID: <487BDB34.7010002@gmail.com>
In-Reply-To: <200807142323.22761.jnareb@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).

Show 5 quoted lines

>> - 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.)

Show 5 quoted lines

>> - 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).

Show 5 quoted lines

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

Show 5 quoted lines

>> $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.

Show 6 quoted lines

>> # 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!