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

From: Lea Wiemann <lewiemann@gmail.com>
Date: Jul 11, 2008, 14:07 UTC
Message-ID: <4877691E.1010000@gmail.com>
In-Reply-To: <200807111133.11662.jnareb@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.

Show 5 quoted lines

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

Show 7 quoted lines

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