Re: [PATCH 1/2] gitweb: fix #patchNN anchors when path_info is enabled
On Thu, 17 Mar 2011, Junio C Hamano wrote:
Show 12 quoted lines
> Jakub Narebski <jnareb@gmail.com> writes:
>
>> It would be better (less error prone) and easier to use '-replay'
>> option to href(), i.e. write
>>
>>> + print $cgi->a({-href => href(-replay=>1, -anchor=>"patch$patchno")},
>>> + "patch") .
>>
>> or even make it so 'href(-anchor=>"ANCHOR")' implies '-replay => 1'.
>
> I don't see why "or even" is an improvement, given the following
> implementation.Well,
-href => href(-anchor=>"patch$patchno")
is closer in spirit to
-href => "#patch$patchno"
that is currently used, and does not work with path_info.
Show 21 quoted lines
> > @@ -1310,6 +1310,7 @@ sub href {
> >
> > $params{'project'} = $project unless exists $params{'project'};
> >
> > - if ($params{-replay}) {
> > + if ($params{-replay} ||
> > + ($params{-anchor} && keys %params == 1)) {
> > while (my ($name, $symbol) = each %cgi_param_mapping) {
> > if (!exists $params{$name}) {
> > $params{$name} = $input_params{$name};
>
> I don't share your intuition that "anchor" is so special that this part
> will not grow into a long chain of "(I want an implicit replay too) ||".
>
> Implicitly enabling it in certain obvious cases is perfectly fine, but the
> logic to do so should be in a separate place. Wouldn't it better to have
> a separate code that sets 'replay' under this and that condition so that
> other people can later add to it at the very beginning of "sub href"?
>
> Unless we do so, if there are other places that need to change the
> behaviour based on 'replay', they need to duplicate the "implicit" logic.So you would prefer to have something like this:
# implicit -replay
if (keys %params == 1 && $params{-anchor}) {
# href(-anchor=>"ANCHOR") works like "#ANCHOR",
# correctly if base href is set (for path_info URLs)
$params{-replay} = 1;
}set above 'if ($params{-replay}) {'?--
Jakub Narebski
Poland
Jakub Narebski <jnareb@gmail.com>