Re: [PATCHv4] gitweb: parse project/action/hash_base:filename PATH_INFO
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Oct 2, 2008, 08:59 UTC
- Message-ID
- <200810021059.19708.jnareb@gmail.com>
- In-Reply-To
- <1222906234-8182-2-git-send-email-giuseppe.bilotta@gmail.com>
Giuseppe Bilotta wrote:
Show 8 quoted lines
> 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.
Show 16 quoted lines
> 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)
Show 8 quoted lines
> @@ -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;
Show 6 quoted lines
> + 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"
Show 7 quoted lines
> - $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='/').
Show 6 quoted lines
> @@ -544,37 +586,6 @@ evaluate_path_info(); > our $git_dir; > $git_dir = "$projectroot/$project" if $project; > > -# dispatch > -my %actions = (
[...]
Show 5 quoted lines
> -);
> -
> 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.
Show 14 quoted lines
> @@ -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