gitweb: fix #patchNN anchors when path_info is enabled

When $feature{'pathinfo'} is used, gitweb sets the base URL to something like:

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

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

http://HOST/gitweb.cgi#patch1

Instead, they should look like:

http://HOST/gitweb.cgi/myproject.git/commitdiff/35a9811ef9d68eae9afd76bede121da4f89b448c#patch1

Add an "-anchor" parameter to href(), so that the full path is included in the patch link.

---
 gitweb/gitweb.perl |   25 +++++++++++++++++++------
 1 files changed, 19 insertions(+), 6 deletions(-)

index 0779f12..57a3caf 100755
@@ -1199,11 +1199,13 @@ 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
 	my $href = $params{-full} ? $my_url : $my_uri;
 
+	$params{-replay} = 1 if ($params{-anchor} && keys %params == 1);
 	$params{'project'} = $project unless exists $params{'project'};
 
 	if ($params{-replay}) {

@@ -1314,6 +1316,10 @@ sub href {
 	# final transformation: trailing spaces must be escaped (URI-encoded)
 	$href =~ s/(\s+)$/CGI::escape($1)/e;
 
+	if ($params{-anchor}) {
+		$href .= "#".esc_param($params{-anchor});
+	}
+
 	return $href;
 }
 

@@ -4334,8 +4340,9 @@ 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(-anchor=>"patch$patchno")},
+				      "patch") .
 				      " | " .
 				      "</td>\n";
 			}

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

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

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

@@ -4539,7 +4550,9 @@ sub git_difftree_body {
 			if ($action eq 'commitdiff') {
 				# link to patch
 				$patchno++;
-				print $cgi->a({-href => "#patch$patchno"}, "patch") .
+				print $cgi->a({-href =>
+				      href(-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

From: Kevin Cernekee <[email protected]>

When $feature{'pathinfo'} is used, gitweb script sets the base URL to itself, so that relative links to static files work correctly. It does it by adding something like below to HTML head:

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

This breaks the "patch" anchor links seen on the commitdiff pages, because these links, being relative (<a href="#patch1">), are resolved (computed) relative to the base URL and not relative to current URL, i.e. as:

http://HOST/gitweb.cgi#patch1

Instead, they should look like this:

http://HOST/gitweb.cgi/myproject.git/commitdiff/35a9811ef9d68eae9afd76bede121da4f89b448c#patch1

Add an "-anchor" parameter to href(), and use href(-anchor=>"patch1") to generate "patch" anchor links, so that the full path is included in the patch link.

While at it, convert

print "foo";
print "bar";

to

print "foo" .

        "bar";

in the neighborhood of changes.

---
I like v2 version, and had only small corrections. For you to not have to resend this patch once again, I have added those corrections and sent them as a patch myself.

Changes from original v2 version from Kevin Cernekee:

* Fix (re-add) accidentally lost in v2 '"<td class=\"link\">"' in first chunk using href(-anchor => ...)

* Reword commit message (hopefully make it better, and not only longer)

* Move code that sets implicit -replay when -anchor is the only parameter lower, and change order of subexpression, for better possible future extendability, if there would be other cases when we would want to automatically turn on -replay.

* Change indenting of code (whitespace-only change).

* Add 'while at it' fixup.

 gitweb/gitweb.perl |   27 ++++++++++++++++++++-------
 1 files changed, 20 insertions(+), 7 deletions(-)

index b04ab8c..3960d34 100755
@@ -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

@@ -1206,6 +1207,9 @@ sub href {
 
 	$params{'project'} = $project unless exists $params{'project'};
 
+	# implicit -replay
+	$params{-replay} = 1 if (keys %params == 1 && $params{-anchor});
+
 	if ($params{-replay}) {
 		while (my ($name, $symbol) = each %cgi_param_mapping) {
 			if (!exists $params{$name}) {

@@ -1314,6 +1318,10 @@ sub href {
 	# final transformation: trailing spaces must be escaped (URI-encoded)
 	$href =~ s/(\s+)$/CGI::escape($1)/e;
 
+	if ($params{-anchor}) {
+		$href .= "#".esc_param($params{-anchor});
+	}
+
 	return $href;
 }
 

@@ -4335,7 +4343,8 @@ sub git_difftree_body {
 				# link to patch
 				$patchno++;
 				print "<td class=\"link\">" .
-				      $cgi->a({-href => "#patch$patchno"}, "patch") .
+				      $cgi->a({-href => href(-anchor=>"patch$patchno")},
+				              "patch") .
 				      " | " .
 				      "</td>\n";
 			}

@@ -4432,8 +4441,9 @@ sub git_difftree_body {
 			if ($action eq 'commitdiff') {
 				# link to patch
 				$patchno++;
-				print $cgi->a({-href => "#patch$patchno"}, "patch");
-				print " | ";
+				print $cgi->a({-href => href(-anchor=>"patch$patchno")},
+				              "patch") .
+				      " | ";
 			}
 			print $cgi->a({-href => href(action=>"blob", hash=>$diff->{'to_id'},
 			                             hash_base=>$hash, file_name=>$diff->{'file'})},

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

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

@@ -4539,7 +4551,8 @@ sub git_difftree_body {
 			if ($action eq 'commitdiff') {
 				# link to patch
 				$patchno++;
-				print $cgi->a({-href => "#patch$patchno"}, "patch") .
+				print $cgi->a({-href => href(-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.3

Thanks.

I think you meant this part:

	# implicit -replay
	$params{-replay} = 1 if (keys %params == 1 && $params{-anchor});

But I don't think it is that much of an improvement as long as it uses the statement modifier syntax ("A if B") for a thing like this.

I will not bother rewriting the commit I made out of this patch myself, but the next person who wants to add the auto-replay for his option will first have to rewrite the above into:

	if (key %params == 1 && $params{-anchor}) {
		$params{-replay} = 1;
	}

before turning it into:

	if ((key %params == 1 && $params{-anchor}) ||
	    ("here comes my new condition")) {
		$params{-replay} = 1;
	}

anyway. If your rewrite were to a plain-vanilla "if () { ... }", it would have been a real improvement.

Besides, I find it a bad taste to use statement modifier syntax unless the modifying condition ("if B" part) is much more likely to hold true than false.

If you limit your use of statement modifiers for conditions that almost always hold true, it will become much easier to scan the code for the first time, because you can skim through and almost ignore the condition part to follow the logic for the normal case. But turning 'replay' on in a specific narrow case (and later in a specific set of narrow cases) is a complete opposite of normal codeflow.

Anyway, thanks for the clean-up. Applied.

I hope you applied amended version - this one has stupid bug in it.

Jakub Narebski
Poland

If this test occurs after $params{'project'} is set, it needs to count both 'project' and '-anchor':

Kevin Cernekee wrotr:

Right. I'm sorry about that.

The above is not a good solution, as it hides the fact that -anchor must be only parameter for trigger implicit -replay.

-- >8 --
From: Kevin Cernekee <[email protected]>
Subject: [PATCH] gitweb: fix #patchNN anchors when path_info is enabled

When $feature{'pathinfo'} is used, gitweb script sets the base URL to itself, so that relative links to static files work correctly. It does it by adding something like below to HTML head:

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

This breaks the "patch" anchor links seen on the commitdiff pages, because these links, being relative (<a href="#patch1">), are resolved (computed) relative to the base URL and not relative to current URL, i.e. as:

http://HOST/gitweb.cgi#patch1

Instead, they should look like this:

http://HOST/gitweb.cgi/myproject.git/commitdiff/35a9811ef9d68eae9afd76bede121da4f89b448c#patch1

Add an "-anchor" parameter to href(), and use href(-anchor=>"patch1") to generate "patch" anchor links, so that the full path is included in the patch link.

While at it, convert

print "foo";
print "bar";

to

print "foo" .

        "bar";

in the neighborhood of changes.

---
 gitweb/gitweb.perl |   27 ++++++++++++++++++++-------
 1 files changed, 20 insertions(+), 7 deletions(-)

index b04ab8c..f275adb 100755
@@ -1199,11 +1199,15 @@ 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
 	my $href = $params{-full} ? $my_url : $my_uri;
 
+	# implicit -replay, must be first of implicit params
+	$params{-replay} = 1 if (keys %params == 1 && $params{-anchor});
+
 	$params{'project'} = $project unless exists $params{'project'};
 
 	if ($params{-replay}) {

@@ -1314,6 +1318,10 @@ sub href {
 	# final transformation: trailing spaces must be escaped (URI-encoded)
 	$href =~ s/(\s+)$/CGI::escape($1)/e;
 
+	if ($params{-anchor}) {
+		$href .= "#".esc_param($params{-anchor});
+	}
+
 	return $href;
 }
 

@@ -4335,7 +4343,8 @@ sub git_difftree_body {
 				# link to patch
 				$patchno++;
 				print "<td class=\"link\">" .
-				      $cgi->a({-href => "#patch$patchno"}, "patch") .
+				      $cgi->a({-href => href(-anchor=>"patch$patchno")},
+				              "patch") .
 				      " | " .
 				      "</td>\n";
 			}

@@ -4432,8 +4441,9 @@ sub git_difftree_body {
 			if ($action eq 'commitdiff') {
 				# link to patch
 				$patchno++;
-				print $cgi->a({-href => "#patch$patchno"}, "patch");
-				print " | ";
+				print $cgi->a({-href => href(-anchor=>"patch$patchno")},
+				              "patch") .
+				      " | ";
 			}
 			print $cgi->a({-href => href(action=>"blob", hash=>$diff->{'to_id'},
 			                             hash_base=>$hash, file_name=>$diff->{'file'})},

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

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

@@ -4539,7 +4551,8 @@ sub git_difftree_body {
 			if ($action eq 'commitdiff') {
 				# link to patch
 				$patchno++;
-				print $cgi->a({-href => "#patch$patchno"}, "patch") .
+				print $cgi->a({-href => href(-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.3

On the commit/commitdiff views, show the author/committer times as follows:

If $feature{'localtime'} is disabled, display the RFC 2822 date/time in GMT, then print (HH:MM -TZ) in the author's/committer's local timezone. (i.e. no change to the current behavior)

If $feature{'localtime'} is enabled, display the RFC 2822 date/time in the author's/committer's local timezone, then print (HH:MM +0000) in GMT.

---
 gitweb/gitweb.perl |   24 ++++++++++++++----------
 1 files changed, 14 insertions(+), 10 deletions(-)

index 578edc0..6b8f9a7 100755
@@ -3953,22 +3953,27 @@ sub git_print_section {
 	print $cgi->end_div;
 }
 
-sub print_local_time {
-	print format_local_time(@_);
-}
-
-sub format_local_time {
+# If localtime is disabled, show the server's local time in parentheses.
+# If localtime is enabled, show GMT in parentheses.
+sub print_alt_time {
 	my $localtime = '';
 	my %date = @_;
+	my ($h, $m, $tz) = ($date{'hour_local'}, $date{'minute_local'},
+		$date{'tz_local'});
+
+	if (gitweb_check_feature('localtime')) {
+		($h, $m, $tz) = ($date{'hour'}, $date{'minute'}, "+0000");
+	}
+
 	if ($date{'hour_local'} < 6) {
 		$localtime .= sprintf(" (<span class=\"atnight\">%02d:%02d</span> %s)",
-			$date{'hour_local'}, $date{'minute_local'}, $date{'tz_local'});
+			$h, $m, $tz);
 	} else {
 		$localtime .= sprintf(" (%02d:%02d %s)",
-			$date{'hour_local'}, $date{'minute_local'}, $date{'tz_local'});
+			$h, $m, $tz);
 	}
 
-	return $localtime;
+	print $localtime;
 }
 
 # Outputs the author name and date in long form

@@ -3982,7 +3987,6 @@ sub git_print_authorship {
 	print "<$tag class=\"author_date\">" .
 	      format_search_author($author, "author", esc_html($author)) .
 	      " [$ad{'rfc2822'}";
-	print_local_time(%ad) if ($opts{-localtime});
 	print "]" . git_get_avatar($co->{'author_email'}, -pad_before => 1)
 		  . "</$tag>\n";
 }

@@ -4009,7 +4013,7 @@ sub git_print_authorship_rows {
 		      "</td></tr>\n" .
 		      "<tr>" .
 		      "<td></td><td> $wd{'rfc2822'}";
-		print_local_time(%wd) if !gitweb_check_feature('localtime');
+		print_alt_time(%wd);
 		print "</td>" .
 		      "</tr>\n";
 	}

1.7.4.1

From: Kevin Cernekee <[email protected]>

By default, with 'localtime' feature disabled, the dates in 'commit', 'commitdiff' and 'tag' views show both GMT time, and localtime in recorded author/committer/tagger timezone, marking localtime with "atnight" class to notify times between 0 and 6 AM local time.

An example output can look like this:

author A U Thor <[email protected]>

           Wed, 16 Mar 2011 07:02:42 +0000 (02:02 -0500)
                                            ^^^^^

where underlined part is marked with "atnight" class (in red with default stylesheet).

If $feature{'localtime'} is enabled, we display the RFC 2822 date/time in the author's/committer's/tagger's local timezone; previous commit removed marking "atnight" times, because there wasn't separate local time to mark up after GMT time.

This commit makes gitweb mark _whole_ RFC 2822 date/time with "atnight" class for times between 0 and 6 AM.

An example output can look like this:

author A U Thor <[email protected]>

           Wed, 16 Mar 2011 02:02:42 -0500
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

where again underlined part is marked with "atnight".

We probably should mark only time part of RFC 2822 date/time with "atnight" class, but such solution would be more involved.

While at it fix whitespace, using spaces for align, tabs for indent.

NOTE that git_print_authorship subroutine is for now left as is; there is no caller in gitweb that uses it with -localtime=>1.

---
Kevin, how about something like this instead? This preserves _intent_ for why there is local time beside GMT time when 'localtime' is disabled better, I think.

Junio and Kevin, I am not sure if authorship should remain with Kevin, or should it revert to me; the solution is quite different.

About no-change to git_print_authorship: alternate solution would be to remove support for -localtime option, like in original patches.

 gitweb/gitweb.perl |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

index cdc2a96..5bda0a8 100755
@@ -4003,15 +4003,23 @@ sub git_print_authorship_rows {
 		my %wd = parse_date($co->{"${who}_epoch"}, $co->{"${who}_tz"});
 		print "<tr><td>$who</td><td>" .
 		      format_search_author($co->{"${who}_name"}, $who,
-			       esc_html($co->{"${who}_name"})) . " " .
+		                           esc_html($co->{"${who}_name"})) . " " .
 		      format_search_author($co->{"${who}_email"}, $who,
-			       esc_html("<" . $co->{"${who}_email"} . ">")) .
+		                           esc_html("<" . $co->{"${who}_email"} . ">")) .
 		      "</td><td rowspan=\"2\">" .
 		      git_get_avatar($co->{"${who}_email"}, -size => 'double') .
 		      "</td></tr>\n" .
 		      "<tr>" .
-		      "<td></td><td> $wd{'rfc2822'}";
-		print_local_time(%wd) if !gitweb_check_feature('localtime');
+		      "<td></td><td> ";
+		if (gitweb_check_feature('localtime')) {
+			if ($wd{'hour_local'} < 6) {
+				print "<span class=\"atnight\">$wd{'rfc2822'}</span>";
+			} else {
+				print $wd{'rfc2822'};
+			}
+		} else {
+			print $wd{'rfc2822'} . format_local_time(%wd);
+		}
 		print "</td>" .
 		      "</tr>\n";
 	}

1.7.3

I don't think it is worth anything to keep dead code that anybody exercises to support -localtime option that nobody asks.

I thought we were getting closer (especially if you consider my suggestion to the earlier round, but obviously I am biased), but this looks far worse than your previous clean-up of Kevin's patch. What is the point of duplicating the atnight logic her? Why not kill the useless helper function "print-local-time", and instead enhance "format-local-time" so that whatever this added code does is performed there when the caller asks?

Then the caller here would look more or less like:

	print "<tr><td>$who</td>" .
              "... author name, email, avatar ..." .
	      "<td></td><td>" .
              format_timestamp(\%wd, gitweb_check_feature('localtime')) .
	      "</td></tr>\n";

and format_timestamp would be like

	sub format_timestamp {
		my %date = %$_[0];
                my $use_localtime = $_[1];
		my $localtime, $ret, $nite;

		$nite = ($date{'hour_local'} < 6);

		if ($use_localtime) {
			$ret = $date{'rfc2822_local'};
                        if ($nite) {
                        	$ret = sprintf("<span class='atnight'>%s</span>", $ret);
			}
		} else {
			... what the current format_local_time does to set
	                ... including the spanning part
                        $ret = "$date{'rfc2822'} ($localtime)";
		}
		return $ret;
	}

Wouldn't it be much cleaner? You can then clean up the other call site of print_local_time in git_print_authorship using the same helper function (presumably you would always pass 0 to $use_localtime there), no?

Right. Especially that refactoring would significantly change this code.

You are right. I concentrated on the fact that IMVHO Kevin's version of this 3/3 solved wrong issue (the problem is not GMT + local, but atnight which needs localtime), but forgot to stop to think how ugly and duplicated the code is... or at least mark this patch as RFC, request for comments on resulting look.

Right.

Well, if we go this route, and assuming that parse_date does only parsing and we use separate subroutine for generating date in an rfc2822 format, then we could mark only time with "atnight" also when 'localtime' feature is enabled.

Right. Well, I'd have to think a bit about API for format_timestamp, but it looks like good direction.

Jakub Narebski
Poland

I don't think there is much to think about for format_timestamp, as I was suggesting to keep what comes in %date more or less the same as what the current parse_date() generates. I was only hinting that parse_date() is misnamed.

Fine with me. I had to dig around for a while before I could find an "atnight" commit, so I don't think this is something that is likely to occur often in my environment. But I can see how it would be useful to preserve the existing functionality.

I applied your patch and verified it in both localtime=0 and localtime=1 cases. So:

I would suggest reverting it to you.

FWIW, this does create a few >80 character lines. But CodingGuidelines doesn't say whether that limit applies to Perl scripts or just C.

With this feature enabled, all timestamps are shown in the local timezone instead of GMT. The timezone is taken from the appropriate timezone string stored in the commit object.

Affected views include:

summary page, last change field (commit time from latest change) commit page, author/committer
commitdiff page, author/committer
log page, author time

No change to:

relative timestamps
patch page

---
 gitweb/gitweb.perl |   21 ++++++++++++++++++++-
 1 files changed, 20 insertions(+), 1 deletions(-)

index 57a3caf..bf341cb 100755
@@ -504,6 +504,19 @@ our %feature = (
 		'sub' => sub { feature_bool('remote_heads', @_) },
 		'override' => 0,
 		'default' => [0]},
+
+	# Use the author/commit localtime rather than GMT for all timestamps.
+	# Disabled by default.
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'localtime'}{'default'} = [1];
+	# To have project specific config enable override in $GITWEB_CONFIG
+	# $feature{'localtime'}{'override'} = 1;
+	# and in project config gitweb.localtime = 0|1;
+	'localtime' => {
+		'sub' => sub { feature_bool('localtime', @_) },
+		'override' => 0,
+		'default' => [0]},
 );
 
 sub gitweb_get_feature {

@@ -2928,6 +2941,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;
 }
 

@@ -3990,7 +4009,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

From: Kevin Cernekee <[email protected]>

With this feature enabled, all timestamps are shown in the local timezone instead of GMT. The timezone is taken from the appropriate timezone string stored in the commit object.

This is useful if most of contributors (to a project) are based in a single office, all within the same timezone. In such case local time is more useful than GMT / UTC time that gitweb uses by default, and which is better choice for geographically scattered contributors.

This change does not affect relative timestamps (e.g. "5 hours ago"), and neither does it affect 'patch' and 'patches' views which already use localtime because they are generated by "git format-patch".

Affected views include:
* 'summary' view, "last change" field (commit time from latest change)
* 'log' view, author time
* 'commit' and 'commitdiff' views, author/committer time
* 'tag' view, tagger time

In the case of 'commit', 'commitdiff' and 'tag' views gitweb used to print both GMT time and time in timezone of author/tagger/comitter, marking localtime with "atnight" as appropriate; after this commit gitweb shows only local time. Marking localtime with "atnight" when needed is left for subsequent commit.

---
Changes from original v2 version by Kevin Cernekee:

* Expanded commit message, explaining "whys" behind introducing this new feature (why and when can it be useful), as per

    http://thread.gmane.org/gmane.comp.version-control.git/169096/focus=169284

* Minor whitespace changes

 gitweb/gitweb.perl |   21 ++++++++++++++++++++-
 1 files changed, 20 insertions(+), 1 deletions(-)

index 3960d34..1df3652 100755
@@ -504,6 +504,19 @@ our %feature = (
 		'sub' => sub { feature_bool('remote_heads', @_) },
 		'override' => 0,
 		'default' => [0]},
+
+	# Use the author/commit localtime rather than GMT for all timestamps.
+	# Disabled by default.
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'localtime'}{'default'} = [1];
+	# To have project specific config enable override in $GITWEB_CONFIG
+	# $feature{'localtime'}{'override'} = 1;
+	# and in project config gitweb.localtime = 0|1;
+	'localtime' => {
+		'sub' => sub { feature_bool('localtime', @_) },
+		'override' => 0,
+		'default' => [0]},
 );
 
 sub gitweb_get_feature {

@@ -2930,6 +2943,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;
 }
 

@@ -3992,7 +4011,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.3

Thanks for moving the explanation up into the log message. Much easier to understand the motivation.

	print "... some string ..." .
        	"... that is sometimes long ..." .
                "... and more but ends with $bag{'rfc2822'}";
	print_local_time(%bag); # perhaps if "some condition";
	print "... more string ...";

	print "... some string ..." .
        	"... that is sometimes long ..." .
                "... and more but ends with " .
		timestamp_string(%bag, "some condition") .
	        "... more string ...";

[...]

I was concentrating mainly on providing good commit message, but I should probably review also code/change of 2/3 and 3/3 patches as a whole.

That's right. This 2/3 commit, and even more the next 3/3 one shows that handling dates in gitweb really needs refactoring. date_parse does both parsing date and formatting dates, something that made difficult to do 3/3 easily and obviously.

The correct solution would be to replace current 2/3 and 3/3 by two commits:
first refactoring that separates parsing date from formatting date, and second that introduces 'localtime' feature and brings changes to only single place: the new date formatting subroutine.

And similar to 'iso-8601' vs 'iso-tz'.

This would not be needed with proposed by you refactoring that makes parse_date do only parsing of timestamp + timezone.

You are right that is very confusing.

Yes, it is better idea.

Note however that gitweb uses a few different date formats, and in some places it really has to be rfc2822 and nothing else.

Those formats are:

* 'RFC 2822' format used by 'log' view for author date, no place for "atnight" without 'localtime' now; displayed with avatar (gravatar or picon) if enabled

* 'RFC 2822' + local time, with optional "atnight" marker, the local time part is here to be able to put "atnight" warning; used by 'commit', 'commitdiff', 'tag' views; displayed with avatar (gravatar or picon) if enabled

Those two can use "atnight", second should use "atnight" both for default case and when 'localtime' feature is enabled.

* 'RFC 2822' format used by "last change" field in summary part of 'summary' view for a project; no avatar, but perhaps we would want to add relative change

* Strange 'RFC 2822' + timezone in parentheses used by 'commitdiff_plain' view; we should probably use the same as git-format-patch, i.e. always localtime RFC2822 format.

* 'iso-tz' format (ISO 8601, but with ' ' instead of 'Z' as timezone separator) is used in mouseover info in 'blame' view.

* 'Short ISO format' (YYYY-MM-DD) is used together with relative date in many places; it is set during parse_commit. Timezone not shown, but GMT is used. In this place width is quite precious (longest is "NN months ago", I think).

* RFC 2822 as value of 'Last-Modified:' HTTP header in 'feed' actions; I'd have to check if it should be in GMT always, and if it is RFC 2822 or some variation.

* RFC 2822 is format used for dates ('pubDate', 'lastBuildDate') for RSS feed format. I'm not sure if it can be localtime, or must be GMT

* ISO 8601 in UTC / GMT version (with 'Z' as timezone) is used for 'updated' and 'published' dates in Atom feeds. I'd have to check if dates here must be in GMT.

Jakub Narebski
Poland

I didn't mean to propose any such thing. I just pointed out that the function whose name says "parse" does not just parse but does more useful things; I meant to hint that it might want to get renamed, not butchered into smaller pieces