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

Giuseppe Bilotta wrote:

On Tue, Oct 7, 2008 at 12:57 PM, Jakub Narebski [off-list ref] wrote:

quoted

On Fri, 3 Oct 2008, Giuseppe Bilotta wrote:

quoted

quoted

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

quoted

quoted

+     $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