Re: [PATCHv4] gitweb: refactor input parameters parse/validation
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Oct 3, 2008, 11:20 UTC
- Message-ID
- <200810031320.28843.jnareb@gmail.com>
- In-Reply-To
- <cb7bb73a0810030024t663940dbqa23f2e1bb75e23bc@mail.gmail.com>
On Fri, 3 Oct 2008, Giuseppe Bilotta wrote:
> On Fri, Oct 3, 2008 at 3:36 AM, Jakub Narebski <jnareb@gmail.com> wrote: >> On Thu, 2 Oct 2008, Giuseppe Bilotta wrote:
Show 23 quoted lines
>>> We thus collect all the parameters into the new %input_params hash,
>>> removing the need for ad-hoc kludgy code in href().
>>
>> Alternate solution would be push data from PATH_INFO in query params
>> data using (for example)
>>
>> $cgi->param('a', $action);
>>
>> or, naming parameters explicitely
>>
>> $cgi->param(-name=>'a', -value=>$action);
>>
>> This avoids need for additional variable, reuses current code, and
>> nicely sidesteps issue whether to use long names for keys ('action',
>> 'file_name') or short ones from CGI query ('a', 'f').
>>
>>
>> It would probably has to be at least partially written to check which
>> of those two solutions (%input_params or $cgi->param('a', $a)))
>> is better.
>
> If we really don't want to care where the parameters came from *at
> all*, writing on $cgi->param is likely to be the cleanest solution.I now think that %input_params is a better solution, even if we won't be implementing -true_replay / -faithful_replay option, where options gotten in path_info would be passed in path_info, and options passed in query string would be passed as CGI params (even if those params could be encoded in path_info). One of the advantages is that having @cgi_param_mapping near the top provides opportunity to describe short CGI query options.
Another advantage is that with %input_params it is easy to find if parameter is multivalued, or is meant to be multivalued: just check ref($input_params{long_name}); well, perhaps not _that_ easy, but...
Show 11 quoted lines
>>> As much of the >>> parameters validation code is now shared between CGI and PATH_INFO, >>> although this requires PATH_INFO parsing to be interleaved into the main >>> code instead of being factored out into its own routine. >> >> I'm not sure if it is worth it then to unify parameters validation, >> with such drawback. > > Yeah, I don't like it very much either. But it does spare a little on > the validation. OTOH, the amount we spare is not extraordinary, so > it's probably not worth the spaghettization of the code ...
Avoiding repetition can be done in different ways, for example we can have gitweb assume that if value is already in params variable ($project, $action, $hash,...) it is already validated, and if it is only in %input_params it is not. Add to that for example putting common part of project path checks into validate_project and you have all the advantages (no code repetition, spared validation) without drawback of harder to maintain spaghetti-like code.
Show 33 quoted lines
>>> +# fill %input_params with the CGI parameters. All values except for 'opt'
>>> +# should be single values, but opt can be an array. We should probably
>>> +# build an array of parameters that can be multi-valued, but since for the time
>>> +# being it's only this one, we just single it out
>>> +while (my ($name, $symbol) = each %cgi_param_mapping) {
>>> + if ($symbol eq 'opt') {
>>> + $input_params{$name} = [$cgi->param($symbol)];
>>> + } else {
>>> + $input_params{$name} = $cgi->param($symbol);
>>> }
>>> }
>>
>> If it was chosen to use short (CGI query) parameter names, the above
>> loop could be replaced by simple
>>
>> %input_params = $cgi->Vars;
>>
>> or to be more exact, if we want to have multi-valued parameters stored
>> as array ref
>>
>> %input_params = map { [ split /\0/ ] if /\0/; } $cgi->Vars;
>>
>>
>> See CGI(3pm):
>>
>> When using this [Vars], the thing you must watch out for are multivalued CGI
>> parameters. Because a hash cannot distinguish between scalar and list con-
>> text, multivalued parameters will be returned as a packed string, separated
>> by the "\0" (null) character. You must split this packed string in order
>> to get at the individual values.
>
> Ah, an interesting alternative. This would make parameter copying a
> one-liner, almost as good as just using $cgi->param for everything :)Hmmmm...
[cut]
Show 8 quoted lines
>> Note that this code does less checking if $project is in path_info than >> for the case where it is set by CGI query. Perhaps there should be base >> fast check in a loop, and more extensive test later. > > Uh ... isn't this exactly what's happening? In the loop we just gobble > until we find a git dir. Validation is then done, and it's the _same_ > validation for both cases. Why do you say that path_info $project is > less checked?
For project from query string we now have:
!validate_pathname($project) ||
!(-d "$projectroot/$project") ||
!check_head_link("$projectroot/$project") ||
($export_ok && !(-e "$projectroot/$project/$export_ok")) ||
($strict_export && !project_in_list($project))For project from path_info we now have:
while ($project && !check_head_link("$projectroot/$project")) {
$project =~ s,/*[^/]*$,,;
}
...
!validate_pathname($project) ||
($export_ok && !-e "$projectroot/$project/$export_ok") ||
($strict_export && !project_in_list($project))
It is almost the same; I have thought that they differ more. Perhaps
some of above code could be refactored into validate_project(), or
project_ok() subroutine.Show 24 quoted lines
>>> $params{'project'} = $project unless exists $params{'project'};
>>>
>>> if ($params{-replay}) {
>>> - while (my ($name, $symbol) = each %mapping) {
>>> - if (!exists $params{$name}) {
>>> - # 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;
>>> - }
>>> - }
>>> + while (my ($name, $val) = each %input_params) {
>>> + $params{$name} = $input_params{$name}
>>> + unless (exists $params{$name});
>>
>> Very nice, short code. Should be something like that from
>> the very beginning.
>
> Ok, I'll try working up a patch for params merging before any
> path_info extensions.Perhaps also put query string handling into evaluate_CGI_query(), or evaluate_query_string(), similar to evaluate_path_info()?
-- Jakub Narebski Poland