Re: [PATCHv4] gitweb: parse parent..current syntax from pathinfo

[Jakub Narebski <jnareb@gmail.com>]

On Thu, 2 Oct 2008, Giuseppe Bilotta wrote:

> This makes it possible to use an URL such as
> $project/somebranch..otherbranch:/filename to get a diff between
> different version of a file. Paths like
> $project/$action/somebranch:/somefile..otherbranch:/otherfile are parsed
> as well.
>

In short, it allows to have link to '*diff' views using path_info URL,
or in general to pass $hash_[parent_]base and $file_parent using
path_info.
 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl |   26 ++++++++++++++++++++++++--
>  1 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 3e5b2b7..89e360f 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -534,7 +534,9 @@ if ($path_info && !defined $action) {
>  
>  # we can now parse ref and pathnames in PATH_INFO
>  if ($path_info) {
> -	my ($refname, $pathname) = split(/:/, $path_info, 2);
> +	$path_info =~ /^((.+?)(:(.+))?\.\.)?(.+?)(:(.+))?$/;
> +	my ($parentrefname, $parentpathname, $refname, $pathname) = (
> +		$2, $4, $5, $7);

Style: I would use (but that is perhaps matter of taste)

+	my ($parentrefname, $parentpathname, $refname, $pathname) =
+		($2, $4, $5, $7);

Also it would be I think simpler to use instead non-catching grouping,
i.e. (?: xxx ) extended pattern (see perlre(1)), and use 
($1, $2, $3, $4), or even simpler  'list = (string =~ regexp)'  form.  


I also think that the situation is more complicated than that, if we
want to be more correct.  

The following path_info layouts with '..' make sense:

  hpb:fp..hb:f
  hpb..hb:f     == hpb:f..hb:f
  hp..h

And the layout below can be though to make sense, but it is just
plain weird.

  hpb:fp..f     == hpb:fp..HEAD:f
  

>  	if (defined $pathname) {
>  		# we got "project.git/action/branch:filename" or "project.git/action/branch:dir/"
>  		# we could use git_get_type(branch:pathname), but it needs $git_dir
> @@ -543,7 +545,11 @@ if ($path_info) {
>  			$input_params{'action'} ||= "tree";
>  			$pathname =~ s,/$,,;
>  		} else {
> -			$input_params{'action'}  ||= "blob_plain";
> +			if ($parentrefname) {
> +				$input_params{'action'} ||= "blobdiff_plain";
> +			} else {
> +				$input_params{'action'} ||= "blob_plain";
> +			}

Good catch.

>  		}
>  		$input_params{'hash_base'} ||= $refname;
>  		$input_params{'file_name'} ||= $pathname;
> @@ -553,6 +559,22 @@ if ($path_info) {
>  		$input_params{'hash'}      ||= $refname;
>  		$input_params{'hash_base'} ||= $refname;
>  	}
> +	# the parent part might be missing the pathname, in which case we use the $file_name, if present
> +	if (defined $parentrefname) {
> +		$input_params{'hash_parent_base'} ||= $parentrefname;
> +		if ($parentpathname) {
> +			$parentpathname =~ s,^/+,,;
> +			$parentpathname =~ s,/$,,;
> +			$input_params{'file_parent'} ||= $parentpathname;
> +		} else {
> +			$input_params{'file_parent'} ||= $input_params{'file_name'};
> +		}
> +		if (defined $input_params{'file_parent'}) {
> +			$input_params{'hash_parent'} ||= git_get_hash_by_path($input_params{'hash_parent_base'}, $input_params{'file_parent'});

This line is bit long, and I think it should be wrapped..

> +		} else {
> +			$input_params{'hash_parent'} ||= $parentrefname;
> +		}
> +	}
>  }

-- 
Jakub Narebski
Poland