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

From: Jakub Narebski <jnareb@gmail.com>
Date: Nov 24, 2010, 21:43 UTC
Message-ID: <201011242243.20545.jnareb@gmail.com>
In-Reply-To: <7vhbf6txi9.fsf@alter.siamese.dyndns.org>

On Wed, 24 Nov 2010, Junio C Hamano wrote:

Show 15 quoted lines

> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > The default value for $per_request_config is 1 (true), which means that
> > old configuration that require to change per session (like gitolite's)
> > will keep working.
> >
> > Because there is a call to evaluate_gitweb_config() in the run() subroutine
> > (the call in run_config() is not invoked at first request to avoid double
> > running it), therefore evaluate_git_version() could be moved back there, and
> > is now evaluated only once.
> >
> > Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> 
> I like the "we can keep the old behaviour by default, but you can go
> fancier if you wanted to" attitude of this patch.

Thanks.

Show 9 quoted lines

> I however am not sure if this particular implementation is regression
> free.  People who wanted the per-request configuration used to be able to
> access what evaluate_uri() did while running evaluate_gitweb_config(),
> which in turn means that their custom configuration could take things like
> my-url and path-info into account, but now for the first invocation they
> cannot.  It probably is just the matter of moving some code around from
> their old custom gitweb-config to a per_request_config sub they need to
> write and point $per_request_config variable at, but that makes "setting 1
> will keep the old behaviour" a false promise, no?

Thanks for catching this; I forgot to take into account the fact that gitweb config changing with request would need request-related variables.

So instead of code flow looking like this

  evaluate_gitweb_config();
  $first_request = 1;

  while (...) {

  	if ($per_request_config) {
		if (ref($per_request_config) eq 'CODE') {
			$per_request_config->();
		} elsif (!$first_request) {
			evaluate_gitweb_config();
		}
	}

	$first_request = 0;

we would move to

  $first_request = 1;
  while (...) {

  	if ($first_request) {
		evaluate_gitweb_config();
	} elsif ($per_request_config) {
		if (ref($per_request_config) eq 'CODE') {
			$per_request_config->();
		} else {
  			evaluate_gitweb_config();
  		}
	}
	$first_request = 0;

I'll send new version soon.

P.S. if we could assume that nobody ever edits gitweb.cgi directly
to set $per_request_config to non-default value, we could probably
get rid of $first_request variable...

Show 41 quoted lines

> > @@ -1068,12 +1076,18 @@ sub reset_timer {
> >  	our $number_of_git_cmds = 0;
> >  }
> >  
> > +our $first_request = 1;
> >  sub run_request {
> >  	reset_timer();
> >  
> >  	evaluate_uri();
> > -	evaluate_gitweb_config();
> > -	evaluate_git_version();
> > +	if ($per_request_config) {
> > +		if (ref($per_request_config) eq 'CODE') {
> > +			$per_request_config->();
> > +		} elsif (!$first_request) {
> > +			evaluate_gitweb_config();
> > +		}
> > +	}
> >  	check_loadavg();
> >  
> >  	# $projectroot and $projects_list might be set in gitweb config file
> > @@ -1126,6 +1140,10 @@ sub evaluate_argv {
> >  
> >  sub run {
> >  	evaluate_argv();
> > +	evaluate_gitweb_config();
> > +	evaluate_git_version();
> > +
> > +	$first_request = 1;
> >  
> >  	$pre_listen_hook->()
> >  		if $pre_listen_hook;
> > @@ -1139,6 +1157,7 @@ sub run {
> >  
> >  		$post_dispatch_hook->()
> >  			if $post_dispatch_hook;
> > +		$first_request = 0;
> >  
> >  		last REQUEST if ($is_last_request->());
> >  	}
>

-- 
Jakub Narebski
Poland