Re: [PATCHv6 4/5] gitweb: parse parent..current syntax from PATH_INFO

[Jakub Narebski <jnareb@gmail.com>]

On Thu, 16 Oct 2008, Giuseppe Bilotta wrote:

> This patch 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.

Just a nitpick: why '$project' and '$action', but 'somebranch',
'otherbranch' and 'filename'?

> 
> All '*diff' actions and in general actions that use $hash_parent[_base]
> and $file_parent can now get all of their parameters from PATH_INFO

Which currently mean 'shortlog', and I guess in the future would mean
also all other log-like views: 'log', 'history', 'search' (the commit
message kind, not the pickaxe kind), and perhaps also 'rss'/'atom'.

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

For what is worth:

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

> ---
>  gitweb/gitweb.perl |   36 ++++++++++++++++++++++++++++++++++--
>  1 files changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 49730f3..1a7b0b9 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -548,7 +548,12 @@ sub evaluate_path_info {
>  		'history',
>  	);
>  
> -	my ($refname, $pathname) = split(/:/, $path_info, 2);
> +	# horrible regexp to catch
> +	# [$hash_parent_base[:$file_parent]..]$hash_parent[:$file_name]

This comment is really nice here, to explain what we try to catch.
Is it really "horrible"... let others decide.

Note: this (as also previous code) makes use of the fact that refname
cannot contain ':' as per git-check-ref-format(1), and the fact that
gitweb limits $hash* parameters to simple revision syntax, allowing
only SHA1 and refnames, and not allowing (see validate_refname() used
in $hash* validation) extended SHA1 syntax like <commit-ish>:<path>
for $hash ('h') param for 'blob' view: gitweb uses $hash_base and
$file_name for that.

(But I guess it is too long explanation to put it in comment)


Side note: the regexp below allow for $parentpathname to contain
'..', but we don't want to rely on such minute detail of implementation
detail (because it depends on whether we use greedy or non-greedy
matching there).

> +	my ($parentrefname, $parentpathname, $refname, $pathname) =
> +		($path_info =~ /^(?:(.+?)(?::(.+))?\.\.)?(.+?)(?::(.+))?$/);
> +
> +	# first, analyze the 'current' part
>  	if (defined $pathname) {
>  		# we got "branch:filename" or "branch:dir/"
>  		# we could use git_get_type(branch:pathname), but it needs $git_dir
> @@ -557,7 +562,13 @@ sub evaluate_path_info {
>  			$input_params{'action'} ||= "tree";
>  			$pathname =~ s,/$,,;
>  		} else {
> -			$input_params{'action'} ||= "blob_plain";
> +			# the default action depends on whether we had parent info
> +			# or not
> +			if ($parentrefname) {
> +				$input_params{'action'} ||= "blobdiff_plain";
> +			} else {
> +				$input_params{'action'} ||= "blob_plain";
> +			}

Nice.

I was wondering about 'project/hash_parent..hash' syntax, but then I have
realized that it doesn't change action (as in 'blob_plain' -> 'blobdiff_plain'),
but is always 'shortlog'.

By the way, I wonder if it should be 'blobdiff_plain' or just 'blobdiff'.
the 'blob_plain' was here to use gitweb as a kind of versioned web
server; there is no such equivalent for 'p/hbp..hb:f' syntax. On the
other hand it is consistent behavior, always using *_plain...

>  		}
>  		$input_params{'hash_base'} ||= $refname;
>  		$input_params{'file_name'} ||= $pathname;
> @@ -577,6 +588,27 @@ sub evaluate_path_info {
>  			$input_params{'hash'} ||= $refname;
>  		}
>  	}
> +
> +	# next, handle the 'parent' part, if present
> +	if (defined $parentrefname) {
> +		# a missing pathspec defaults to the 'current' filename, allowing e.g.
> +		# someproject/blobdiff/oldrev..newrev:/filename
> +		if ($parentpathname) {
> +			$parentpathname =~ s,^/+,,;
> +			$parentpathname =~ s,/$,,;

Hmmm... should we strip trailing '/' here?

> +			$input_params{'file_parent'} ||= $parentpathname;
> +		} else {
> +			$input_params{'file_parent'} ||= $input_params{'file_name'};
> +		}
> +		# we assume that hash_parent_base is wanted if a path was specified,
> +		# or if the action wants hash_base instead of hash

Nice comment.

> +		if (defined $input_params{'file_parent'} ||
> +			grep {$input_params{'action'} eq $_} @wants_base) {

My preference of style would be to use here:

+		    grep { $input_params{'action'} eq $_ } @wants_base) {

> +			$input_params{'hash_parent_base'} ||= $parentrefname;
> +		} else {
> +			$input_params{'hash_parent'} ||= $parentrefname;
> +		}
> +	}
>  }
>  evaluate_path_info();
>  
> -- 
> 1.5.6.5
> 
> 

-- 
Jakub Narebski
Poland