On Tue, Sep 30, 2008 at 2:21 AM, Jakub Narebski <jnareb@xxxxxxxxx> 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)
>>>> @@ -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)
>>> 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.
> 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
--
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
