threads / patch / 26796

v2, 3 partsgitweb: rename parse_date() to format_date()

Subject: [PATCH v2 1/3] gitweb: rename parse_date() to format_date()

## tl;dr

12 messages between Mar 19, 2011 and Mar 20, 2011. Diffs are folded; open one to read it.

replies: 11people: 3as markdown or json

Kevin Cernekee· Mar 19, 2011, 20:48 UTC · lore

One might reasonably expect a function named parse_date() to be used for something along these lines:

$unix_time_t = parse_date("2011-03-19");
But instead, gitweb's parse_date works more like:
&parse_date(1300505805, "-0800") = {
        'hour' => 3,
        'minute' => 36,
        ...
        'rfc2822' => 'Sat, 19 Mar 2011 03:36:45 +0000',
        ...
}
Rename the function to improve clarity.  No change to functionality.
Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
---
v2: Add "-0800" to the commit message.  No code changes.
 gitweb/gitweb.perl |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)
Show changes to gitweb/gitweb.perl +9 −9
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index b04ab8c..57ef08c 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2893,7 +2893,7 @@ sub git_get_rev_name_tags {
 ## ----------------------------------------------------------------------
 ## parse to hash functions
 
-sub parse_date {
+sub format_date {
 	my $epoch = shift;
 	my $tz = shift || "-0000";
 
@@ -3953,7 +3953,7 @@ sub git_print_authorship {
 	my $tag = $opts{-tag} || 'div';
 	my $author = $co->{'author_name'};
 
-	my %ad = parse_date($co->{'author_epoch'}, $co->{'author_tz'});
+	my %ad = format_date($co->{'author_epoch'}, $co->{'author_tz'});
 	print "<$tag class=\"author_date\">" .
 	      format_search_author($author, "author", esc_html($author)) .
 	      " [$ad{'rfc2822'}";
@@ -3973,7 +3973,7 @@ sub git_print_authorship_rows {
 	my @people = @_;
 	@people = ('author', 'committer') unless @people;
 	foreach my $who (@people) {
-		my %wd = parse_date($co->{"${who}_epoch"}, $co->{"${who}_tz"});
+		my %wd = format_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"})) . " " .
@@ -4906,7 +4906,7 @@ sub git_log_body {
 		next if !%co;
 		my $commit = $co{'id'};
 		my $ref = format_ref_marker($refs, $commit);
-		my %ad = parse_date($co{'author_epoch'});
+		my %ad = format_date($co{'author_epoch'});
 		git_print_header_div('commit',
 		               "<span class=\"age\">$co{'age_string'}</span>" .
 		               esc_html($co{'title'}) . $ref,
@@ -5369,7 +5369,7 @@ sub git_project_index {
 sub git_summary {
 	my $descr = git_get_project_description($project) || "none";
 	my %co = parse_commit("HEAD");
-	my %cd = %co ? parse_date($co{'committer_epoch'}, $co{'committer_tz'}) : ();
+	my %cd = %co ? format_date($co{'committer_epoch'}, $co{'committer_tz'}) : ();
 	my $head = $co{'id'};
 	my $remote_heads = gitweb_check_feature('remote_heads');
 
@@ -5674,7 +5674,7 @@ sub git_blame_common {
 			my $short_rev = substr($full_rev, 0, 8);
 			my $author = $meta->{'author'};
 			my %date =
-				parse_date($meta->{'author-time'}, $meta->{'author-tz'});
+				format_date($meta->{'author-time'}, $meta->{'author-tz'});
 			my $date = $date{'iso-tz'};
 			if ($group_size) {
 				$current_color = ($current_color + 1) % $num_colors;
@@ -6702,7 +6702,7 @@ sub git_commitdiff {
 			-charset => 'utf-8',
 			-expires => $expires,
 			-content_disposition => 'inline; filename="' . "$filename" . '"');
-		my %ad = parse_date($co{'author_epoch'}, $co{'author_tz'});
+		my %ad = format_date($co{'author_epoch'}, $co{'author_tz'});
 		print "From: " . to_utf8($co{'author'}) . "\n";
 		print "Date: $ad{'rfc2822'} ($ad{'tz_local'})\n";
 		print "Subject: " . to_utf8($co{'title'}) . "\n";
@@ -7064,7 +7064,7 @@ sub git_feed {
 	if (defined($commitlist[0])) {
 		%latest_commit = %{$commitlist[0]};
 		my $latest_epoch = $latest_commit{'committer_epoch'};
-		%latest_date   = parse_date($latest_epoch);
+		%latest_date   = format_date($latest_epoch);
 		my $if_modified = $cgi->http('IF_MODIFIED_SINCE');
 		if (defined $if_modified) {
 			my $since;
@@ -7195,7 +7195,7 @@ XML
 		if (($i >= 20) && ((time - $co{'author_epoch'}) > 48*60*60)) {
 			last;
 		}
-		my %cd = parse_date($co{'author_epoch'});
+		my %cd = format_date($co{'author_epoch'});
 
 		# get list of changed files
 		open my $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
-- 
1.7.4.1
Kevin Cernekee· Mar 19, 2011, 20:48 UTC · re: Kevin Cernekee · lore

[PATCH v5 2/3] gitweb: introduce localtime feature

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 improves usability if the majority of a project's contributors are based in a single office, all within the same timezone. It also makes the interface more friendly to non-developers who may need to track updates, such as program managers and supervisors.

This change does not affect relative timestamps (e.g. "5 hours ago"), nor 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/committer:

   Fri, 18 Mar 2011 01:28:57 +0000 (18:28 -0700)
With localtime enabled, the times will be swapped:
   Thu, 17 Mar 2011 18:28:57 -0700 (01:28 +0000)

When the local time is between 00:00 and 05:59, inclusive, the entire date string will be printed in red ("atnight" style) on the commit and commitdiff views. This is a change from the current behavior, in which only the parenthesized time/tz is colored red. This simplifies the code, and makes the special coloring easier to notice.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---

v5: Change "$use_localtime" to "$commit_view" for clarity. Apply "atnight" style to the entire timestamp string, not just the " (hh:mm -zzzz)" portion. Update commit message to reflect the latter change.

 gitweb/gitweb.perl |   84 +++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 61 insertions(+), 23 deletions(-)
Show changes to gitweb/gitweb.perl +61 −23
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 57ef08c..aa038bd 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -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 {
@@ -2919,6 +2932,10 @@ sub format_date {
 	$date{'hour_local'} = $hour;
 	$date{'minute_local'} = $min;
 	$date{'tz_local'} = $tz;
+	$date{'rfc2822_local'} = sprintf "%s, %d %s %4d %02d:%02d:%02d $tz",
+	                         $days[$wday], $mday, $months[$mon],
+	                         1900+$year, $hour ,$min, $sec;
+
 	$date{'iso-tz'} = sprintf("%04d-%02d-%02d %02d:%02d:%02d %s",
 	                          1900+$year, $mon+1, $mday,
 	                          $hour, $min, $sec, $tz);
@@ -3928,22 +3945,43 @@ sub git_print_section {
 	print $cgi->end_div;
 }
 
-sub print_local_time {
-	print format_local_time(@_);
-}
-
-sub format_local_time {
-	my $localtime = '';
-	my %date = @_;
-	if ($date{'hour_local'} < 6) {
-		$localtime .= sprintf(" (<span class=\"atnight\">%02d:%02d</span> %s)",
-			$date{'hour_local'}, $date{'minute_local'}, $date{'tz_local'});
+# Returns a timestamp string, which may contain HTML.
+# If $commit_view is 0, the string looks like:
+#   Fri, 18 Mar 2011 01:28:57 +0000                [localtime disabled]
+#   Thu, 17 Mar 2011 18:28:57 -0700                [localtime enabled]
+# If $commit_view is 1, the string looks like:
+#   Fri, 18 Mar 2011 01:28:57 +0000 (18:28 -0700)  [localtime disabled]
+#   Thu, 17 Mar 2011 18:28:57 -0700 (01:28 +0000)  [localtime enabled]
+# If $commit_view is 1, the entire string will use the "atnight" class
+# (red text) if the local time is between 00:00 and 05:59 inclusive.
+# This helps to flag commits made in the wee hours of the morning.
+sub timestamp_html {
+	my ($date, $commit_view) = @_;
+	my $timestamp;
+	my $alt_time;
+
+	if (gitweb_check_feature('localtime')) {
+		$timestamp = $date->{'rfc2822_local'};
+		$alt_time = sprintf(" (%02d:%02d %s)",
+		                    $date->{'hour'},
+		                    $date->{'minute'},
+		                    "+0000");
 	} else {
-		$localtime .= sprintf(" (%02d:%02d %s)",
-			$date{'hour_local'}, $date{'minute_local'}, $date{'tz_local'});
+		$timestamp = $date->{'rfc2822'};
+		$alt_time = sprintf(" (%02d:%02d %s)",
+		                    $date->{'hour_local'},
+		                    $date->{'minute_local'},
+		                    $date->{'tz_local'});
 	}
-
-	return $localtime;
+	if ($commit_view) {
+		$timestamp .= $alt_time;
+		if ($date->{'hour_local'} < 6) {
+			$timestamp = "<span class=\"atnight\">" .
+			             $timestamp .
+			             "</span>";
+		}
+	}
+	return $timestamp;
 }
 
 # Outputs the author name and date in long form
@@ -3956,10 +3994,9 @@ sub git_print_authorship {
 	my %ad = format_date($co->{'author_epoch'}, $co->{'author_tz'});
 	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";
+	      " [" . timestamp_html(\%ad, 0) . "] ".
+	      git_get_avatar($co->{'author_email'}, -pad_before => 1) .
+	      "</$tag>\n";
 }
 
 # Outputs table rows containing the full author or committer information,
@@ -3983,9 +4020,9 @@ sub git_print_authorship_rows {
 		      git_get_avatar($co->{"${who}_email"}, -size => 'double') .
 		      "</td></tr>\n" .
 		      "<tr>" .
-		      "<td></td><td> $wd{'rfc2822'}";
-		print_local_time(%wd);
-		print "</td>" .
+		      "<td></td><td> " .
+		      timestamp_html(\%wd, 1) .
+		      "</td>" .
 		      "</tr>\n";
 	}
 }
@@ -5395,8 +5432,9 @@ sub git_summary {
 	print "<table class=\"projects_list\">\n" .
 	      "<tr id=\"metadata_desc\"><td>description</td><td>" . esc_html($descr) . "</td></tr>\n" .
 	      "<tr id=\"metadata_owner\"><td>owner</td><td>" . esc_html($owner) . "</td></tr>\n";
-	if (defined $cd{'rfc2822'}) {
-		print "<tr id=\"metadata_lchange\"><td>last change</td><td>$cd{'rfc2822'}</td></tr>\n";
+	if (keys %cd) {
+		print "<tr id=\"metadata_lchange\"><td>last change</td><td>" .
+		      timestamp_html(\%cd, 0) . "</td></tr>\n";
 	}
 
 	# use per project git URL list in $projectroot/$project/cloneurl
-- 
1.7.4.1
Kevin Cernekee· Mar 19, 2011, 20:48 UTC · re: Kevin Cernekee · lore

[PATCH/RFC 3/3] gitweb: change highlighting of "atnight" commits

The current behavior is to show the timestamp for "atnight" commits (commits for which the local time is between 00:00 and 05:59, inclusive) in red, on the commit and commitdiff views. On the log view, the timestamps are shown normally.

Swap this behavior so that the commit and commitdiff views do not color these timestamps red, but the log view does. This allows late-night commits to be easily identified (and possibly subjected to extra scrutiny) when reading through the change log.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
---

Personally, I am not too concerned with the "atnight" behavior because only a minuscule percentage of our commits happen late at night. But since we're touching this code, we might as well tweak anything in there that needs tweaking.

 gitweb/gitweb.perl |   26 +++++++++++++-------------
 1 files changed, 13 insertions(+), 13 deletions(-)
Show changes to gitweb/gitweb.perl +13 −13
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index aa038bd..2d4349f 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3946,17 +3946,17 @@ sub git_print_section {
 }
 
 # Returns a timestamp string, which may contain HTML.
-# If $commit_view is 0, the string looks like:
+# If -commit_view is not specified, the string looks like:
 #   Fri, 18 Mar 2011 01:28:57 +0000                [localtime disabled]
 #   Thu, 17 Mar 2011 18:28:57 -0700                [localtime enabled]
-# If $commit_view is 1, the string looks like:
+# If -commit_view is specified, the string looks like:
 #   Fri, 18 Mar 2011 01:28:57 +0000 (18:28 -0700)  [localtime disabled]
 #   Thu, 17 Mar 2011 18:28:57 -0700 (01:28 +0000)  [localtime enabled]
-# If $commit_view is 1, the entire string will use the "atnight" class
+# If -atnight is specified, the entire string will use the "atnight" class
 # (red text) if the local time is between 00:00 and 05:59 inclusive.
 # This helps to flag commits made in the wee hours of the morning.
 sub timestamp_html {
-	my ($date, $commit_view) = @_;
+	my ($date, %opts) = @_;
 	my $timestamp;
 	my $alt_time;
 
@@ -3973,13 +3973,13 @@ sub timestamp_html {
 		                    $date->{'minute_local'},
 		                    $date->{'tz_local'});
 	}
-	if ($commit_view) {
+	if ($opts{-commit_view}) {
 		$timestamp .= $alt_time;
-		if ($date->{'hour_local'} < 6) {
-			$timestamp = "<span class=\"atnight\">" .
-			             $timestamp .
-			             "</span>";
-		}
+	}
+	if ($opts{-atnight} && $date->{'hour_local'} < 6) {
+		$timestamp = "<span class=\"atnight\">" .
+			     $timestamp .
+			     "</span>";
 	}
 	return $timestamp;
 }
@@ -3994,7 +3994,7 @@ sub git_print_authorship {
 	my %ad = format_date($co->{'author_epoch'}, $co->{'author_tz'});
 	print "<$tag class=\"author_date\">" .
 	      format_search_author($author, "author", esc_html($author)) .
-	      " [" . timestamp_html(\%ad, 0) . "] ".
+	      " [" . timestamp_html(\%ad, -atnight => 1) . "] ".
 	      git_get_avatar($co->{'author_email'}, -pad_before => 1) .
 	      "</$tag>\n";
 }
@@ -4021,7 +4021,7 @@ sub git_print_authorship_rows {
 		      "</td></tr>\n" .
 		      "<tr>" .
 		      "<td></td><td> " .
-		      timestamp_html(\%wd, 1) .
+		      timestamp_html(\%wd, -commit_view => 1) .
 		      "</td>" .
 		      "</tr>\n";
 	}
@@ -5434,7 +5434,7 @@ sub git_summary {
 	      "<tr id=\"metadata_owner\"><td>owner</td><td>" . esc_html($owner) . "</td></tr>\n";
 	if (keys %cd) {
 		print "<tr id=\"metadata_lchange\"><td>last change</td><td>" .
-		      timestamp_html(\%cd, 0) . "</td></tr>\n";
+		      timestamp_html(\%cd) . "</td></tr>\n";
 	}
 
 	# use per project git URL list in $projectroot/$project/cloneurl
-- 
1.7.4.1
Jakub Narebski· Mar 19, 2011, 22:18 UTC · re: Kevin Cernekee · lore

[PATCH -1/3] gitweb: Always call parse_date with timezone parameter

Timezone is required to correctly set local time, which would be needed for future 'localtime' feature.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
Show changes to gitweb/gitweb.perl +3 −3
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index b04ab8c..0f89ed2 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -4906,7 +4906,7 @@ sub git_log_body {
 		next if !%co;
 		my $commit = $co{'id'};
 		my $ref = format_ref_marker($refs, $commit);
-		my %ad = parse_date($co{'author_epoch'});
+		my %ad = parse_date($co{'author_epoch'}, $co{'athor_tz'});
 		git_print_header_div('commit',
 		               "<span class=\"age\">$co{'age_string'}</span>" .
 		               esc_html($co{'title'}) . $ref,
@@ -7064,7 +7064,7 @@ sub git_feed {
 	if (defined($commitlist[0])) {
 		%latest_commit = %{$commitlist[0]};
 		my $latest_epoch = $latest_commit{'committer_epoch'};
-		%latest_date   = parse_date($latest_epoch);
+		%latest_date   = parse_date($latest_epoch, $latest_commit{'comitter_tz'});
 		my $if_modified = $cgi->http('IF_MODIFIED_SINCE');
 		if (defined $if_modified) {
 			my $since;
@@ -7195,7 +7195,7 @@ XML
 		if (($i >= 20) && ((time - $co{'author_epoch'}) > 48*60*60)) {
 			last;
 		}
-		my %cd = parse_date($co{'author_epoch'});
+		my %cd = parse_date($co{'author_epoch'}, $co{'author_tz'});
 
 		# get list of changed files
 		open my $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
-- 
1.7.3
Jakub Narebski· Mar 19, 2011, 22:53 UTC · re: Jakub Narebski · lore

[PATCH -1/3 (amend)] gitweb: Always call parse_date with timezone parameter

Timezone is required to correctly set local time, which would be needed for future 'localtime' feature.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
I am very sorry, a typo snuck in in previous version

BTW. Kevin, when there is series of patches which are dependent on each other and create together a logical whole, it is recommended to provide cover letter for such "true series".

 gitweb/gitweb.perl |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
Show changes to gitweb/gitweb.perl +3 −3
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index b04ab8c..16abd4c 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -4906,7 +4906,7 @@ sub git_log_body {
 		next if !%co;
 		my $commit = $co{'id'};
 		my $ref = format_ref_marker($refs, $commit);
-		my %ad = parse_date($co{'author_epoch'});
+		my %ad = parse_date($co{'author_epoch'}, $co{'author_tz'});
 		git_print_header_div('commit',
 		               "<span class=\"age\">$co{'age_string'}</span>" .
 		               esc_html($co{'title'}) . $ref,
@@ -7064,7 +7064,7 @@ sub git_feed {
 	if (defined($commitlist[0])) {
 		%latest_commit = %{$commitlist[0]};
 		my $latest_epoch = $latest_commit{'committer_epoch'};
-		%latest_date   = parse_date($latest_epoch);
+		%latest_date   = parse_date($latest_epoch, $latest_commit{'comitter_tz'});
 		my $if_modified = $cgi->http('IF_MODIFIED_SINCE');
 		if (defined $if_modified) {
 			my $since;
@@ -7195,7 +7195,7 @@ XML
 		if (($i >= 20) && ((time - $co{'author_epoch'}) > 48*60*60)) {
 			last;
 		}
-		my %cd = parse_date($co{'author_epoch'});
+		my %cd = parse_date($co{'author_epoch'}, $co{'author_tz'});
 
 		# get list of changed files
 		open my $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
-- 
1.7.3
Kevin Cernekee· Mar 19, 2011, 22:56 UTC · re: Jakub Narebski · lore

Re: [PATCH -1/3] gitweb: Always call parse_date with timezone parameter

On Sat, Mar 19, 2011 at 3:18 PM, Jakub Narebski <jnareb@gmail.com> wrote:
Show 6 quoted lines
> @@ -4906,7 +4906,7 @@ sub git_log_body {
>                next if !%co;
>                my $commit = $co{'id'};
>                my $ref = format_ref_marker($refs, $commit);
> -               my %ad = parse_date($co{'author_epoch'});
> +               my %ad = parse_date($co{'author_epoch'}, $co{'athor_tz'});
Should be 'author_tz'

Looking at the master branch, I don't see %ad actually getting used anywhere? Maybe it is safe to delete the line entirely, since git_print_authorship() calls parse_date() itself.

Show 6 quoted lines
> @@ -7064,7 +7064,7 @@ sub git_feed {
>        if (defined($commitlist[0])) {
>                %latest_commit = %{$commitlist[0]};
>                my $latest_epoch = $latest_commit{'committer_epoch'};
> -               %latest_date   = parse_date($latest_epoch);
> +               %latest_date   = parse_date($latest_epoch, $latest_commit{'comitter_tz'});
Should be 'committer_tz'

I would agree that it isn't such a good thing for $latest_date{'rfc2822_local'} to be set to GMT in this case. Although the feeds don't need local times for anything, since RSS/Atom readers seem to do their own timezone translations.

It probably makes sense to add this argument so that nobody gets bit later when they try to use the rfc2822_local field.

Am I correct in interpreting "PATCH -1/3" as: "apply this before Kevin's set of 3 patches?"

Jakub Narebski· Mar 20, 2011, 00:25 UTC · re: Kevin Cernekee · lore

Re: [PATCH -1/3] gitweb: Always call parse_date with timezone parameter

Kevin Cernekee wrote:
> Am I correct in interpreting "PATCH -1/3" as: "apply this before
> Kevin's set of 3 patches?"

Well, it must be applied before change of name from parse_date... or it would have to be changed.

-- 
Jakub Narebski
Poland
Junio C Hamano· Mar 20, 2011, 00:27 UTC · re: Kevin Cernekee · lore

Re: [PATCH -1/3] gitweb: Always call parse_date with timezone parameter

Kevin Cernekee <cernekee@gmail.com> writes:
Show 7 quoted lines
> On Sat, Mar 19, 2011 at 3:18 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> ...
> Should be 'author_tz'
>
> Looking at the master branch, I don't see %ad actually getting used
> anywhere?  Maybe it is safe to delete the line entirely, since
> git_print_authorship() calls parse_date() itself.

Thanks for being careful, I agree "while at it, remove an unnecessary call" would be better.

Show 6 quoted lines
>> @@ -7064,7 +7064,7 @@ sub git_feed {
> ...
> Should be 'committer_tz'
>
> I would agree that it isn't such a good thing for
> $latest_date{'rfc2822_local'} to be set to GMT in this case.
I would say it isn't good in _any_ case.

If the localtime conversion logic _were_ very expensive, and if the function that fills 'rfc2822_local' _were_ written to allow callers that do not need local time by passing undef to its tz parameter to avoid the computation cost, it may make sense to allow 'rfc2822_local' to be undef, but neither is true. This callsite was simply buggy.

> Am I correct in interpreting "PATCH -1/3" as: "apply this before
> Kevin's set of 3 patches?"

I had the same reaction; judging from the contents, I now realize that is what was meant by "minus one", but originally I read that hyphen as "I don't know how many iterations we had so instead of writing v$N I am just striking it out" ;-)

Kevin Cernekee· Mar 20, 2011, 02:11 UTC · re: Kevin Cernekee · lore

[PATCH v2 4/3] gitweb: Always call format_date with timezone parameter

From: Jakub Narebski <jnareb@gmail.com>

Make the timezone parameter mandatory. This ensures that the *_local fields are always populated with accurate information.

Also, delete an unnecessary call to format_date().
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
---

v2: Fix typos. Remove unnecessary call. Remove default "-0000" tz value. Rebase on top of my patch 3/3 (as applying -1/3 then 1/3 would create a merge conflict).

 gitweb/gitweb.perl |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)
Show changes to gitweb/gitweb.perl +4 −5
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 2d4349f..485ebd0 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2907,8 +2907,7 @@ sub git_get_rev_name_tags {
 ## parse to hash functions
 
 sub format_date {
-	my $epoch = shift;
-	my $tz = shift || "-0000";
+	my ($epoch, $tz) = @_;
 
 	my %date;
 	my @months = ("Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec");
@@ -4943,7 +4942,6 @@ sub git_log_body {
 		next if !%co;
 		my $commit = $co{'id'};
 		my $ref = format_ref_marker($refs, $commit);
-		my %ad = format_date($co{'author_epoch'});
 		git_print_header_div('commit',
 		               "<span class=\"age\">$co{'age_string'}</span>" .
 		               esc_html($co{'title'}) . $ref,
@@ -7102,7 +7100,8 @@ sub git_feed {
 	if (defined($commitlist[0])) {
 		%latest_commit = %{$commitlist[0]};
 		my $latest_epoch = $latest_commit{'committer_epoch'};
-		%latest_date   = format_date($latest_epoch);
+		%latest_date   = format_date($latest_epoch,
+		                             $latest_commit{'committer_tz'});
 		my $if_modified = $cgi->http('IF_MODIFIED_SINCE');
 		if (defined $if_modified) {
 			my $since;
@@ -7233,7 +7232,7 @@ XML
 		if (($i >= 20) && ((time - $co{'author_epoch'}) > 48*60*60)) {
 			last;
 		}
-		my %cd = format_date($co{'author_epoch'});
+		my %cd = format_date($co{'author_epoch'}, $co{'author_tz'});
 
 		# get list of changed files
 		open my $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
-- 
1.7.4.1
Jakub Narebski· Mar 20, 2011, 10:37 UTC · re: Kevin Cernekee · lore

Re: [PATCH v2 4/3] gitweb: Always call format_date with timezone parameter

On Sun, 20 Mar 2011, Kevin Cernekee wrote:
> From: Jakub Narebski <jnareb@gmail.com>
> 
> Make the timezone parameter mandatory.  This ensures that the *_local
> fields are always populated with accurate information.
That is better commit message that what I came with.
 
Show 7 quoted lines
> Also, delete an unnecessary call to format_date().
> 
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
> ---
> 
> v2: Fix typos.  Remove unnecessary call.
Thanks.
> Remove default "-0000" tz value.

Hmmm... I wonder if default value for timezone might have been intended to handle case of malformed date, without timezone.

This fragment
        my $epoch = shift;
        my $tz = shift || "-0000";

is from very beginnings of gitweb... and "v042" is a bit uninformative as a commit message. 991910a (v042, 2005-08-07).

> Rebase on top of my patch 3/3 (as applying -1/3 then 1/3 would create a
> merge conflict).
You are right, that is better solution.
-- 
Jakub Narebski
Poland
Jakub Narebski· Mar 20, 2011, 11:07 UTC · re: Jakub Narebski · lore

Re: [PATCH v2 4/3] gitweb: Always call format_date with timezone parameter

Jakub Narebski wrote:
> On Sun, 20 Mar 2011, Kevin Cernekee wrote:
> > Rebase on top of my patch 3/3 (as applying -1/3 then 1/3 would create a
> > merge conflict).
> 
> You are right, that is better solution.

Well, better for you, as it makes things easier for you: no need to resolve conflict, or to rerun update script:

  $ git checkout HEAD^ -- gitweb/gitweb.perl
  $ sed -e 's/\bparse_date\b/\bformat_date\b/' \
    <gitweb/gitweb.perl >gitweb/gitweb.perl+  &&
    mv -f gitweb/gitweb.perl+ gitweb/gitweb.perl
But it takes bugfix hostage to applying new feature series.
-- 
Jakub Narebski
Poland
Kevin Cernekee· Mar 20, 2011, 20:47 UTC · re: Jakub Narebski · lore

Re: [PATCH v2 4/3] gitweb: Always call format_date with timezone parameter

On Sun, Mar 20, 2011 at 4:07 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> But it takes bugfix hostage to applying new feature series.
I can reorder and resubmit if needed...

But, since the bug isn't actually showing any symptoms in the existing code, and since the fix only depends on patch 1/3 ("rename parse_date" cleanup) rather than 2/3 + 3/3 (localtime + atnight features), I'm hoping this won't be a major point of contention.

← back to recent threads