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

[Jakub Narebski <jnareb@gmail.com>]

On Tue, 30 Sep 2008, Giuseppe Bilotta wrote:
> On Tue, Sep 30, 2008 at 10:48 AM, Jakub Narebski <jnareb@gmail.com> wrote:

>> I presume that you would want to replace for example $hash_base
>> everywhere by %input_params{'hash_base'}?
> 
> No. %input_params{'hash_base'} would only be the _input_ hash base.
> $hash_base would be kept if it's supposed to indicate the value of
> hash base that is being manipulated.

Ah, sorry, I misunderstood. Then your idea is the same as one of mine
(except perhaps some details).
 
>> I can think of yet another solution, namely to abstract getting
>> parameters from CGI query string, from path_info, and possibly in the
>> future also from command line options, and use this mechanism in
>> the getting parameters and validation part.
>>
>> The %params hash would be filled from CGI parameters by using simply
>> "%params = $cgi->Vars;", then added to in evaluate_path_info instead
>> of directly modifying global parameters variables.
> 
> So far I agree.

Using "%input_params = %cgi->Vars;" has consequence of using short
parameter names for keys (and also a bit strange syntax for multivalue
options, see CGI(3pm)).

>> The input validation
>> and dispatch part would be modified to use %params (taking care of
>> multivalued parameters as described in CGI(3pm)), like below:

This has the additional advantage of doing gitweb parameter validation
_once_, and not like it is now done for example first in the "input
validation and dispatch" section, and then in evaluate_path_info()
subroutine.

On the other hand $project is checked _already_ in evaluate_path_info(),
because it has to to find where project name ends, so this part would
get duplicated, unless something smart is done.

>>
>>  our $action = $params{'a'} || $params{'action'};
>
> Not too sure about that. The path_info (or whatever)-derived params
> should be converted to use the same name as the CGI params. Or
> conversely, CGI params should be mapped to the corresponding
> full-form.

After thinking about it a little, I agree with above paragraph.

>> That is just for consideration: each approach has its advantages and
>> disadvantages.  Your proposal, as I understand it, is similar to the
>> way described in "Storing options in a hash" subsection of
>> Getopt::Long(3pm) manpage.
> 
> I'll read that, although it probably is.

Perhaps gitweb should have implement something like GetOptions?

>> Or we could just scrap and revert adding href(..., -replay=>1).
>> There is much trouble with getting it right and performing well,
>> and it is less useful than I thought (at least now).
> 
> Dunno, the idea in itself is not bad. We just have to get it right ;)
> 
> In a way, I actually think that -replay=>1 should be the default, I
> suspect it makes sense in most cases.

PITA but useful. Hmmm....

-- 
Jakub Narebski
Poland