Re: [PATCH] gitweb: refactor input parameters parse/validation

[Jakub Narebski <jnareb@gmail.com>]

Giuseppe Bilotta wrote:
> On Tue, Oct 7, 2008 at 12:57 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>> On Fri, 3 Oct 2008, Giuseppe Bilotta wrote:

>>> +     # find which part of PATH_INFO is project
>>> +     my $project = $path_info;
>>
>> Hmmm... now $project is local (lexically) here.
> 
> Yes, itt's only used temporarily here, to see if  a proper $project
> can be defined. It gets redefined outside. It just made sense to name
> it like this 8-)

Well, if $project is local in evaluate_path_info(), so could be
$path_info...
 
>>> +     $project =~ s,/+$,,;
>>> +     while ($project && !check_head_link("$projectroot/$project")) {
>>> +             $project =~ s,/*[^/]*$,,;
>>> +     }
>>> +     # validate project
>>> +     $project = validate_project($project);
>>
>> I'm not sure if it is worth worrying over, but I think you repeat
>> check_head_link() check here.
>>
>> [After examining code further].  But I think you do double validation;
>> once you do it here, and once you do it copying to global variables
>> such as $action or $project, and double checking check_head_link()
>> won't be easy to avoid; fortunately it is cheap filesystem-level check
>> (might be slow only when stat is extremely slow, and is not cached).
> 
> I know. This is actually the reason why I had interleaved path_info
> definition and global validation in my previous version of the patch.
> The big issue here is that path_info evaluation _needs_ (partial)
> validation.
> 
> A possible alternative could be to only put validated parameters into
> %input_params. This would completely separate the validation for cgi
> and path_info (modulo shared subs).
> 
> Of course, the check_head_link would still be repeated inside
> evaluate_path_info, but the other params could skip a double
> validation.

Wouldn't it be simpler and as good solution to just leave validation
off evaluate_path_info() (well, of course except check_head_link() test),
and allow it to be validated when assigning global 'params' variables?
check_head_link() would be repeated for path_info links, but that
should not affect performance much.

-- 
Jakub Narebski
Poland