Re: [PATCH 1/2] gitweb: fix #patchNN anchors when path_info is enabled

[Jakub Narebski <jnareb@gmail.com>]

On Thu, 17 Mar 2011, Junio C Hamano wrote:
> 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.
 
> >   @@ -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