Re: [PATCH 1/6] gitweb: action in path with use_pathinfo

From: Jakub Narebski <jnareb@gmail.com>
Date: Sep 29, 2008, 01:03 UTC
Message-ID: <200809290303.21290.jnareb@gmail.com>
In-Reply-To: <1222030663-22540-2-git-send-email-giuseppe.bilotta@gmail.com>

I'm sorry for the delay reviewing those patches.

On Sun, 21 Sep 2008, Giuseppe Bilotta wrote:

> When using path info, reduce the number of CGI parameters by embedding
> the action in the path. The typicial cgiweb path is thus
> $project/$action/$hash_base:$file_name or $project/$action/$hash

cgiweb?

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

I would also state that gitweb _generates_ such pathinfo links if configured (or if coming from pahinfo URL), and that this change allow to represent wider number of gitweb links (gitweb URLs) in pathinfo form.

Also, from what I understand, generated pathinfo links now always use action, so they are a tiny little bit longer.

Show 44 quoted lines

> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl |  109 ++++++++++++++++++++++++++++++++++------------------
>  1 files changed, 72 insertions(+), 37 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index da474d0..e783d12 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -488,6 +488,37 @@ if (defined $searchtext) {
>  	$search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;
>  }
>  
> +# dispatch
> +my %actions = (
> +	"blame" => \&git_blame,
> +	"blobdiff" => \&git_blobdiff,
> +	"blobdiff_plain" => \&git_blobdiff_plain,
> +	"blob" => \&git_blob,
> +	"blob_plain" => \&git_blob_plain,
> +	"commitdiff" => \&git_commitdiff,
> +	"commitdiff_plain" => \&git_commitdiff_plain,
> +	"commit" => \&git_commit,
> +	"forks" => \&git_forks,
> +	"heads" => \&git_heads,
> +	"history" => \&git_history,
> +	"log" => \&git_log,
> +	"rss" => \&git_rss,
> +	"atom" => \&git_atom,
> +	"search" => \&git_search,
> +	"search_help" => \&git_search_help,
> +	"shortlog" => \&git_shortlog,
> +	"summary" => \&git_summary,
> +	"tag" => \&git_tag,
> +	"tags" => \&git_tags,
> +	"tree" => \&git_tree,
> +	"snapshot" => \&git_snapshot,
> +	"object" => \&git_object,
> +	# those below don't need $project
> +	"opml" => \&git_opml,
> +	"project_list" => \&git_project_list,
> +	"project_index" => \&git_project_index,
> +);
> +

This is as I understand simply moving %actions hash around? Well, you could instead split hash declaration from defining it, in the form of

   my %actions = ();
   ...
   %actions = (
        ...
   );

but I guess moving declaration earlier is good solution.

Show 11 quoted lines

>  # now read PATH_INFO and use it as alternative to parameters
>  sub evaluate_path_info {
>  	return if defined $project;
> @@ -512,6 +543,16 @@ 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 perhaps "$action = ($path_info =~ m!^([^/]+)/!;" But that is Perl, so TIMTOWDI.

Show 6 quoted lines

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

I don't think you need quoting pattern metacharacters; valid actions should not contain regexp metacharacters. Defensive programming?

Show 8 quoted lines

>  	my ($refname, $pathname) = split(/:/, $path_info, 2);
>  	if (defined $pathname) {
>  		# we got "project.git/branch:filename" or "project.git/branch:dir/"
> @@ -525,10 +566,12 @@ sub evaluate_path_info {
>  		}
>  		$hash_base ||= validate_refname($refname);
>  		$file_name ||= validate_pathname($pathname);
> +		$hash      ||= git_get_hash_by_path($hash_base, $file_name);

I don't understand why you feel that you need to do this (this is additional git command fork, as git_get_hash_by_path calls Git, to be more exact it calls git-ls-tree (it could call git-rev-parse instead). Moreover, I don't understand why you need to do this _here_, instead of just before where you would have to have $hash variable set.

Show 26 quoted lines

>  	} elsif (defined $refname) {
>  		# we got "project.git/branch"
> -		$action ||= "shortlog";
> -		$hash   ||= validate_refname($refname);
> +		$action    ||= "shortlog";
> +		$hash      ||= validate_refname($refname);
> +		$hash_base ||= validate_refname($refname);
>  	}
>  }
>  evaluate_path_info();
> @@ -624,8 +636,13 @@ 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) ];
> +				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;
> +				}
>  			}
>  		}
>  	}

What this change is about? And why this change is _here_, in this commit? It is I think unrelated, and wrong change.

href(..., -replay=>1) is all about reusing current URL, perhaps with a few parameters changed, like for example pagination links differ only in page number param change. For example if we had only hb= and f= parameters, -replay=>1 links should use only those, and not add h= parameter because somewhere we felt that we need $hash to be calculated.

> @@ -636,10 +653,28 @@ sub href (%) {

This hunk is about generating pathinfo URL, isn't it?

You probably would want to change comment at the top of this part of href() subroutine, namely

  	if ($use_pathinfo) {
		# use PATH_INFO for project name

as you now try to use PATH_INFO for more than project name. Please do not leave comments to get out of sync with the code.

Show 18 quoted lines

>  		$href .= "/".esc_url($params{'project'}) if defined $params{'project'};
>  		delete $params{'project'};
>  
> -		# Summary just uses the project path URL
> -		if (defined $params{'action'} && $params{'action'} eq 'summary') {
> +		# Summary just uses the project path URL, any other action come next
> +		# in the URL
> +		if (defined $params{'action'}) {
> +			$href .= "/".esc_url($params{'action'}) unless $params{'action'} eq 'summary';
>  			delete $params{'action'};
>  		}
> +
> +		# next, we put either hash_base:file_name or hash
> +		if (defined $params{'hash_base'}) {
> +			$href .= "/".esc_url($params{'hash_base'});
> +			if (defined $params{'file_name'}) {
> +				$href .= ":".esc_url($params{'file_name'});
> +				delete $params{'hash'} if $params{'hash'} eq git_get_hash_by_path($params{'hash_base'},$params{'file_name'});

First, this page has around 140 characters. That is too long, much too long. Please try to wrap code around 80-characters.

Second, following Petr 'Pasky' Baudis suggestion of reducing complexity and shortening gitweb URLs, we could unconditionally remove redundant 'hash' parameter if we have both 'hash_base' and 'file_name' parameters. This would also simplify and speed up (lack of extra fork) gitweb code.

Show 9 quoted lines

> +				delete $params{'file_name'};
> +			} else {
> +				delete $params{'hash'} if $params{'hash'} eq $params{'hash_base'};
> +			}
> +			delete $params{'hash_base'};
> +		} elsif (defined $params{'hash'}) {
> +			$href .= "/".esc_url($params{'hash'});
> +			delete $params{'hash'};
> +		}

O.K.... I think.

Did you test this, preferably also using Mechanize test, with gitweb configuration turning path_info on by default.?

>  	}
>  
>  	# now encode the parameters explicitly

Hmmm... now I am not so sure if it wouldn't be better to split this patch in pathinfo parsing and pathinfo generation. The first part would be obvious, the second part would be smaller and easier to review.

-- 
Jakub Narebski
Poland