Re: [PATCH 1/6] gitweb: action in path with use_pathinfo
- From
Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
- Date
- Sep 30, 2008, 08:05 UTC
- Message-ID
- <cb7bb73a0809300105s24706d79hb40e147739ec6f05@mail.gmail.com>
- In-Reply-To
- <200809300221.25094.jnareb@gmail.com>
On Tue, Sep 30, 2008 at 2:21 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> Perhaps not in so large and detailed form... I guess explanation of > using ':/' as separator should be put there as well, if you plan to > squash those patches.
I will add the explanation for the :/ separator in the patch for URL generation (forgot it in v3)
Show 28 quoted lines
>>>> @@ -624,8 +636,13 @@ sub href (%) {
>>>> if ($params{-replay}) {
>>>> while (my ($name, $symbol) = each %mapping) {
>>>> if (!exists $params{$name}) {
>>>> - # to allow for multivalued params we use arrayref form
>>>> - $params{$name} = [ $cgi->param($symbol) ];
>>>> + 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;
>>>> + }
>>>> }
>>>> }
>>>> }
>>>
>>> What this change is about? And why this change is _here_, in this
>>> commit? It is I think unrelated, and wrong change.
>>
>> This is about being able to recycle CGI parameters that came through
>> as part of path_info instead of the CGI parameter list. It might not
>> be the best way to recover it, though. I *did* have a few thoughts
>> about an alternative way that consisted of build a parameter list
>> merging CGI and path-info parameter, but since this approach seemed to
>> work, I went with it.
>
> Fact, I have totally forgot about this.Me too actually (the first version of this patch is almost a year old, and since I forgot to write down why I did a few things ... taught me that I should start putting more comments, though 8-D)
Show 30 quoted lines
>>> href(..., -replay=>1) is all about reusing current URL, perhaps with
>>> a few parameters changed, like for example pagination links differ only
>>> in page number param change. For example if we had only hb= and f=
>>> parameters, -replay=>1 links should use only those, and not add h=
>>> parameter because somewhere we felt that we need $hash to be calculated.
>>
>> Assume for example that you are to an url such as
>>
>> http://git.oblomov.eu/git/tree/refs/remotes/origin/master:gitweb
>>
>> Without this patch, the 'history' link on the second header line links
>> to ARRAY(0xblah)ARRAY(0xblah). With this patch, it shows the proper
>> link. So either replay is being abused somewhere in the link
>> generation code, or this CGI+path_info parameter retrieval is
>> necessary, one way or the other.
>
> 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).Exactly.
Show 38 quoted lines
> 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.
>
> This has the same problem as your proposed solution of putting some
> parameters which didn't come from URL but were filled from other info.
> $hash parameter is most likely to be culprit here.
>
> On the other hand it is more generic and doesn't rely on knowledge that
> there is no multi-valued parameter which can be expressed in path_info
> (currently only 'opt' parameter can be multi-valued, and requires
> arrayref, but also 'opt' parameter is and cannot be put in path_info).
>
> 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)
-- Giuseppe "Oblomov" Bilotta