Re: [PATCHv4] gitweb: parse project/action/hash_base:filename PATH_INFO

[Jakub Narebski <jnareb@gmail.com>]

Giuseppe Bilotta wrote:

> This patch enables gitweb to parse URLs with more information embedded
> in PATH_INFO, reducing the need for CGI parameters. The typical gitweb
> path is now $project/$action/$hash_base:$file_name or
> $project/$action/$hash
> 
> This is mostly backwards compatible with the old-style gitweb paths,
> except when $project/$branch was used to access a branch whose name
> matches a gitweb action.

Nice summary.

> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl |   90 +++++++++++++++++++++++++++++++---------------------
>  1 files changed, 54 insertions(+), 36 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index e7e4d6b..f088681 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -495,6 +495,37 @@ if (defined $searchtext) {
>  	$search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;
>  }
>  
> +# dispatch
> +my %actions = (
> +	"blame" => \&git_blame,
[...]
> +);

I'm not sure if the '# dispatch' comment is correct here now that
%actions hash is moved away from actual dispatch (selecting action
to run)

> @@ -519,9 +550,19 @@ sub evaluate_path_info {
>  	# do not change any parameters if an action is given using the query string
>  	return if $action;
>  	$path_info =~ s,^\Q$project\E/*,,;
> +
> +	# next comes the action
> +	$action = $path_info;
> +	$action =~ s,/.*$,,;

I would use here pattern matching, but your code is also good and
doesn't need changing; just for completeness below there is alternate
solution:

+	$path_info =~ m,^(.*?)/,;
+	$action = $1;

> +	if (exists $actions{$action}) {
> +		$path_info =~ s,^$action/*,,;
> +	} else {
> +		$action  = undef;
> +	}
> +

[...]
> @@ -534,8 +575,9 @@ sub evaluate_path_info {
>  		$file_name ||= validate_pathname($pathname);
>  	} elsif (defined $refname) {
>  		# we got "project.git/branch"

You meant here

  		# we got "project.git/branch" or "project.git/action/branch"

> -		$action ||= "shortlog";
> -		$hash   ||= validate_refname($refname);
> +		$action    ||= "shortlog";
> +		$hash      ||= validate_refname($refname);
> +		$hash_base ||= validate_refname($refname);
>  	}
>  }

This hunk is IMHO incorrect.  First, $refname is _either_ $hash, or
$hash_base; it cannot be both.  Second, in most cases (like the case
of 'shortlog' action, either explicit or implicit) it is simply $hash;
I think it can be $hash_base when $file_name is not set only in
singular exception case of 'tree' view for the top tree (lack of
filename is not an error, but is equivalent to $file_name='/').

> @@ -544,37 +586,6 @@ evaluate_path_info();
>  our $git_dir;
>  $git_dir = "$projectroot/$project" if $project;
>  
> -# dispatch
> -my %actions = (
[...]
> -);
> -
>  if (!defined $action) {
>  	if (defined $hash) {
>  		$action = git_get_type($hash);

I _think_ the '# dispatch' comment should be left here, and not moved
with the %actions hash.

> @@ -631,8 +642,15 @@ sub href (%) {
>  	if ($params{-replay}) {
>  		while (my ($name, $symbol) = each %mapping) {
>  			if (!exists $params{$name}) {
> -				# to allow for multivalued params we use arrayref form
> -				$params{$name} = [ $cgi->param($symbol) ];
> +				# the parameter we want to recycle may be either part of the
> +				# list of CGI parameter, or recovered from PATH_INFO
> +				if ($cgi->param($symbol)) {
> +					# to allow for multivalued params we use arrayref form
> +					$params{$name} = [ $cgi->param($symbol) ];
> +				} else {
> +					no strict 'refs';
> +					$params{$name} = $$name if $$name;

I would _perhaps_ add here comment that multivalued parameters can come
only from CGI query string, so there is no need for something like:

+					$params{$name} = (ref($$name) ? @$name : $$name) if $$name;

> +				}
>  			}
>  		}
>  	}

This fragment is a bit of ugly code, hopefully corrected in later patch.
I think it would be better to have 'refactor parsing/validation of input
parameters' to be very fist patch in series; I am not sure but I suspect
that is a kind of bugfix for current "$project/$hash" ('shortlog' view)
and "$project/$hash_base:$file_name" ('blob_plain' and 'tree' view)
path_info.

P.S. It is a bit of pity that Mechanize test from Lea Wiemann caching
gitweb code is not in the 'master' or at least 'pu'.  Using big, single,
monolithic patch instead of patch series of small, easy reviewable
commits strikes again... ;-(

-- 
Jakub Narebski
Poland