Re: [PATCH/RFC] gitweb: allow configurations that change with each request

[Jakub Narebski <jnareb@gmail.com>]

On Sat, 31 Jul 2010, Jonathan Nieder wrote:

> gitolite's contrib/gitweb/gitweb.conf includes:
> 
> 	$ENV{GL_USER} = $cgi->remote_user || "gitweb";
> 
> which is useful for setups where a user has to be authenticated
> to access certain repos.  Perhaps other typical configurations
> change per session in other ways, too.
> 
> v1.7.2-rc2~6 (gitweb: Move evaluate_gitweb_config out of run_request,
> 2010-07-05) broke such configurations for a speedup, by loading
> the configuration once per FastCGI process.
> 
> Probably in the end there should be a way to specify in the
> configuration whether a particular installation wants the speedup or
> the flexibility.  But for now it is easier to just undo the relevant
> change.
> 
> This partially reverts commit 869d58813b24c74e84c9388041eafcef40cb51e4.

Why only *partially* reverts...

...ah, I see, I did "while at it" change fixing timer and number of git
commands info for persistent environments (i.e. gitweb run as FastCGI
script).
 
> Reported-by: Julio Lajara <julio.lajara@alum.rpi.edu>
> Analysis-by: Jakub Narebski <jnareb@gmail.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

Reluctantly-acked-by: Jakub Narebski <jnareb@gmail.com>

> ---
> Jakub Narebski wrote:
> 
> > I wonder if it would be possible to 
> > re-enable this feature (which I think is needed to be able to use
> > $cgi->remote_user) but without having all pay the [slight] performance
> > penalty of including (and I think parsing) config file once per each
> > request.
> 
> I dunno.  Maybe this would be a good place to start.

Currently this is non-issue; the eventual performance penalty is I guess
very small.

The problem would be with adding caching support to gitweb.  While
default GitwebCache::* caching engine should have low startup cost, 
the CHI generic caching interface that one might want to use instead
uses Moose (Mouse with Any::Moose?) for OOP, which imposes some startup
cost.  One enables and configures output caching in gitweb config, so
if gitweb config would be read once per run then cache interface could
be started once per run, not once per request.
 
Nevertheless this change is backwards incompatibile change, and should
probably wait for 1.7.3 (pity that 1.7.2 was so recently released).


One solution I can think of (still backwards incompatibile) would be to
provide $per_request_config variable, which would hold anonymous sub
with parts of config that need to be done per request (this should work
with global variables (our), but I think it wouldn't work with lexical
variables (my)).  For example gitolite's contrib/gitweb/gitweb.conf would
then include:

  $per_request_config = sub {
  	$ENV{GL_USER} = $cgi->remote_user || "gitweb";
  }

What do you think about it?

>  gitweb/gitweb.perl |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index e0e9532..300c4b1 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1060,8 +1060,12 @@ sub run_request {
>  	reset_timer();
>  
>  	evaluate_uri();
> +	evaluate_gitweb_config();
>  	check_loadavg();
>  
> +	# $projectroot and $projects_list might be set in gitweb config file
> +	$projects_list ||= $projectroot;
> +
>  	evaluate_query_params();
>  	evaluate_path_info();
>  	evaluate_and_validate_params();
> @@ -1109,12 +1113,8 @@ sub evaluate_argv {
>  
>  sub run {
>  	evaluate_argv();
> -	evaluate_gitweb_config();
>  	evaluate_git_version();
>  
> -	# $projectroot and $projects_list might be set in gitweb config file
> -	$projects_list ||= $projectroot;
> -
>  	$pre_listen_hook->()
>  		if $pre_listen_hook;
>  

That reminds me: '$projects_list ||= $projectroot;' line should be put,
I think, inside evaluate_gitweb_config().  But that of course should not
be done in this patch.

-- 
Jakub Narebski
Poland