Re: [PATCHv6 1/5] gitweb: parse project/action/hash_base:filename PATH_INFO
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Oct 18, 2008, 22:41 UTC
- Message-ID
- <200810190041.44711.jnareb@gmail.com>
- In-Reply-To
- <1224188831-17767-2-git-send-email-giuseppe.bilotta@gmail.com>
On Thu, 16 Oct 2008, 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, > $project/$branch[:$filename], except when it was used to access a branch > whose name matches a gitweb action.
...in which case old code would access branch, and new code would treat name as action. This shouldn't matter in practice, unless there are branches named exactly as gitweb actions.
[But I think current commit description is enough. Just in case...]
> > Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
I like it. For what it is worth:
Acked-by: Jakub Narebski <jnareb@gmail.com>
Show 21 quoted lines
> ---
> gitweb/gitweb.perl | 37 +++++++++++++++++++++++++++++++------
> 1 files changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index c5254af..6d0dc26 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -534,23 +534,48 @@ sub evaluate_path_info {
> return if $input_params{'action'};
> $path_info =~ s,^\Q$project\E/*,,;
>
> + # next, check if we have an action
> + my $action = $path_info;
> + $action =~ s,/.*$,,;
> + if (exists $actions{$action}) {
> + $path_info =~ s,^$action/*,,;
> + $input_params{'action'} = $action;
> + }
> +
> + # list of actions that want hash_base instead of hash...and can have no file_name ('f') parameter.> + my @wants_base = ( > + 'tree', > + 'history', > + );
I like this solution. It makes path_info URL unambiguous: it is <project>/log/<hash> (where <hash> is usually refname), and <project>/tree/<hash_base> which is just shortcut for <project>/tree/<hash_base>:/
It means that the @wants_base list contains actions which require / use $hash_base and $file_name, but $file_name might be not set, which denotes top directory (root tree).
Show 6 quoted lines
> +
> my ($refname, $pathname) = split(/:/, $path_info, 2);
> if (defined $pathname) {
> - # we got "project.git/branch:filename" or "project.git/branch:dir/"
> + # we got "branch:filename" or "branch:dir/"
> # we could use git_get_type(branch:pathname), but it needs $git_dirWell, this comment was there... but actually I think we avoid git_get_type("branch:pathname") because currently it is additional fork. And with convention that filename part of path_info ends in '/' for directories (quite sensible I think) it is not needed. Moreso after next patch in series, when we state action explicitly, and the only difference _might_ be for 'history' view (which is for directories and for files both).
Show 16 quoted lines
> $pathname =~ s,^/+,,;
> if (!$pathname || substr($pathname, -1) eq "/") {
> - $input_params{'action'} = "tree";
> + $input_params{'action'} ||= "tree";
> $pathname =~ s,/$,,;
> } else {
> - $input_params{'action'} = "blob_plain";
> + $input_params{'action'} ||= "blob_plain";
> }
> $input_params{'hash_base'} ||= $refname;
> $input_params{'file_name'} ||= $pathname;
> } elsif (defined $refname) {
> - # we got "project.git/branch"
> - $input_params{'action'} = "shortlog";
> - $input_params{'hash'} ||= $refname;
> + # we got "branch". in this case we have to choose if we have to ^ ^
|-|-- ??? (typo?)Show 7 quoted lines
> + # set hash or hash_base. > + # > + # Most of the actions without a pathname only want hash to be > + # set, except for the ones specified in @wants_base that want > + # hash_base instead. It should also be noted that hand-crafted > + # links having 'history' as an action and no pathname or hash > + # set will fail, but that happens regardless of PATH_INFO.
Ah, I see that the comment about what makes action to be put into @wants_base is here.
> + $input_params{'action'} ||= "shortlog";
> + if (grep {$input_params{'action'} eq $_} @wants_base) {Style: I would use here (but perhaps it is just me) for better readibility
+ if (grep { $input_params{'action'} eq $_ } @wants_base) {Show 11 quoted lines
> + $input_params{'hash_base'} ||= $refname;
> + } else {
> + $input_params{'hash'} ||= $refname;
> + }
> }
> }
> evaluate_path_info();
> --
> 1.5.6.5
>
> -- Jakub Narebski Poland