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

[Jakub Narebski <jnareb@gmail.com>]

On Fri, 11 July 2008, Lea Wiemann wrote:

> As follow-ups to this message, I'll be sending three patches for
> 
> 1) adding the Mechanize tests,

Somehow I didn't get Cc-ed this patch...

> The Mechanize tests succeed before and after patch (3) is applied, so
> I'm reasonably confident that my refactoring didn't introduce any
> (major) bugs.

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?

> On my to-do list:
> 
> - Benchmarks.  I'm planning to time a replay of kernel.org's gitweb logs
> on the test server, with and without caching.  Nothing fancy.  (The
> performance of the test setup on odin3.kernel.org is not representative
> of gitweb's actual performance under load.)

Do you plan to compare other gitweb caching implementations? I mean
here old kernel.org caching by J.H., and repo.or.cz caching (only for
projects list page) by Pasky.

> - Implement support for Last-Modified or ETags, since those basically
> fall out for free with the current implementation.  (This will require
> mod_perl, since CGI doesn't allow for accessing arbitrary request
> headers AFAIK.)

>From CGI(3pm):

       http()
           Called with no arguments returns the list of HTTP environment vari-
           ables, including such things as HTTP_USER_AGENT, HTTP_ACCEPT_LANGUAGE,
           and HTTP_ACCEPT_CHARSET, corresponding to the like-named HTTP header
           fields in the request.  Called with the name of an HTTP header field,
           returns its value.  Capitalization and the use of hyphens versus under-
           scores are not significant.

           For example, all three of these examples are equivalent:

              $requested_language = http('Accept-language');
              $requested_language = http('Accept_language');
              $requested_language = http('HTTP_ACCEPT_LANGUAGE');


>From http://hoohoo.ncsa.uiuc.edu/cgi/env.html

  In addition to these, the header lines received from the client, if
  any, are placed into the environment with the prefix HTTP_ followed
  by the header name. Any '-' characters in the header name are changed
  to '_' characters. The server may exclude any headers which it has
  already processed, such as Authorization, Content-type, and
  Content-length. If necessary, the server may choose to exclude any
  or all of these headers if including them would exceed any system
  environment limits.

So you _can_ access 'If-Modified-Since', 'If-None-Match' (by web browsers)
and 'If-Not-Modified-Since', 'If-Match' (by caches) from CGI.

> That will make the site a tad more responsive, I hope,
> and it will also hugely reduce the load for RSS/Atom requests, which
> currently make up almost half of all requests to kernel.org's gitweb and
> get served in full each time (i.e. "200 OK" instead of "304 Not Modified").

Currently only one "shortcut" is that gitweb respects HEAD request
(returning only HTTP headers) for feeds, and if I remember correctly
only for feeds.  I don't know however how much work gitweb does before
HEAD request shortcut.

> - Make gitweb use more parts of the Git::Repo API; in particular, the
> commit and tag parsing code should be ripped out, and gitweb should use
> the (much prettier) Git::Commit/Git::Tag API instead.  Perhaps some more
> functions (like ls_tree) can be generalized into the API as well; I went
> the easy route for now and simply replaced most "open '-|'" calls with
> $repo->cmd_output calls.

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


I'll try to review the rest of patches by tomorrow...
-- 
Jakub Narebski
Poland