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

From: Jakub Narebski <jnareb@gmail.com>
Date: Sep 30, 2008, 08:48 UTC
Message-ID: <200809301048.40046.jnareb@gmail.com>
In-Reply-To: <cb7bb73a0809300105s24706d79hb40e147739ec6f05@mail.gmail.com>

On Tue, 30 Sep 2008, Giuseppe Bilotta wrote:

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

[...]

Show 14 quoted lines

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

[...]

Show 50 quoted lines

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