Re: [PATHv2 8/8] gitweb: make the supported snapshot formats array global

[Jakub Narebski <jnareb@gmail.com>]

On Sun, 19 Oct 2008, Giuseppe Bilotta wrote:

> The @snapshot_fmts array, containing the list of the supported snapshot
> formats, was recreated locally in three different routines (with the
> different name @supported_fmts in one of them).
> 
> Its local generation is particularly expensive because two of the
> callers, href() and format_snapshot_links(), are often called many times
> in a single page.
> 
> Simplify code and speed up page generation by making the array global.
> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

Very good idea, although I'd prefer for this patch to come first;
it is not controversial contrary to other patches in this (sub)series
which IMHO needs some thought first.

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

> ---
>  gitweb/gitweb.perl |   21 ++++++++-------------
>  1 files changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 5fd5a1f..d1475b7 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -748,6 +748,10 @@ if (defined $searchtext) {
>  our $git_dir;
>  $git_dir = "$projectroot/$project" if $project;
>  
> +# list of supported snapshot formats
> +our @snapshot_fmts = gitweb_check_feature('snapshot');
> +@snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);
> +
>  # dispatch
>  if (!defined $action) {
>  	if (defined $hash) {
> @@ -858,11 +862,7 @@ sub href (%) {
>  			# snapshot_format should always be defined when href()
>  			# is called, but just in case some code forgets, we
>  			# fall back to the default
> -			if (!$fmt) {
> -				my @snapshot_fmts = gitweb_check_feature('snapshot');
> -				@snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);
> -				$fmt = $snapshot_fmts[0];
> -			}
> +			$fmt ||= $snapshot_fmts[0];
>  			$href .= $known_snapshot_formats{$fmt}{'suffix'};
>  			delete $params{'snapshot_format'};
>  		}
> @@ -1695,8 +1695,6 @@ sub format_diff_line {
>  # linked.  Pass the hash of the tree/commit to snapshot.
>  sub format_snapshot_links {
>  	my ($hash) = @_;
> -	my @snapshot_fmts = gitweb_check_feature('snapshot');
> -	@snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);
>  	my $num_fmts = @snapshot_fmts;
>  	if ($num_fmts > 1) {
>  		# A parenthesized list of links bearing format names.
> @@ -4898,20 +4896,17 @@ sub git_tree {
>  }
>  
>  sub git_snapshot {
> -	my @supported_fmts = gitweb_check_feature('snapshot');
> -	@supported_fmts = filter_snapshot_fmts(@supported_fmts);
> -
>  	my $format = $input_params{'snapshot_format'};
> -	if (!@supported_fmts) {
> +	if (!@snapshot_fmts) {
>  		die_error(403, "Snapshots not allowed");
>  	}
>  	# default to first supported snapshot format
> -	$format ||= $supported_fmts[0];
> +	$format ||= $snapshot_fmts[0];
>  	if ($format !~ m/^[a-z0-9]+$/) {
>  		die_error(400, "Invalid snapshot format parameter");
>  	} elsif (!exists($known_snapshot_formats{$format})) {
>  		die_error(400, "Unknown snapshot format");
> -	} elsif (!grep($_ eq $format, @supported_fmts)) {
> +	} elsif (!grep($_ eq $format, @snapshot_fmts)) {
>  		die_error(403, "Unsupported snapshot format");
>  	}
>  
> -- 
> 1.5.6.5
> 
> 

-- 
Jakub Narebski
Poland