patch, 2 partsgitweb: fix #patchNN anchors when path_info is enabled

> Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
> ---
>  gitweb/gitweb.perl |   14 +++++++++++++-
>  1 files changed, 13 insertions(+), 1 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 3b6a90d..d171ad5 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -504,6 +504,12 @@ our %feature = (
>  		'sub' => sub { feature_bool('remote_heads', @_) },
>  		'override' => 0,
>  		'default' => [0]},
> +
> +	# Use localtime rather than GMT for all timestamps.  Disabled
> +	# by default.  Project specific override is not supported.
> +	'localtime' => {
> +		'override' => 0,
> +		'default' => [0]},

>  );
>  
>  sub gitweb_get_feature {
> @@ -2927,6 +2933,12 @@ sub parse_date {
>  	$date{'iso-tz'} = sprintf("%04d-%02d-%02d %02d:%02d:%02d %s",
>  	                          1900+$year, $mon+1, $mday,
>  	                          $hour, $min, $sec, $tz);
> +
> +	if (gitweb_check_feature('localtime')) {
> +		$date{'rfc2822'}   = sprintf "%s, %d %s %4d %02d:%02d:%02d $tz",
> +				     $days[$wday], $mday, $months[$mon],
> +				     1900+$year, $hour ,$min, $sec;
> +	}

>  	return %date;
>  }
>  
> @@ -3989,7 +4001,7 @@ sub git_print_authorship_rows {
>  		      "</td></tr>\n" .
>  		      "<tr>" .
>  		      "<td></td><td> $wd{'rfc2822'}";
> -		print_local_time(%wd);
> +		print_local_time(%wd) if !gitweb_check_feature('localtime');

>  		print "</td>" .
>  		      "</tr>\n";
>  	}
> -- 
> 1.7.4.1
> 

> Kevin Cernekee <cernekee@gmail.com> writes:
>
>> With this feature enabled, all timestamps are shown in the machine's
>> local timezone instead of GMT.
>
> This does not describe why would one want such way of displaying
> timestamps, and which views would be affected.
>
> BTW. should it be timezone of web server (machine where gitweb is
> run), or local time of author / committer / tagger as described in the
> timezone part of git timestamp?

>> +	# Use localtime rather than GMT for all timestamps.  Disabled
>> +	# by default.  Project specific override is not supported.
>> +	'localtime' => {
>> +		'override' => 0,
>> +		'default' => [0]},
>
> Why project specific override is not supported?

> This does not describe why would one want such way of displaying
> timestamps, and which views would be affected.
>
> BTW. should it be timezone of web server (machine where gitweb is
> run), or local time of author / committer / tagger as described in the
> timezone part of git timestamp?

> > This does not describe why would one want such way of displaying
> > timestamps, and which views would be affected.
> >
> > BTW. should it be timezone of web server (machine where gitweb is
> > run), or local time of author / committer / tagger as described in the
> > timezone part of git timestamp?
> 
> The case I am currently trying to improve is the one in which all
> developers are at a single site.
> 
> In the open source world it is common to have developers scattered all
> over the globe, so some of them will inevitably have to perform
> timezone conversions.
> 
> But Git is becoming a popular tool in the private sector and it is
> common to have most/all contributors based in a single office.  In the
> latter case, it is helpful to display the local timezone instead of
> GMT.  This also helps make the data more readable by program managers
> and other non-developers who have an interest in tracking the project.

> > Why project specific override is not supported?  I think it might make
> > sense to enable this feature on project-by-project basis; some
> > projects might be dispersed geographically, some might not.
> 
> Mostly ease of testing.  I did not need it for any of my projects.

> The cases I tested were: 
> 
> default 0
> default 1
> override 1, project unset
> override 1, project 0
> override 1, project 1

> > Is it still an RFC 2822 conformant date?  If it is not, then above
> > change is invalid, and we have to implement this feature in different
> > way.
> 
> I believe it is still valid.
> 
> Original date: Thu, 17 Mar 2011 02:11:05 +0000
> New date: Wed, 16 Mar 2011 19:11:05 -0700
> Sample date from RFC 2822 Appendix A: Fri, 21 Nov 1997 09:55:06 -0600

> The problem is that in this configuration, PATH_INFO is used to set the
> base URL:
> 
> <base href="http://git.example.com/gitweb.cgi">
> 
> This breaks the "patch" anchor links seen on the commitdiff pages,
> because they are computed relative to the base URL:
> 
> http://git.example.com/gitweb.cgi#patch1

> <Location /gitweb>
>         Options ExecCGI
>         SetHandler cgi-script
> </Location>
> 
> GITWEB_{JS,CSS,LOGO,...} all start with gitweb-static/
> 
> gitweb.cgi renamed to /var/www/html/gitweb
> 
> This gives me simple, easy-to-read URLs that look like:
> 
> http://HOST/gitweb/myproject.git/commitdiff/0faa4a6ef921d8a233f30d66f9a3e1b24e8ec906

> 
> Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
> ---
>  gitweb/gitweb.perl |   31 +++++++++++++++++++++++++------
>  1 files changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 1b9369d..3b6a90d 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1199,6 +1199,7 @@ if (defined caller) {
>  # -full => 0|1      - use absolute/full URL ($my_uri/$my_url as base)
>  # -replay => 1      - start from a current view (replay with modifications)
>  # -path_info => 0|1 - don't use/use path_info URL (if possible)
> +# -anchor           - add #ANCHOR to end of URL

>  sub href {
>  	my %params = @_;
>  	# default is to use -absolute url() i.e. $my_uri
> @@ -1314,6 +1315,10 @@ sub href {
>  	# final transformation: trailing spaces must be escaped (URI-encoded)
>  	$href =~ s/(\s+)$/CGI::escape($1)/e;
>  
> +	if (defined($params{'anchor'})) {
> +		$href .= "#".esc_param($params{'anchor'});
> +	}
> +
>  	return $href;
>  }

>  
> @@ -4334,8 +4339,10 @@ sub git_difftree_body {
>  			if ($action eq 'commitdiff') {
>  				# link to patch
>  				$patchno++;
> -				print "<td class=\"link\">" .
> -				      $cgi->a({-href => "#patch$patchno"}, "patch") .
> +				print $cgi->a({-href =>
> +				      href(action=>"commitdiff",
> +				      hash=>$hash, anchor=>"patch$patchno")},
> +				      "patch") .

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

>   @@ -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};

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

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

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