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

> On Thu, 2 Oct 2008, Giuseppe Bilotta wrote:
>> 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?
>
> Well, it is either checking $action and setting either $hash or
> $hash_base, or setting both, with some comments on why and when it is
> needed (as discussed on #git). IIUC $hash_base is needed only for
> filename-taking tree actions which acts on top-tree, and therefore
> don't need $file_name, like 'project/tree/branch' or related
> 'project/history/branch' (the latter is practically almost equivalent
> to 'project/shortlog/branch' or 'project/branch').
>
> I'm not sure if it wouldn't be better to call validate_refname($refname)
> once, either as:
>
>  $hash_base ||= $hash ||= validate_refname($refname);
>
> but that might be incorrect in the obscure case of setting $hash via 'h'
> CGI query parameter, and letting gitweb to set-up $hash_base via
> path_info, so perhaps ($refname is local to evaluate_path_info, IIRC)
>
>  $refname = validate_refname($refname);
>  $hash      ||= $refname;
>  $hash_base ||= $refname;

>>>> @@ -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.
>
> Why not cleanup first?

> When implementing href(..., -replay=>1) I have forgot that some of
> gitweb parameters are implicitly passed ($project, because it is needed
> in most gitweb links), and some can be passed via path_info ($hash
> and/or $hash_base, $file_name). Your code adds $action to the mix, but
> it doesn't change the fact that 1.) even before your code -replay case
> was incorrect for some path_info links (handcrafted, as gitweb generates
> only $project via path_info); 2.) code you have added is a bit ugly.
>
> Besides using variables change a little meaning of -replay, namely
> in your code gitweb always sets action, even for non-path_name links
> when we started from "default action" (i.e. without action set) links.
> I guess this is mainly theoretical issue, as I don't think that default
> views use many -replay links.