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

My configuration is as follows:

$feature{'pathinfo'}{'default'} = [1];

<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

The problem is that in this configuration, PATH_INFO is used to set the
base URL:

<base href="http://HOST/gitweb">

This breaks the "patch" anchor links seen on the commitdiff pages,
because they are computed relative to the base URL:

http://HOST/gitweb#patch1

My solution is to add an "anchor" parameter to href(), so that the full
path is included in the patchNN links.

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") .
 				      " | " .
 				      "</td>\n";
 			}

@@ -4432,7 +4439,10 @@ sub git_difftree_body {
 			if ($action eq 'commitdiff') {
 				# link to patch
 				$patchno++;
-				print $cgi->a({-href => "#patch$patchno"}, "patch");
+				print $cgi->a({-href =>
+				      href(action=>"commitdiff",
+				      hash=>$hash, anchor=>"patch$patchno")},
+				      "patch");
 				print " | ";
 			}
 			print $cgi->a({-href => href(action=>"blob", hash=>$diff->{'to_id'},

@@ -4452,7 +4462,10 @@ sub git_difftree_body {
 			if ($action eq 'commitdiff') {
 				# link to patch
 				$patchno++;
-				print $cgi->a({-href => "#patch$patchno"}, "patch");
+				print $cgi->a({-href =>
+				      href(action=>"commitdiff",
+				      hash=>$hash, anchor=>"patch$patchno")},
+				      "patch");
 				print " | ";
 			}
 			print $cgi->a({-href => href(action=>"blob", hash=>$diff->{'from_id'},

@@ -4494,7 +4507,10 @@ sub git_difftree_body {
 			if ($action eq 'commitdiff') {
 				# link to patch
 				$patchno++;
-				print $cgi->a({-href => "#patch$patchno"}, "patch") .
+				print $cgi->a({-href =>
+				      href(action=>"commitdiff",
+				      hash=>$hash, anchor=>"patch$patchno")},
+				      "patch") .
 				      " | ";
 			} elsif ($diff->{'to_id'} ne $diff->{'from_id'}) {
 				# "commit" view and modified file (not onlu mode changed)

@@ -4539,7 +4555,10 @@ sub git_difftree_body {
 			if ($action eq 'commitdiff') {
 				# link to patch
 				$patchno++;
-				print $cgi->a({-href => "#patch$patchno"}, "patch") .
+				print $cgi->a({-href =>
+				      href(action=>"commitdiff",
+				      hash=>$hash, anchor=>"patch$patchno")},
+				      "patch") .
 				      " | ";
 			} elsif ($diff->{'to_id'} ne $diff->{'from_id'}) {
 				# "commit" view and modified file (not only pure rename or copy)

-- 
1.7.4.1

With this feature enabled, all timestamps are shown in the machine's
local timezone instead of GMT.

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 [off-list ref] writes:

My configuration is as follows:

Very minor issue: Documentation/SubmittingPatches states the
following:

    - describe changes in imperative mood, e.g. "make xyzzy do frotz"
      instead of "[This patch] makes xyzzy do frotz" or "[I] changed
      xyzzy to do frotz", as if you are giving orders to the codebase
      to change its behaviour.

I think this also means trying to avoid "My configuration..." and "My
solution..." etc. in commit message.  But this is just a side issue,
not worth worrying over in my opinion; perhaps something to think
about in the future.

$feature{'pathinfo'}{'default'} = [1];

[...]

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

I think that the above configuration is enough to trigger bug /
errorneous behavior that you describe, isn't it?  It is better to try
to find minimal way to reproduce a bug when describing it.

I guess that
 

<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

 
is not strictly necessary to trigger a bug.

My solution is to add an "anchor" parameter to href(), so that the full
path is included in the patchNN links.

This is a very good idea.  Thank you very much for sending this patch,
and contributing to gitweb.

Its implemetation could be though improved a bit; see below.

Shouldn't it be:

  +# -anchor => ANCHOR - add #ANCHOR to end of URL

Here you have slight mismatch between description, which uses
'-anchor', and code, which uses 'anchor'.

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'.
The href() part of patch would then look something like this:

  @@ -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 => ANCHOR - add #ANCHOR to end of URL, implies -replay if used alone
   sub href {
   	my %params = @_;
   	# default is to use -absolute url() i.e. $my_uri
  @@ -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};
  @@ -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;
   }

Do you want to resend patch with those corrections yourself, or should
I do this?

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Kevin Cernekee [off-list ref] 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?
 

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.

It is not as if this feature affect only non-project views, or doesn't
make sense on less that site-wide basis, like other nonoverridable
features.

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.

Hmmm... I wonder if it wouldn't be better to print both times (perhaps
reversed) in this case...

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

-- 
Jakub Narebski
Poland
ShadeHawk on #git

On Thu, Mar 17, 2011 at 4:01 AM, Jakub Narebski [off-list ref] wrote:

Jakub,

Thanks for all of your constructive feedback.  I have taken your
suggestions into account and posted an updated series of patches.

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.

It turned out to be a simple change, and it is in v2.  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

Hmmm... I wonder if it wouldn't be better to print both times (perhaps
reversed) in this case...

I have submitted a third patch which does this.

On Thu, Mar 17, 2011 at 21:12, Kevin Cernekee wrote:

On Thu, Mar 17, 2011 at 4:01 AM, Jakub Narebski [off-list ref] wrote:

quoted

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.

Such explanation should in my opinion be present in the proposed commit
message.  Good commit message should explain not only what the change is
intent to do (to catch when implementation and intent differs), but also
whys behind the change (to find whether commit is worth having).

The above nicely explains why and when such feature would be useful,
 

quoted

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.

You mean that for your instance of gitweb all projects were single
office (not dispersed geographically), so for you enabling it site-wide
was enough, isn't it?
 

It turned out to be a simple change, and it is in v2.

Thanks.

The cases I tested were: 

default 0
default 1
override 1, project unset
override 1, project 0
override 1, project 1

Well, I think the non-overridden / overridden feature we have tested quite
well.  BTW did you add test for 'localtime' feature to t9500 test?  Perhaps
it is not strictly necessary, though...

quoted

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

Thanks.

quoted

Hmmm... I wonder if it wouldn't be better to print both times (perhaps
reversed) in this case...

I have submitted a third patch which does this.

Note that we print localtime (time in author / committer / tagger timezone)
to be able to mark given time as "atnight", so one can easily see commits
and tags which needs more careful review because they were made 4 AM or
something.

If you reverse the direction you still should make sure that "atnight"
styling applies to localtime.

-- 
Jakub Narebski
Poland