On Tue, 30 Sep 2008, Giuseppe Bilotta wrote:
> On Tue, Sep 30, 2008 at 2:21 AM, Jakub Narebski <jnareb@xxxxxxxxx> wrote:
[...]
>> Ah. Now I understand.
>>
>> When creating code for href(..., -replay=>1), which by the way I thought
>> would be more useful than actually is, I have forgot that parameters to
>> gitweb could be passed in other way that through CGI parameters
>> (CGI query)[1].
>>
>> Using
>>
>> $params{$name} = [ $cgi->param($symbol) ];
>>
>> is a cute hack, but it doesn't work for arguments passed via path_info
>> (was: project, hash_base and file_name; while now it is project, action,
>> hash_base (in full) and file_name).
[...]
>> The solution I thought about and abandoned in favor of this cute hack
>> was to have additional hash (in addition to %mapping), which would map
>> action names to references to variables holding the value for parameter.
[...]
>> I am talking there about the following solution:
>>
>> my %action_vars = (
>> project => \$project,
>> action => \$action,
>> # ...
>> extra_options => \@extra_options,
>> );
>> # ...
>> while (my ($name, $symbol) = each %mapping) {
>> if (!exists $params{$name}) {
>> $params{$name} = ${$action_vars{$name}};
>> }
>> }
>>
>>
>> This avoids cure hack of (from your code)
>>
>> } else {
>> no strict 'refs';
>> $params{$name} = $$name if $$name;
>> }
>>
>> 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'}?
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. 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'};
if (defined $action) {
if ($action =~ m/[^0-9a-zA-Z\.\-_]/) {
die_error(400, "Invalid action parameter");
}
}
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.
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).
--
Jakub Narebski
Poland
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
