Re: [PATCH 0/9] Gitweb caching v5

From: Jakub Narebski <jnareb@gmail.com>
Date: Jan 15, 2010, 01:40 UTC
Message-ID: <m37hrkdu4k.fsf@localhost.localdomain>
In-Reply-To: <1263432185-21334-1-git-send-email-warthog9@eaglescrag.net>

"John 'Warthog9' Hawley" <warthog9@eaglescrag.net> writes:

Show 10 quoted lines

> Afternoon everyone,
>  
> This is the latest incarnation of gitweb w/ caching.  This is
> finally at the point where it should probably start either being
> considered for inclusion or mainline, or I need to accept that this
> will never get in and more perminantely fork (as is the case with
> Fedora where this is going in as gitweb-caching as a parrallel rpm
> package).
> 
> That said this brings the base up to mainline (again),

Could you tell us which commit is the base of this series (like in git-request-pull output), i.e. which commit this series is rebased against?

> it updates a
> number of elements in the caching engine, and this is a much cleaner
> break-out of the tree vs. what I am currently developing against.

Is caching engine part changed since v2?

> v5:
> 	- Missed a couple of things that were in my local tree, and
> 	  added them back in.

That doesn't tell us much.

Show 11 quoted lines

> 	- Split up the die_error and the version matching patch
> 	- Set version matching to be on by default - otherwise this
> 	  really is code that will never get checked, or at best
> 	  enabled by default by distributions
> 	- Added a minor code cleanup with respect to $site_header
> 	  that was already in my tree
> 	- Applied against a more recent git tree vs. 1.6.6-rc2
> 	- Removed breakout patch for now (did that in v4 actually)
> 	  and will deal with that separately 
> 
> 	http://git.kernel.org/?p=git/warthog9/gitweb.git;a=shortlog;h=refs/heads/gitweb-ml-v5

 
----
Short comments about patches in this series; I will be sending
detailed comments for each patch individually.

> John 'Warthog9' Hawley (9):
>   gitweb: Load checking

Looks good.

>   gitweb: change die_error to take "extra" argument for extended die
>     information

Commit message could be better (summary should really be shorter), and I think there is some indent typo, but otherwise looks good.

>   gitweb: Add option to force version match

For me it needs to be disabled in gitweb test suite (t/gitweb-lib.sh), if it is enabled by default. I really like that I can test current gitweb without need to recompile git.

Also it should have tests that it works as intended (both for matching and non-matching versions) in t/t9501-gitweb-standalone-http-status.sh

>   gitweb: Makefile improvements

Does it differ from my proposal (i.e. gitweb/Makefile doing the work), based on your idea ("make gitweb" for Makefile and gitweb/Makefile)?

>   gitweb: add a get function to compliment print_local_time
>   gitweb: add a get function to compliment print_sort_th

Those two looks O.K. from what I seen.

>   gitweb: cleanup error message produced by undefined $site_header

Shouldn't there be such protection for other such variables, like $site_footer and $home_text (and a bit diferent protection against undefined $projects_list)? By the way, how did you arrived at undefined $site_header: deafult build configuration leaves it empty, but defined.

>   gitweb: Convert output to using indirect file handle

I have alternate solution, using shorter filehandle name (just $out) in

  git://repo.or.cz/git/jnareb-git.git gitweb/cache-kernel
  http://repo.or.cz/w/git/jnareb-git.git/shortlog/refs/heads/gitweb/cache-kernel

I would have to think a bit about separate handle for binary files; I am not sure if it is really required.

>   gitweb: File based caching layer (from git.kernel.org)

I am working (time permitting) in spliting this large code drop into smaller commits, namely:

 - href(..., -path_info => 0)          (for cache key)
 - simple file based caching + tests
 - global expire time + tests
 - output caching in gitweb            (WIP)
 - adaptive expiration time            (planned)
 - tee output / cache write            (planned)
 - expire time variation from CHI      (planned)
 - locking for single writer           (planned)
 - server-side generating info         (planned)
 - AJAX-y generating info              (wishlist)

while ensuring that it pass all existing gitweb tests, and adding new tests for new features.

-- 
Jakub Narebski
Poland
ShadeHawk on #git