Re: [PATCH 0/3] Git::Repo API and gitweb caching

[Lea Wiemann <lewiemann@gmail.com>]

Jakub Narebski wrote:
> On Fri, 11 July 2008, Lea Wiemann wrote:
>> 1) adding the Mechanize tests,
> 
> Somehow I didn't get Cc-ed this patch...

Yup, nobody got Cc'ed; apologies.

> If I remember correctly Mechanize tests detected some bugs in gitweb
> (nice!), but there were swept under the rug, i.e. put as TODO.
> 
> Does that mean that those errors were corrected, or that refactoring
> "just" didn't break anything more?

Refactoring really just didn't break anything more; it could be that
something got corrected accidentally, but I'd be surprised.

>> - Benchmarks.
> 
> Do you plan to compare other gitweb caching implementations? [k.org, repo.or.cz]

Yup, sure.

>> - Implementing support for Last-Modified or ETags [...] will require
>> mod_perl, since CGI doesn't allow for accessing arbitrary request headers
> 
>               $requested_language = http('Accept-language');
> 
>   the header lines received from the client, if any, are placed into the
>   environment with the prefix HTTP_ followed by the header name.

Right, you'd think the request headers should be accessible this way,
but apparently not all of them are.

If you take this script, ...

#!/usr/bin/perl
use CGI qw(http);
print "Content-type: text/plain\n";
print "Last-Modified: Thu, 03 Jul 2008 22:39:42 GMT\n\n";
print "Header: ", http('If-Last-Modified');

... then my browser (according to LiveHTTPHeaders) sends an
If-Last-Modified header, but it doesn't get through to the CGI script.
It does work if you test Accept-Language.  (Try print `env` to get an
idea of what gets through.)  It happens on Apache and thttpd.
Apparently this part somehow applies:

>   'The server may exclude any headers which it has already processed,'

I honestly have no idea why the If-Last-Modified headers gets eaten (and
googling didn't help), but I assume that at least it'll be possible to
access all headers with mod_perl.

> 'If-Not-Modified-Since', 'If-Match' (by caches)

Wait, are you sure caches would use those headers (I believe only the
latter actually exists BTW), or did you fall prey to a thinko? ;)

> one "shortcut" is that gitweb respects HEAD request
> (returning only HTTP headers) for feeds

Yes, and I think it does help performance-wise, but only a really small
fraction of the RSS/Atom requests actually use HEAD.  Most use GET.

> I think that ls_tree and git-ls-tree output parsing should be
> generalized into Git::Tree API as well.

True, though I'm still not sure how to make element access work pretty
and fast.  I'll keep pondering it for a while.

> I'll try to review the rest of patches by tomorrow...

Thanks!

Johannes Schindelin wrote:
> FWIW there are a few reasons why splitting up (3) might be the thing
> you really want to do

I've put splitting it on my list; I'm not sure though if I'll get around
to doing it today.  Everyone, comments on patch (3) are still
appreciated in the meantime. ;-)  I'll integrate any suggestions/patches
when I split it up.

-- Lea