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

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

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

>>> I think that gitweb should use single source, not CGI query parameters
>>> or variable saving [sanitized] value.
>>
>> The alternative I've been thinking about would be to have an
>> %input_parameters hash that holds all input parameters regardless of
>> hash; thus CGI query parameters and data extracted from PATH_INFO,
>> presently, but also command line options in the future, or whatever
>> else.
>>
>> This is somewhat different from your %action_vars alternative, in the
>> sense that it isolates _input_ data, whereas if I understand correctly
>> the approach you suggest would isolate _output_ data (in the sense of
>> data to be used during link creation and whatnot).
>>
>> Presently, the gitweb code defines some $variables from the input
>> parameters, and then overwrites them for output. Keeping the input
>> stuff clearly separate from the output stuff would mean that any
>> routine can retrieve the input data regardless of the subsequent
>> mangling and without any need to make ad-hoc backups or other tricks.
>>
>> So my proposal is that I implement this %input_params stuff as the
>> first patch for the pathinfo series, and use %input_params all around
>> where cgi parameters are used currently (of course, %input_params is
>> initialized with the CGI parameters at first). The next patch would be
>> the extraction of parameters from PATH_INFO. And thirdly the PATH_INFO
>> URL generation (with or without the /-before-filename thing, at your
>> preference)
>
> 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.

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

> The input validation
> and dispatch part would be modified to use %params (taking care of
> multivalued parameters as described in CGI(3pm)), like below:
>
>  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.

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

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

-- 
Giuseppe "Oblomov" Bilotta