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

["Giuseppe Bilotta" <giuseppe.bilotta@gmail.com>]

On Thu, Oct 2, 2008 at 10:59 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> Giuseppe Bilotta wrote:
>>
>> +# 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)

Bingo.

>> @@ -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;


Yeah, I just followed the existing code style.


>> @@ -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"

Yes I do.

>> -             $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='/').

OTOH, while setting both $hash and $hash_base has worked fine for me
so far (because the right one is automatically used and apparently
setting the other doesn't hurt), choosing which one to set is a much
hairier case. Do you have suggestions for a better way to always make
it work?

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

I agree.

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

But implementing the path_info parsing first makes the input param
refactoring SO much nicer that I would rather put a comment here
saying "this code sucks: we should rather collect all input
parameters" and then clean it up on the subsequent patch.


-- 
Giuseppe "Oblomov" Bilotta