Re: [PATCHv6 5/5] gitweb: generate parent..current URLs

[Jakub Narebski <jnareb@gmail.com>]

On Thu, 16 Oct 2008, Giuseppe Bilotta wrote:

> If use_pathinfo is enabled, href now creates links that contain paths in
> the form $project/$action/oldhash:/oldname..newhash:/newname for actions
> that use hash_parent etc.
> 
> If any of the filename contains two consecutive dots, it's kept as a CGI
> parameter since the resulting path would otherwise be ambiguous.

Note that this part is _required_ after previous patch, because
otherwise gitweb could generate path_info links which point to
different view than intended, when filename contains '..' in it,
like for example some test vectors in git test suite, for example
files in t/t5515

> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

For what is worth:

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


BTW. I wanted to test this using gitweb-Mechanize test from Lea Wiemann
GSoC 2008 work on gitweb caching, but I couldn't make it run as single
individual test (cd t && ./t9503-gitweb-Mechanize.sh); it requires to
run as "make test" and I didn't want that.  I could have resurrected my
initial version of Test::WWW::Mechanize::CGI test...

> ---
>  gitweb/gitweb.perl |   30 +++++++++++++++++++++++++-----
>  1 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 1a7b0b9..f4642e7 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -759,7 +759,8 @@ sub href (%) {
>  		# try to put as many parameters as possible in PATH_INFO:
>  		#   - project name
>  		#   - action
> -		#   - hash or hash_base:/filename
> +		#   - hash_parent or hash_parent_base:/file_parent
> +		#   - hash or hash_base:/file_name

Minor nit: this contain independent change 'filename' -> 'file_name',
but I think it is not worth separating...

>  
>  		# When the script is the root DirectoryIndex for the domain,
>  		# $href here would be something like http://gitweb.example.com/
> @@ -778,17 +779,36 @@ sub href (%) {
>  			delete $params{'action'};
>  		}
>  
> -		# Finally, we put either hash_base:/file_name or hash
> +		# Next, we put hash_parent_base:/file_parent..hash_base:/file_name,
> +		# stripping nonexistent or useless pieces
> +		$href .= "/" if ($params{'hash_base'} || $params{'hash_parent_base'}
> +			|| $params{'hash_parent'} || $params{'hash'});

Nice trick (and required change).

>  		if (defined $params{'hash_base'}) {
> -			$href .= "/".esc_url($params{'hash_base'});
> -			if (defined $params{'file_name'}) {
> +			if (defined $params{'hash_parent_base'}) {
> +				$href .= esc_url($params{'hash_parent_base'});
> +				# skip the file_parent if it's the same as the file_name
> +				delete $params{'file_parent'} if $params{'file_parent'} eq $params{'file_name'};
> +				if (defined $params{'file_parent'} && $params{'file_parent'} !~ /\.\./) {
> +					$href .= ":/".esc_url($params{'file_parent'});
> +					delete $params{'file_parent'};
> +				}

Side note: I wonder if we should use esc_url or esc_param here...

> +				$href .= "..";
> +				delete $params{'hash_parent'};
> +				delete $params{'hash_parent_base'};
> +			} elsif (defined $params{'hash_parent'}) {
> +				$href .= esc_url($params{'hash_parent'}). "..";
> +				delete $params{'hash_parent'};
> +			}
> +
> +			$href .= esc_url($params{'hash_base'});
> +			if (defined $params{'file_name'} && $params{'file_name'} !~ /\.\./) {
>  				$href .= ":/".esc_url($params{'file_name'});
>  				delete $params{'file_name'};
>  			}
>  			delete $params{'hash'};
>  			delete $params{'hash_base'};
>  		} elsif (defined $params{'hash'}) {
> -			$href .= "/".esc_url($params{'hash'});
> +			$href .= esc_url($params{'hash'});
>  			delete $params{'hash'};
>  		}
>  	}
> -- 
> 1.5.6.5
> 
> 

-- 
Jakub Narebski
Poland