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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 17, 2011, 18:40 UTC
Message-ID: <7v62rh4ml1.fsf@alter.siamese.dyndns.org>
In-Reply-To: <m3hbb258pw.fsf@localhost.localdomain>

Jakub Narebski <jnareb@gmail.com> writes:

Show 7 quoted lines

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

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