Re: [PATHv2 7/8] gitweb: embed snapshot format parameter in PATH_INFO

[Jakub Narebski <jnareb@gmail.com>]

On Sun, 19 Oct 2008, Giuseppe Bilotta wrote:

I'm sorry for the delay.

> When PATH_INFO is active, get rid of the sf CGI parameter by embedding
> the snapshot format information in the PATH_INFO URL, in the form of an
> appropriate extension.

The question is: should we use format suffix (e.g. 'tar.gz'),
or format name (e.g. 'tgz')?  The latter is later easier to parse,
see comments to first patch in better snapshot support for path_info.

> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index e9e9e60..5fd5a1f 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -795,6 +795,7 @@ sub href (%) {
>  		#   - action
>  		#   - hash_parent or hash_parent_base:/file_parent
>  		#   - hash or hash_base:/file_name
> +		#   - the snapshot_format as an appropriate suffix
>  
>  		# When the script is the root DirectoryIndex for the domain,
>  		# $href here would be something like http://gitweb.example.com/

Good.

> @@ -806,6 +807,10 @@ sub href (%) {
>  		$href .= "/".esc_url($params{'project'}) if defined $params{'project'};
>  		delete $params{'project'};
>  
> +		# since we destructively absorb parameters, we keep this
> +		# boolean that remembers if we're handling a snapshot
> +		my $is_snapshot = $params{'action'} eq 'snapshot';
> +

Side note: we destructively absorb parameters, because parameters
which are not absorbed are then used to generate query string part
of URL.

Deleting parameter but remembering the fact that it was used is one
(but not only) solution.

>  		# Summary just uses the project path URL, any other action is
>  		# added to the URL
>  		if (defined $params{'action'}) {
> @@ -845,6 +850,22 @@ sub href (%) {
>  			$href .= esc_url($params{'hash'});
>  			delete $params{'hash'};
>  		}
> +
> +		# If the action was a snapshot, we can absorb the
> +		# snapshot_format parameter too
> +		if ($is_snapshot) {
> +			my $fmt = $params{'snapshot_format'};
> +			# 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];
> +			}

I anderstand that the above code is improved with new patch?

> +			$href .= $known_snapshot_formats{$fmt}{'suffix'};

Again: should we use snapshot prefix, or snapshot name, which means here
do we use $known_snapshot_formats{$fmt}{'suffix'}; or just $fmt; ?

> +			delete $params{'snapshot_format'};
> +		}
>  	}
>  
>  	# now encode the parameters explicitly
> -- 
> 1.5.6.5
> 
> 

-- 
Jakub Narebski
Poland