[PATCH 1/2] gitweb: rename parse_date() to format_date()

[PATCH 1/2] gitweb: rename parse_date() to format_date()

[Kevin Cernekee]

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) = {
        '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>
---
 gitweb/gitweb.perl |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

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

[PATCH v4 2/2] gitweb: introduce localtime feature

[Kevin Cernekee]

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)

Local times between 00:00 and 05:59, inclusive, will still be printed
in red ("atnight" style) in these views.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |   87 ++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 65 insertions(+), 22 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 57ef08c..b3b7f3f 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,48 @@ 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 an RFC 2822 timestamp string, which may contain HTML.
+# If $use_localtime is 0, don't do anything special.
+# If $use_localtime is 1, add an alternate HH:MM timestamp in parentheses at
+# the end.  If $feature{'localtime'} is enabled this looks like:
+#   Thu, 17 Mar 2011 18:28:57 -0700 (01:28 +0000)
+# Otherwise, it looks like:
+#   Fri, 18 Mar 2011 01:28:57 +0000 (18:28 -0700)
+# If $use_localtime is 1, this will also apply the "atnight" style to
+# local times between 00:00 and 05:59.
+sub timestamp_html {
+	my %date = %{$_[0]};
+	my $use_localtime = $_[1];
+	my $timestamp;
+	my $alt_time;
+
+	if (gitweb_check_feature('localtime')) {
+		$timestamp = $date{'rfc2822_local'};
+		if ($use_localtime && $date{'hour_local'} < 6) {
+			$timestamp = "<span class=\"atnight\">" .
+			             $timestamp .
+			             "</span>";
+		}
+		$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'});
+		if ($use_localtime && $date{'hour_local'} < 6) {
+			$alt_time = "<span class=\"atnight\">" .
+			            $alt_time .
+			            "</span>";
+		}
 	}
 
-	return $localtime;
+	if ($use_localtime) {
+		$timestamp .= $alt_time;
+	}
+
+	return $timestamp;
 }
 
 # Outputs the author name and date in long form
@@ -3956,10 +3999,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 +4025,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 +5437,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

Re: [PATCH v4 2/2] gitweb: introduce localtime feature

[Jakub Narebski]

On Sat, 19 Mar 2011, Kevin Cernekee wrote:

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

[...]
> 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)

First, currently the localtime part is needed only to have "atnight"
warning.  I wonder if with 'localtime' feature enabled gitweb should
show GMT time, or is it not necessary.

With 'localtime' it could simply be

     Thu, 17 Mar 2011 18:28:57 -0700

either marked with "atnight" as whole, or only time (see below).

> Local times between 00:00 and 05:59, inclusive, will still be printed
> in red ("atnight" style) in these views.

Second, from above description it is not clear which part of date is
marked with "atnight" style when 'localtime' feature is enabled.

Is the whole localtime part marked, and GMT left not marked:

     Thu, 17 Mar 2011 18:28:57 -0700 (01:28 +0000)
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Or perhaps only the local _time_ is marked with "atnight" style:

     Thu, 17 Mar 2011 18:28:57 -0700 (01:28 +0000)
                      ^^^^^^^^

Or perhaps whole date now uses "atnight" style:

     Thu, 17 Mar 2011 18:28:57 -0700 (01:28 +0000)
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


[...]
> @@ -3928,22 +3945,48 @@ 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 an RFC 2822 timestamp string, which may contain HTML.

I'm not sure if we need to write about it being RFC2822-like format;
it is just implementation detail.

Perhaps

  +# Returns formatted date and time, outputs HTML.

It is important if it is HTML or not, to know if it needs to be esc_html
or not.

> +# If $use_localtime is 0, don't do anything special.
> +# If $use_localtime is 1, add an alternate HH:MM timestamp in parentheses at
> +# the end.

See comment below.

>             If $feature{'localtime'} is enabled this looks like: 
> +#   Thu, 17 Mar 2011 18:28:57 -0700 (01:28 +0000)
> +# Otherwise, it looks like:
> +#   Fri, 18 Mar 2011 01:28:57 +0000 (18:28 -0700)
> +# If $use_localtime is 1, this will also apply the "atnight" style to
> +# local times between 00:00 and 05:59.

I would really prefer to split this patch in two: first refactor 
date-printing code, introducing and using timestamp_html subroutine,
with no changes in output, and leave introducing 'localtime' feature
for a second patch.

> +sub timestamp_html {
> +	my %date = %{$_[0]};
> +	my $use_localtime = $_[1];

Why not use

   	my ($date, $use_localtime) = @_;

and $date->{'rfc2822_local'} instead of $date{'rfc2822_local'}?


Also with current code the calling convention for timestamp_html (or 
format_timestamp_html, or format_date_html) looks like this:

  print     " [" . timestamp_html(\%ad, 0) . "] "

or

  print     timestamp_html(\%wd, 1)

This would require anyone who stumbles upon on a calling site to have
to refer to definition of this function to understand it.

In many other places we use "named parameters" for such boolean flags;
then the calling convention could be

  timestamp_html(\%date)

or

  timestamp_html(\%date, -long=>1)

(or -localtime=>1, or -atnight=>1, etc.).  

We can also/instead provide timestamp_short_html and timestamp_long_html
so one can write

  timestamp_short_html(%date)

and

  timestamp_long_html(%date)

-- 
Jakub Narebski
Poland

Re: [PATCH v4 2/2] gitweb: introduce localtime feature

[Junio C Hamano]

Kevin Cernekee <cernekee@gmail.com> writes:

> -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'});

Looks like we used to only paint HH:MM part but... 

> +# Returns an RFC 2822 timestamp string, which may contain HTML.
> +# If $use_localtime is 0, don't do anything special.
> +# If $use_localtime is 1, add an alternate HH:MM timestamp in parentheses at
> +# the end.  If $feature{'localtime'} is enabled this looks like:
> +#   Thu, 17 Mar 2011 18:28:57 -0700 (01:28 +0000)
> +# Otherwise, it looks like:
> +#   Fri, 18 Mar 2011 01:28:57 +0000 (18:28 -0700)
> +# If $use_localtime is 1, this will also apply the "atnight" style to
> +# local times between 00:00 and 05:59.
> +sub timestamp_html {
> +	my %date = %{$_[0]};
> +	my $use_localtime = $_[1];
> +	my $timestamp;
> +	my $alt_time;
> +
> +	if (gitweb_check_feature('localtime')) {
> +		$timestamp = $date{'rfc2822_local'};
> +		if ($use_localtime && $date{'hour_local'} < 6) {
> +			$timestamp = "<span class=\"atnight\">" .
> +			             $timestamp .
> +			             "</span>";
> +		}
> +		$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'});
> +		if ($use_localtime && $date{'hour_local'} < 6) {
> +			$alt_time = "<span class=\"atnight\">" .
> +			            $alt_time .
> +			            "</span>";
> +		}

... we now paint the whole line, which I personally think is a friendly
move for color challenged people (me included---a larger span of text
painted in different colors tends to help you still notice it better using
value/brightness difference, even if your hue perception is weaker than
other people). But it is a change from the old behaviour and might be
worth stating in the log message.

> -	return $localtime;
> +	if ($use_localtime) {
> +		$timestamp .= $alt_time;
> +	}

You kept -localtime nobody uses? If you dig the history, you would notice
that it is merely a remnant of f88bafa (gitweb: uniform author info for
commit and commitdiff, 2009-06-30) that forgot to remove the feature while
removing the last callsite, and I don't think anybody even was against
such a change.

I'd suggest to remove $alt_time codepath from this function. Nobody will
notice.

>  # Outputs the author name and date in long form
> @@ -3956,10 +3999,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) . "] ".
> ...
>  # Outputs table rows containing the full author or committer information,
> @@ -3983,9 +4025,9 @@ sub git_print_authorship_rows {
> -		print "</td>" .
> +		      "<td></td><td> " .
> +		      timestamp_html(\%wd, 1) .
> +		      "</td>" .

While I don't agree with Jakub about the presense / absence of "[]" around
it (that is not a part of "timestamp", but is how the caller wants to
prepare the space to plug a timestamp string in), I do agree that the
second parameter "use-localtime" looks funny, not because it is an unnamed
parameter (which I personally think is fine) but because it isn't about
the localness of the displayed time anymore.  Your 'localtime' feature
alone controls in which timezone the timestamp is shown, and this only
controls the use of 'atnight' highlighting. The parameter needs to be
renamed, and perhaps you may clarify it further by making it a keyword
argument as Jakub suggests.

Other than that, I think this round is very nicely done.

This is a complete tangent, but I wonder if it makes sense to keep the
output from print_authorship plain (no atnight markings) while painting
the output from print_authorship_rows.

I would understand it if the design _were_ to paint late-nite commits
differently when we list multiple commits in a row (e.g. summary view) to
make them easier to stand out (for amusement purposes), and not to paint
the timestamp when we show only one commit to avoid distraction.  But that
doesn't seem to be the case.

Re: [PATCH v4 2/2] gitweb: introduce localtime feature

[Kevin Cernekee]

On Sat, Mar 19, 2011 at 10:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Looks like we used to only paint HH:MM part but...
> ... we now paint the whole line, which I personally think is a friendly
> move for color challenged people (me included---a larger span of text
> painted in different colors tends to help you still notice it better using
> value/brightness difference, even if your hue perception is weaker than
> other people). But it is a change from the old behaviour and might be
> worth stating in the log message.

For the $feature{'localtime'} disabled case, the coloring is the same as before.

I will paint the whole line in the next spin, and mention it in the
commit message.

>
>> -     return $localtime;
>> +     if ($use_localtime) {
>> +             $timestamp .= $alt_time;
>> +     }
>
> You kept -localtime nobody uses?

> While I don't agree with Jakub about the presense / absence of "[]" around
> it (that is not a part of "timestamp", but is how the caller wants to
> prepare the space to plug a timestamp string in), I do agree that the
> second parameter "use-localtime" looks funny, not because it is an unnamed
> parameter (which I personally think is fine) but because it isn't about
> the localness of the displayed time anymore.  Your 'localtime' feature
> alone controls in which timezone the timestamp is shown, and this only
> controls the use of 'atnight' highlighting. The parameter needs to be
> renamed, and perhaps you may clarify it further by making it a keyword
> argument as Jakub suggests.

$use_localtime indicates whether or not to add the " (hh:mm -zzzz)" at
the end.  This also enables the atnight coloring.

This argument name was suggested in an earlier post and I guess I took
it a little too literally...

Do you think it would be a good idea to take two separate options:
-atnight for the variable coloring, and -alt_time (or some other name)
to show " (hh:mm -zzzz)" after the RFC 2822 string?

Or maybe take one option, named something like "-commitpage", to
indicate that it is a format specific to that view?  If it is not
specified, the caller gets back an uncolored RFC 2822 date.

Also, is there a cleaner way of writing this?

sub timestamp_html {
    my %date = %{$_[0]};
    shift;
    my %opts = @_;

Or should I pass in the options as a hash reference, more like $cgi->a():

sub timestamp_html {
    my %date = %{$_[0]};
    my %opts = %{$_[1]};

Thanks.

Re: [PATCH v4 2/2] gitweb: introduce localtime feature

[Jakub Narebski]

On Sat, 19 Mar 2011, Kevin Cernekee wrote:
> On Sat, Mar 19, 2011 at 10:56 AM, Junio C Hamano <gitster@pobox.com> wrote:

> > Looks like we used to only paint HH:MM part but...
> > ... we now paint the whole line, which I personally think is a friendly
> > move for color challenged people (me included---a larger span of text
> > painted in different colors tends to help you still notice it better using
> > value/brightness difference, even if your hue perception is weaker than
> > other people). But it is a change from the old behaviour and might be
> > worth stating in the log message.
> 
> For the $feature{'localtime'} disabled case, the coloring is the same as before.

No, it is not the same.  It used to be

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

and now it is

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

which is I also think improvement,... but should be mentioned in the
commit message.
 
> I will paint the whole line in the next spin, and mention it in the
> commit message.

I think current solution of using

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

when 'localtime' feature is enabled is better than painting the whole:
you are now painting the _local_ part, i.e the part responsible for
"atnight" warning.


[...]
> $use_localtime indicates whether or not to add the " (hh:mm -zzzz)" at
> the end.  This also enables the atnight coloring.
> 
> This argument name was suggested in an earlier post and I guess I took
> it a little too literally...
> 
> Do you think it would be a good idea to take two separate options:
> -atnight for the variable coloring, and -alt_time (or some other name)
> to show " (hh:mm -zzzz)" after the RFC 2822 string?

Perhaps name this parameter -long.  Or simply always use the long form.

> Or maybe take one option, named something like "-commitpage", to
> indicate that it is a format specific to that view?  If it is not
> specified, the caller gets back an uncolored RFC 2822 date.
> 
> Also, is there a cleaner way of writing this?
> 
> sub timestamp_html {
>     my %date = %{$_[0]};
>     shift;
>     my %opts = @_;

  sub timestamp_html {
      my %date = %{ shift };
      my %opts = @_;


> 
> Or should I pass in the options as a hash reference, more like $cgi->a():
> 
> sub timestamp_html {
>     my %date = %{$_[0]};
>     my %opts = %{$_[1]};

Or just use hash reference for $date:

  sub timestamp_html {
      my ($date, %opts) = @_;

and use $date->{'sth'}.

-- 
Jakub Narebski
Poland

Re: [PATCH v4 2/2] gitweb: introduce localtime feature

[Kevin Cernekee]

On Sat, Mar 19, 2011 at 2:09 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>> For the $feature{'localtime'} disabled case, the coloring is the same as before.
>
> No, it is not the same.  It used to be
>
>  Wed, 16 Mar 2011 07:02:42 +0000 (02:02 -0500)
>                                   ^^^^^
>
> and now it is
>
>  Wed, 16 Mar 2011 07:02:42 +0000 (02:02 -0500)
>                                  ^^^^^^^^^^^^^
> which is I also think improvement,... but should be mentioned in the
> commit message.

Oh OK, I did not notice this or intend to change the formatting.  My bad.

>> I will paint the whole line in the next spin, and mention it in the
>> commit message.
>
> I think current solution of using
>
>  Wed, 16 Mar 2011 02:02:42 -0500 (07:02:42 +0000)
>  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> when 'localtime' feature is enabled is better than painting the whole:
> you are now painting the _local_ part, i.e the part responsible for
> "atnight" warning.

I can fix up v4 if Junio thinks this is a better way to go...

It does make the code a little more complicated, though.  If we paint
the entire string, it can be done once toward the end of the function,
rather than twice (two different ways) inside the localtime /
no-localtime clauses.

I am largely indifferent to how "atnight" is handled but it would be
good to arrive at a consensus.

>> Also, is there a cleaner way of writing this?
>>
>> sub timestamp_html {
>>     my %date = %{$_[0]};
>>     shift;
>>     my %opts = @_;
>
>  sub timestamp_html {
>      my %date = %{ shift };
>      my %opts = @_;

This did not work for me.  It interprets "shift" as a variable name.

> Or just use hash reference for $date:
>
>  sub timestamp_html {
>      my ($date, %opts) = @_;
>
> and use $date->{'sth'}.

That is what I did on the latest 3/3 (change "atnight" highlighting)
patch.  Seemed to work OK.

Thanks.

Re: [PATCH v4 2/2] gitweb: introduce localtime feature

[Jakub Narebski]

Kevin Cernekee wrote:
> On Sat, Mar 19, 2011 at 2:09 PM, Jakub Narebski <jnareb@gmail.com> wrote:

> >  sub timestamp_html {
> >      my %date = %{ shift };
> >      my %opts = @_;
> 
> This did not work for me.  It interprets "shift" as a variable name.

Damn autoquoting ('shift' is taken as bareword here).

Either

        my %date = %{ shift @_ };

or

        my %date = %{ +shift };

works.  Not that it matter much...
-- 
Jakub Narebski
Poland

Re: [PATCH v4 2/2] gitweb: introduce localtime feature

[J.H.]

Hey all,

Sorry for not jumping in on this earlier, too much travel / real world
going on here the past couple of weeks.

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

I'd argue there are two types of "local" time that anyone using gitweb
would be looking for (particularly if this is called local time)

1) Time Local to the observer:  Specifically I don't care where every
other commit has taken place, I want to know what time it was in my
preferred time zone (local time zone likely)

2) Time local to the project:  There will be instances where a project
is based in a specific time zone (home office perhaps?) and you will
want to see the commits from that perspective.

The patch itself (as a commit in gitweb) shows the time + TZ (which is
somewhat useful), but there is something quite useful about the rest of
gitweb only handling a single timezone (GMT/UTC) from the backend (I'll
come back to this point), if for no other reason it makes for uniform
handling of time overall.

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

I'd agree, having multiple different timezones, or trying to think in
UTC/GMT when your not used to it is a pain, and is a valid use case of
gitweb.

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

Agreed.

> 
> 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)
> 
> Local times between 00:00 and 05:59, inclusive, will still be printed
> in red ("atnight" style) in these views.

Ok, while I agree with the use case(s) I think the solution is barking
up completely the wrong tree.  My basic complaint is that this is a
change that effects the backend and ties the backend to a specific TZ,
when this is a front facing / client issue.

While I don't always like JavaScript, this is a situation where I think
it would be a much better solution than doing some extensive changes to
time handling in gitweb.

Basically the change would leave things alone should this be disabled
(you are already doing this, which is good), however should this be
enabled a couple of minor things change:

	1) By default gitweb will continue to display things in UTC.
	   This is a good fallback, and a reasonably safe thing to do
	   should someone have JavaScript disabled.  The reality is
	   most users with it disabled will know or understand what to
	   do with UTC times

	2) Keep the original TZ marked in the html, somewhere hidden on
	   the page is fine

	3) Once a page is loaded attempt to execute the Javascript,
	   which will just cycle through the page and update the Date /
	   Times based on a set of possible (though user choosable
	   options):
		- Local Time (could easily default to this and
		  JavaScript can detect that from the browser)
		- Specific Timezone
		- Default / UTC
		- Original Timezone (from author / commit)

	   Could easily include the original timestamp / utc if
	   Javascript modifies it.  Easy enough to just automatically
	   store the choice (should one be made) in a cookie in the
	   browser, and give the maintainer of the site and easy way
	   to set a rational default given their specific environment.

The obvious advantages:
	- Doesn't give weird data to people behind caching proxies
	- Ability for people working diverse timezones to see things
	  in their local time zone pretty trivially
	- If a site is using gitweb-caching they can take advantage
	  of the feature
	- Won't break bots / scripts that may be crawling the pages or
	  reading the rss feeds (because the timestamps will all be the
	  same assuming it doesn't try to render the javascript)

If you are interested I can bang that out tomorrow (shouldn't take
long), but I would *MUCH* rather see this done via JavaScript than to
muddy up the backend with multiple timezones and such.

- John 'Warthog9' Hawley

Re: [PATCH v4 2/2] gitweb: introduce localtime feature

[Kevin Cernekee]

On Sun, Mar 20, 2011 at 3:38 PM, J.H. <warthog9@eaglescrag.net> wrote:
> I'd argue there are two types of "local" time that anyone using gitweb
> would be looking for (particularly if this is called local time)
>
> 1) Time Local to the observer:  Specifically I don't care where every
> other commit has taken place, I want to know what time it was in my
> preferred time zone (local time zone likely)
>
> 2) Time local to the project:  There will be instances where a project
> is based in a specific time zone (home office perhaps?) and you will
> want to see the commits from that perspective.

That is probably true for distributed projects, but many software
projects are handled entirely at one location so there is only one
timezone of interest.  My goal was to make it easy to support this
case.

I have no particular objection to implementing a more comprehensive
solution, but my gut feeling is that it would be harder to push the
changes upstream (and would have no direct benefit for my local gitweb
installation).

I also figured that if nobody had done this by now, the demand is weak
and it maybe doesn't warrant a huge amount of effort.

>               - Local Time (could easily default to this and
>                 JavaScript can detect that from the browser)

In my case it would be best to have a way to default to the browser's
local time.  Would there be a way to set this default on the server
side?

My group has switched to Git, but most other groups in the company are
using other SCM systems.  I frequently send gitweb links to non-Git
users, and want to make their experience as intuitive as possible.
So, it would be best if the correct timezone was automatically
selected.

>       - Won't break bots / scripts that may be crawling the pages or
>         reading the rss feeds (because the timestamps will all be the
>         same assuming it doesn't try to render the javascript)

FWIW, my latest patches did not change the format of the RSS/Atom feeds.

If we are worried about breaking bots that scrape the regular HTML
pages, that ties our hands in a lot of other ways.

> If you are interested I can bang that out tomorrow (shouldn't take
> long), but I would *MUCH* rather see this done via JavaScript than to
> muddy up the backend with multiple timezones and such.

OK.  I can help test/improve the implementation if you post a prototype.

Thanks.

Re: [PATCH v4 2/2] gitweb: introduce localtime feature

[Jakub Narebski]

On Sun, 20 Mar 2011, J.H. wrote:

> > 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.
> 
> I'd argue there are two types of "local" time that anyone using gitweb
> would be looking for (particularly if this is called local time)
> 
> 1) Time Local to the observer:  Specifically I don't care where every
> other commit has taken place, I want to know what time it was in my
> preferred time zone (local time zone likely)

This can be done only via JavaScript, otherwise how would you get user's
timezone?  Well, you could specify timezone via a form, save it in
a cookie and do conversion to timezone from cookie on server... but this
means more code, and would screw up with output caching if/where 
implemented.
 
> 2) Time local to the project:  There will be instances where a project
> is based in a specific time zone (home office perhaps?) and you will
> want to see the commits from that perspective.

Kevin's patch assumes that geographically concentrated project means all
or almost all commits are generated "in office" and use the same timezone,
which is a timezone of a project.

Currently there is no way to specify _project_ timezone.  Perhaps instead
of 'localtime' feature, which since v2 means also per-project 
`gitweb.localtime' configuration variable, we would allow for 
`gitweb.timezone' configuration variable, which can be "gmt", "utc", or
"localtime" (meaning local to author / committer / tagger).

What do you think?
 
> The patch itself (as a commit in gitweb) shows the time + TZ (which is
> somewhat useful), but there is something quite useful about the rest of
> gitweb only handling a single timezone (GMT/UTC) from the backend (I'll
> come back to this point), if for no other reason it makes for uniform
> handling of time overall.

Single timezone (currently GMT/UTC, perhaps made configurable, perhaps
made client local via JavaScript) is good to compare dates.  Author local
time is good to notice "atnight" commits.

[...]
> > 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".
> 
> Agreed.

[...]
> Ok, while I agree with the use case(s) I think the solution is barking
> up completely the wrong tree.  My basic complaint is that this is a
> change that effects the backend and ties the backend to a specific TZ,
> when this is a front facing / client issue.
> 
> While I don't always like JavaScript, this is a situation where I think
> it would be a much better solution than doing some extensive changes to
> time handling in gitweb.
> 
> Basically the change would leave things alone should this be disabled
> (you are already doing this, which is good), however should this be
> enabled a couple of minor things change:
> 
> 	1) By default gitweb will continue to display things in UTC.
> 	   This is a good fallback, and a reasonably safe thing to do
> 	   should someone have JavaScript disabled.  The reality is
> 	   most users with it disabled will know or understand what to
> 	   do with UTC times
> 
> 	2) Keep the original TZ marked in the html, somewhere hidden on
> 	   the page is fine

We can use what microformats use for date, i.e. 1997-07-16T19:20:30+01:00
or 1997-07-16T19:20:30+0100, in 'date' or 'title' attribute (with 
appropriate microformat class)... or we can use raw git date, i.e. epoch
plus numerical timezone.
 
Note that JavaScript mangling of dates is quite independent on whether
dates are displayed in GMT/UTC or in author / committer / tagger timezone
like for current 'localtime' feature.

> 	3) Once a page is loaded attempt to execute the Javascript,
> 	   which will just cycle through the page and update the Date /
> 	   Times based on a set of possible (though user choosable
> 	   options):
> 		- Local Time (could easily default to this and
> 		  JavaScript can detect that from the browser)
> 		- Specific Timezone
> 		- Default / UTC
> 		- Original Timezone (from author / commit)

Hmmm... we could also automatically update relative dates to reflect
passing of time ;-)

> 	   Could easily include the original timestamp / utc if
> 	   Javascript modifies it.  Easy enough to just automatically
> 	   store the choice (should one be made) in a cookie in the
> 	   browser, and give the maintainer of the site and easy way
> 	   to set a rational default given their specific environment.
> 
> The obvious advantages:
> 	- Doesn't give weird data to people behind caching proxies
> 	- Ability for people working diverse timezones to see things
> 	  in their local time zone pretty trivially
> 	- If a site is using gitweb-caching they can take advantage
> 	  of the feature
> 	- Won't break bots / scripts that may be crawling the pages or
> 	  reading the rss feeds (because the timestamps will all be the
> 	  same assuming it doesn't try to render the javascript)
> 
> If you are interested I can bang that out tomorrow (shouldn't take
> long), but I would *MUCH* rather see this done via JavaScript than to
> muddy up the backend with multiple timezones and such.

Note that we would have to write this JavaScript code...

-- 
Jakub Narebski
Poland

Re: [PATCH v4 2/2] gitweb: introduce localtime feature

[J.H.]

On 03/20/2011 05:20 PM, Jakub Narebski wrote:
> On Sun, 20 Mar 2011, J.H. wrote:
> 
>>> 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.
>>
>> I'd argue there are two types of "local" time that anyone using gitweb
>> would be looking for (particularly if this is called local time)
>>
>> 1) Time Local to the observer:  Specifically I don't care where every
>> other commit has taken place, I want to know what time it was in my
>> preferred time zone (local time zone likely)
> 
> This can be done only via JavaScript, otherwise how would you get user's
> timezone?  Well, you could specify timezone via a form, save it in
> a cookie and do conversion to timezone from cookie on server... but this
> means more code, and would screw up with output caching if/where 
> implemented.

I think this would screw up less caching vs. more caching w/ Javascript.
 Without doing this with Javascript having any hard coded timezone like
what's proposed here is bad, particularly if for some reason two people
can select different time zones for whatever reason (not an unreasonable
extension of what's been proposed)

>> 2) Time local to the project:  There will be instances where a project
>> is based in a specific time zone (home office perhaps?) and you will
>> want to see the commits from that perspective.
> 
> Kevin's patch assumes that geographically concentrated project means all
> or almost all commits are generated "in office" and use the same timezone,
> which is a timezone of a project.
> 
> Currently there is no way to specify _project_ timezone.  Perhaps instead
> of 'localtime' feature, which since v2 means also per-project 
> `gitweb.localtime' configuration variable, we would allow for 
> `gitweb.timezone' configuration variable, which can be "gmt", "utc", or
> "localtime" (meaning local to author / committer / tagger).
> 
> What do you think?

Not specifically thinking of setting a per-repo timezone, when I was
thinking of "project" I was thinking of the entire gitweb install (I.E.
git.kernel.org as a project or pkgs.fedoraproject.org or drupal.org as a
project)

I would think, if we are doing this in JavaScript, that this is an over
complication to do on a per-repo basis.  I would guess setting a generic
default and letting people deviate makes more sense.  Just my thought
anyway, but I think there's a lot of extra per-repo configurations that
add complexity with minimal gain.

>> The patch itself (as a commit in gitweb) shows the time + TZ (which is
>> somewhat useful), but there is something quite useful about the rest of
>> gitweb only handling a single timezone (GMT/UTC) from the backend (I'll
>> come back to this point), if for no other reason it makes for uniform
>> handling of time overall.
> 
> Single timezone (currently GMT/UTC, perhaps made configurable, perhaps
> made client local via JavaScript) is good to compare dates.  Author local
> time is good to notice "atnight" commits.
> 
> [...]
>>> 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".
>>
>> Agreed.
> 
> [...]
>> Ok, while I agree with the use case(s) I think the solution is barking
>> up completely the wrong tree.  My basic complaint is that this is a
>> change that effects the backend and ties the backend to a specific TZ,
>> when this is a front facing / client issue.
>>
>> While I don't always like JavaScript, this is a situation where I think
>> it would be a much better solution than doing some extensive changes to
>> time handling in gitweb.
>>
>> Basically the change would leave things alone should this be disabled
>> (you are already doing this, which is good), however should this be
>> enabled a couple of minor things change:
>>
>> 	1) By default gitweb will continue to display things in UTC.
>> 	   This is a good fallback, and a reasonably safe thing to do
>> 	   should someone have JavaScript disabled.  The reality is
>> 	   most users with it disabled will know or understand what to
>> 	   do with UTC times
>>
>> 	2) Keep the original TZ marked in the html, somewhere hidden on
>> 	   the page is fine
> 
> We can use what microformats use for date, i.e. 1997-07-16T19:20:30+01:00
> or 1997-07-16T19:20:30+0100, in 'date' or 'title' attribute (with 
> appropriate microformat class)... or we can use raw git date, i.e. epoch
> plus numerical timezone.

Well we already have the time in the html, it's just the original TZ
that would need to get added.  Though it's still likely easier to store
the timestamp + tz than use the microformats in the JS.

> Note that JavaScript mangling of dates is quite independent on whether
> dates are displayed in GMT/UTC or in author / committer / tagger timezone
> like for current 'localtime' feature.

I wasn't intending to change the current formatting of the times we have
in place currently.  Just allowing people to change what time was being
displayed.

>> 	3) Once a page is loaded attempt to execute the Javascript,
>> 	   which will just cycle through the page and update the Date /
>> 	   Times based on a set of possible (though user choosable
>> 	   options):
>> 		- Local Time (could easily default to this and
>> 		  JavaScript can detect that from the browser)
>> 		- Specific Timezone
>> 		- Default / UTC
>> 		- Original Timezone (from author / commit)
> 
> Hmmm... we could also automatically update relative dates to reflect
> passing of time ;-)

Yup, which could also go into changing the "22 minutes ago" kind of
stuff in real time (beyond just dates).  Bonus to cached pages.

>> 	   Could easily include the original timestamp / utc if
>> 	   Javascript modifies it.  Easy enough to just automatically
>> 	   store the choice (should one be made) in a cookie in the
>> 	   browser, and give the maintainer of the site and easy way
>> 	   to set a rational default given their specific environment.
>>
>> The obvious advantages:
>> 	- Doesn't give weird data to people behind caching proxies
>> 	- Ability for people working diverse timezones to see things
>> 	  in their local time zone pretty trivially
>> 	- If a site is using gitweb-caching they can take advantage
>> 	  of the feature
>> 	- Won't break bots / scripts that may be crawling the pages or
>> 	  reading the rss feeds (because the timestamps will all be the
>> 	  same assuming it doesn't try to render the javascript)
>>
>> If you are interested I can bang that out tomorrow (shouldn't take
>> long), but I would *MUCH* rather see this done via JavaScript than to
>> muddy up the backend with multiple timezones and such.
> 
> Note that we would have to write this JavaScript code...

I'll mock something up tomorrow.

- John 'Warthog9' Hawley

Re: [PATCH v4 2/2] gitweb: introduce localtime feature

[Jakub Narebski]

On Mon, 21 Mar 2011 at 03:35, J.H. wrote:
> On 03/20/2011 05:20 PM, Jakub Narebski wrote:
>> On Sun, 20 Mar 2011, J.H. wrote:
>> 
>>>> 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.
>>>
>>> I'd argue there are two types of "local" time that anyone using gitweb
>>> would be looking for (particularly if this is called local time)
>>>
>>> 1) Time Local to the observer:  Specifically I don't care where every
>>> other commit has taken place, I want to know what time it was in my
>>> preferred time zone (local time zone likely)
>> 
>> This can be done only via JavaScript, otherwise how would you get user's
>> timezone?  Well, you could specify timezone via a form, save it in
>> a cookie and do conversion to timezone from cookie on server... but this
>> means more code, and would screw up with output caching if/where 
>> implemented.
> 
> I think this would screw up less caching vs. more caching w/ Javascript.
> Without doing this with Javascript having any hard coded timezone like
> what's proposed here is bad, particularly if for some reason two people
> can select different time zones for whatever reason (not an unreasonable
> extension of what's been proposed)

First, it would complicate caching, as output would depend not only purely
on URL, but can also depend on cookies.  Cache key would have to take it
into account.

Second, it would reduce effectiveness of cache, as single page would have
to have multiple versions (up to 24, one per timezone, I guess).

>>> 2) Time local to the project:  There will be instances where a project
>>> is based in a specific time zone (home office perhaps?) and you will
>>> want to see the commits from that perspective.
>> 
>> Kevin's patch assumes that geographically concentrated project means all
>> or almost all commits are generated "in office" and use the same timezone,
>> which is a timezone of a project.
>> 
>> Currently there is no way to specify _project_ timezone.  Perhaps instead
>> of 'localtime' feature, which since v2 means also per-project 
>> `gitweb.localtime' configuration variable, we would allow for 
>> `gitweb.timezone' configuration variable, which can be "gmt", "utc", or
>> "localtime" (meaning local to author / committer / tagger).
>> 
>> What do you think?
> 
> Not specifically thinking of setting a per-repo timezone, when I was
> thinking of "project" I was thinking of the entire gitweb install (I.E.
> git.kernel.org as a project or pkgs.fedoraproject.org or drupal.org as a
> project)

Well, if we are doing it on server side, then by having 'timezone' feature
rather than 'localtime' one from Kevin patches, we would have per-repo
configuration "for free".

> I would think, if we are doing this in JavaScript, that this is an over
> complication to do on a per-repo basis.  I would guess setting a generic
> default and letting people deviate makes more sense.  Just my thought
> anyway, but I think there's a lot of extra per-repo configurations that
> add complexity with minimal gain.

It it is set _by client_, then of course it doesn't make much sense to
make it configurable per-repository.  Besides it would be difficult to
implement and store in JavaScript, I think.

Per-repository settings makes sense for _server-side_; different 
repositories may have different character: some might be geographically
distributed, some might have all contributors in single timezone.

[...]
>>> Basically the change would leave things alone should this be disabled
>>> (you are already doing this, which is good), however should this be
>>> enabled a couple of minor things change:
>>>
>>> 	1) By default gitweb will continue to display things in UTC.
>>> 	   This is a good fallback, and a reasonably safe thing to do
>>> 	   should someone have JavaScript disabled.  The reality is
>>> 	   most users with it disabled will know or understand what to
>>> 	   do with UTC times
>>>
>>> 	2) Keep the original TZ marked in the html, somewhere hidden on
>>> 	   the page is fine
>> 
>> We can use what microformats use for date, i.e. 1997-07-16T19:20:30+01:00
>> or 1997-07-16T19:20:30+0100, in 'date' or 'title' attribute (with 
>> appropriate microformat class)... or we can use raw git date, i.e. epoch
>> plus numerical timezone.
> 
> Well we already have the time in the html, it's just the original TZ
> that would need to get added.  Though it's still likely easier to store
> the timestamp + tz than use the microformats in the JS.

I agree that epoch + timezone should be easier to manipulate in JavaScript
than ISO-8601 or similar format... I think.  Note that at least with epoch
timezone is necessary only if you want to display date in author etc. zone;
if you want to use client (web browser) timezone, it is not necessary.

Note that _where and how_ timestamp is stored for JavaScript manipulation
would become gitweb API as much as its links are.  I wonder if HTML5 says
anything on attribute values used for machine-readable data; microformats
site says that using "title" for non human-readable stuff is deprecated
for accessibility reasons.

>> Note that JavaScript mangling of dates is quite independent on whether
>> dates are displayed in GMT/UTC or in author / committer / tagger timezone
>> like for current 'localtime' feature.
> 
> I wasn't intending to change the current formatting of the times we have
> in place currently.  Just allowing people to change what time was being
> displayed.

I wanted to say that JavaScript post-processing of dates is a bit 
orthogonal to David's server-side localtime (or similar) feature.

>>> 	3) Once a page is loaded attempt to execute the Javascript,
>>> 	   which will just cycle through the page and update the Date /
>>> 	   Times based on a set of possible (though user choosable
>>> 	   options):
>>> 		- Local Time (could easily default to this and
>>> 		  JavaScript can detect that from the browser)
>>> 		- Specific Timezone
>>> 		- Default / UTC
>>> 		- Original Timezone (from author / commit)
>> 
>> Hmmm... we could also automatically update relative dates to reflect
>> passing of time ;-)
> 
> Yup, which could also go into changing the "22 minutes ago" kind of
> stuff in real time (beyond just dates).  Bonus to cached pages.

...though this should probably made it into separate commit.
 
-- 
Jakub Narebski
Poland

Re: [PATCH v4 2/2] gitweb: introduce localtime feature

[Piotr Krukowiecki]

On Mon, Mar 21, 2011 at 5:01 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Mon, 21 Mar 2011 at 03:35, J.H. wrote:
>> On 03/20/2011 05:20 PM, Jakub Narebski wrote:
>>> On Sun, 20 Mar 2011, J.H. wrote:
>>>
>>>>> 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.
>>>>
>>>> I'd argue there are two types of "local" time that anyone using gitweb
>>>> would be looking for (particularly if this is called local time)
>>>>
>>>> 1) Time Local to the observer:  Specifically I don't care where every
>>>> other commit has taken place, I want to know what time it was in my
>>>> preferred time zone (local time zone likely)
>>>
>>> This can be done only via JavaScript, otherwise how would you get user's
>>> timezone?  Well, you could specify timezone via a form, save it in
>>> a cookie and do conversion to timezone from cookie on server... but this
>>> means more code, and would screw up with output caching if/where
>>> implemented.
>>
>> I think this would screw up less caching vs. more caching w/ Javascript.
>> Without doing this with Javascript having any hard coded timezone like
>> what's proposed here is bad, particularly if for some reason two people
>> can select different time zones for whatever reason (not an unreasonable
>> extension of what's been proposed)
>
> First, it would complicate caching, as output would depend not only purely
> on URL, but can also depend on cookies.  Cache key would have to take it
> into account.
>
> Second, it would reduce effectiveness of cache, as single page would have
> to have multiple versions (up to 24, one per timezone, I guess).

It would reduce the effectiveness only if it's really needed. If everyone works
from one timezone, the cache have only one version. Only if there are users
from multiple timezones, which means the feature is really required, the cache
would have multiple versions.

In my case we have main office in one place, but we get commits from
several other timezones. So although the "one timezone for project" would
benefit most people, some might still not be very happy.

-- 
Piotr Krukowiecki

Re: [PATCH v4 2/2] gitweb: introduce localtime feature

[J.H.]

On 03/21/2011 09:01 AM, Jakub Narebski wrote:
> On Mon, 21 Mar 2011 at 03:35, J.H. wrote:
>> On 03/20/2011 05:20 PM, Jakub Narebski wrote:
>>> On Sun, 20 Mar 2011, J.H. wrote:
>>>
>>>>> 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.
>>>>
>>>> I'd argue there are two types of "local" time that anyone using gitweb
>>>> would be looking for (particularly if this is called local time)
>>>>
>>>> 1) Time Local to the observer:  Specifically I don't care where every
>>>> other commit has taken place, I want to know what time it was in my
>>>> preferred time zone (local time zone likely)
>>>
>>> This can be done only via JavaScript, otherwise how would you get user's
>>> timezone?  Well, you could specify timezone via a form, save it in
>>> a cookie and do conversion to timezone from cookie on server... but this
>>> means more code, and would screw up with output caching if/where 
>>> implemented.
>>
>> I think this would screw up less caching vs. more caching w/ Javascript.
>> Without doing this with Javascript having any hard coded timezone like
>> what's proposed here is bad, particularly if for some reason two people
>> can select different time zones for whatever reason (not an unreasonable
>> extension of what's been proposed)
> 
> First, it would complicate caching, as output would depend not only purely
> on URL, but can also depend on cookies.  Cache key would have to take it
> into account.
> 
> Second, it would reduce effectiveness of cache, as single page would have
> to have multiple versions (up to 24, one per timezone, I guess).

The implementation I have running in the back of my head would be
completely independent, so I'm not sure what implementation you are
thinking of that would cause this.  One of the reasons I'm suggesting
Javascript at all is to avoid exactly this problem.

>>>> 2) Time local to the project:  There will be instances where a project
>>>> is based in a specific time zone (home office perhaps?) and you will
>>>> want to see the commits from that perspective.
>>>
>>> Kevin's patch assumes that geographically concentrated project means all
>>> or almost all commits are generated "in office" and use the same timezone,
>>> which is a timezone of a project.
>>>
>>> Currently there is no way to specify _project_ timezone.  Perhaps instead
>>> of 'localtime' feature, which since v2 means also per-project 
>>> `gitweb.localtime' configuration variable, we would allow for 
>>> `gitweb.timezone' configuration variable, which can be "gmt", "utc", or
>>> "localtime" (meaning local to author / committer / tagger).
>>>
>>> What do you think?
>>
>> Not specifically thinking of setting a per-repo timezone, when I was
>> thinking of "project" I was thinking of the entire gitweb install (I.E.
>> git.kernel.org as a project or pkgs.fedoraproject.org or drupal.org as a
>> project)
> 
> Well, if we are doing it on server side, then by having 'timezone' feature
> rather than 'localtime' one from Kevin patches, we would have per-repo
> configuration "for free".

Doing it on the server overly complicates things, breaks web caches,
gitweb-caching and generally I'd argue is not what we want.  It would
work for small installations, in particular those not using
gitweb-caching, but honestly this is a useful enough feature for a more
distributed audience it needs to work with gitweb-caching, without
putting undue burden on the caching engine (as a whole).  Javascript is
the obvious, and simple answer to that.  Won't add any substantial
complexity to the backend, and will allow for more complex choices and
instant changes (without involving the backend at all).

That pretty much sounds like a win to me anyway...

>> I would think, if we are doing this in JavaScript, that this is an over
>> complication to do on a per-repo basis.  I would guess setting a generic
>> default and letting people deviate makes more sense.  Just my thought
>> anyway, but I think there's a lot of extra per-repo configurations that
>> add complexity with minimal gain.
> 
> It it is set _by client_, then of course it doesn't make much sense to
> make it configurable per-repository.  Besides it would be difficult to
> implement and store in JavaScript, I think.

Cookies solve the problem, and those are basically trivial to deal with
in Javascript.

> Per-repository settings makes sense for _server-side_; different 
> repositories may have different character: some might be geographically
> distributed, some might have all contributors in single timezone.

Agreed, since I've been arguing for the client side work, I think we are
on the same page here.

> [...]
>>>> Basically the change would leave things alone should this be disabled
>>>> (you are already doing this, which is good), however should this be
>>>> enabled a couple of minor things change:
>>>>
>>>> 	1) By default gitweb will continue to display things in UTC.
>>>> 	   This is a good fallback, and a reasonably safe thing to do
>>>> 	   should someone have JavaScript disabled.  The reality is
>>>> 	   most users with it disabled will know or understand what to
>>>> 	   do with UTC times
>>>>
>>>> 	2) Keep the original TZ marked in the html, somewhere hidden on
>>>> 	   the page is fine
>>>
>>> We can use what microformats use for date, i.e. 1997-07-16T19:20:30+01:00
>>> or 1997-07-16T19:20:30+0100, in 'date' or 'title' attribute (with 
>>> appropriate microformat class)... or we can use raw git date, i.e. epoch
>>> plus numerical timezone.
>>
>> Well we already have the time in the html, it's just the original TZ
>> that would need to get added.  Though it's still likely easier to store
>> the timestamp + tz than use the microformats in the JS.
> 
> I agree that epoch + timezone should be easier to manipulate in JavaScript
> than ISO-8601 or similar format... I think.  Note that at least with epoch
> timezone is necessary only if you want to display date in author etc. zone;
> if you want to use client (web browser) timezone, it is not necessary.
> 
> Note that _where and how_ timestamp is stored for JavaScript manipulation
> would become gitweb API as much as its links are.  I wonder if HTML5 says
> anything on attribute values used for machine-readable data; microformats
> site says that using "title" for non human-readable stuff is deprecated
> for accessibility reasons.

I wouldn't entirely agree with that.  While I'd say sticking the hidden
data into a commonly accepted and suggested place would be ideal,
looking back over the wonderful world of Javascript indicates it's a
giant messy jungle as ever.  Since what I'm also thinking will be just
about completely independent of the backend, I wouldn't call what I'm
doing / suggesting here an API, and in particular one I would not
declare as externally usable - I.E. if we have to change this to be more
compliant in a year or two (particularly once microformats are more
generally agreed upon) I don't think it would break anyone or anything.

That said from what I can find of microformats (today) the most widely
agreed upon date formatting is either to use a full hCalendar (this
would be completely unweildly and if that's the "right" way to do it, I
would suggest we ignore it), or to use a date time element, of which
there are no specific ones for commit / applied / etc (I see no reason
why we can't just start using them to define the standard fundamentally)

<span class="dtcommit" title="YYYY-MM-DDTHH:MM:SSZ"></span>

or

<span class="dtcommit">
	<abbr class="value" title="2011-03-11">March 21st, 2011</abbr>
	<span class="value"> 18:30 </span>
</span>

But to be clear all that is currently accpeted (as far as I can tell)
for microformats with respect to this are dtstart (to specify the start
of an event, more specifically like I'm having a party at dtstart) and
dtend - when the event ends.  There, as far as I can tell, isn't the
idea of a single point microformat which would be more useful to us, and
doing the start/end properly would bloat the page a lot (for index pages
it would be substantial)

Also keeping in mind that miroformats are almost explicitly for the
benefit of search engines, which I'm not sure if a majority of gitweb
installations care about, it may not be worthwhile to actually go down
that road.

>>> Note that JavaScript mangling of dates is quite independent on whether
>>> dates are displayed in GMT/UTC or in author / committer / tagger timezone
>>> like for current 'localtime' feature.
>>
>> I wasn't intending to change the current formatting of the times we have
>> in place currently.  Just allowing people to change what time was being
>> displayed.
> 
> I wanted to say that JavaScript post-processing of dates is a bit 
> orthogonal to David's server-side localtime (or similar) feature.

It's a different direction, yes, but it's definitely not orthogonal.  If
you do this on the server side, as David originally suggested, you can
create a mess in other places (external proxies, external web caches,
gitweb-caching, etc).  I like his idea, just not where it's implemented
at current.  If you leave the server side alone, and deal with the
processing of the dates on the client side you alleviate all of the
server side gotchas, while only introducing one or two gotchas to the
client side.

This isn't an orthogonal discussion, it's a discussion of where the
right place / right solution to accomplish the same goal is.

>>>> 	3) Once a page is loaded attempt to execute the Javascript,
>>>> 	   which will just cycle through the page and update the Date /
>>>> 	   Times based on a set of possible (though user choosable
>>>> 	   options):
>>>> 		- Local Time (could easily default to this and
>>>> 		  JavaScript can detect that from the browser)
>>>> 		- Specific Timezone
>>>> 		- Default / UTC
>>>> 		- Original Timezone (from author / commit)
>>>
>>> Hmmm... we could also automatically update relative dates to reflect
>>> passing of time ;-)
>>
>> Yup, which could also go into changing the "22 minutes ago" kind of
>> stuff in real time (beyond just dates).  Bonus to cached pages.
> 
> ...though this should probably made it into separate commit.

I'd agree.

- John 'Warthog9' Hawley

Re: [PATCH v4 2/2] gitweb: introduce localtime feature

[Jakub Narebski]

J.H. wrote:
> On 03/21/2011 09:01 AM, Jakub Narebski wrote:

> > First, it would complicate caching, as output would depend not only purely
> > on URL, but can also depend on cookies.  Cache key would have to take it
> > into account.
> > 
> > Second, it would reduce effectiveness of cache, as single page would have
> > to have multiple versions (up to 24, one per timezone, I guess).
> 
> The implementation I have running in the back of my head would be
> completely independent, so I'm not sure what implementation you are
> thinking of that would cause this.  One of the reasons I'm suggesting
> Javascript at all is to avoid exactly this problem.

Well, of course if output is the same, only post-processed by JavaScript
(and no server-side fallback), then there is no interference between
timezones and output caching.
 
N.B. caching of git output, or of parsed data, allows to generate multiple
outputs from one cached value... but OTOH output caching is simpler.

[...]
> > Well, if we are doing it on server side, then by having 'timezone' feature
> > rather than 'localtime' one from Kevin patches, we would have per-repo
> > configuration "for free".
> 
> Doing it on the server overly complicates things, breaks web caches,
> gitweb-caching and generally I'd argue is not what we want.  It would
> work for small installations, in particular those not using
> gitweb-caching, but honestly this is a useful enough feature for a more
> distributed audience it needs to work with gitweb-caching, without
> putting undue burden on the caching engine (as a whole).  Javascript is
> the obvious, and simple answer to that.  Won't add any substantial
> complexity to the backend, and will allow for more complex choices and
> instant changes (without involving the backend at all).
> 
> That pretty much sounds like a win to me anyway...

Note that proposed "on server" solution, like the one in Kevin's patches,
would not include a way to select a way of presenting dates by client: it
all would be set on server, and not interfere with output caching.

As to complexity: you would either have complexity in backend, or 
complexity in JavaScript...


But I agree that JavaScript enhancement solution is better.

> > > I would think, if we are doing this in JavaScript, that this is an over
> > > complication to do on a per-repo basis.  I would guess setting a generic
> > > default and letting people deviate makes more sense.  Just my thought
> > > anyway, but I think there's a lot of extra per-repo configurations that
> > > add complexity with minimal gain.
> > 
> > It it is set _by client_, then of course it doesn't make much sense to
> > make it configurable per-repository.  Besides it would be difficult to
> > implement and store in JavaScript, I think.
> 
> Cookies solve the problem, and those are basically trivial to deal with
> in Javascript.

I was talking about difficulty of _per-repo_ configuration with 
_client-side_ scripting.  For each [visited] repository we would have to
store config: too much for a cookie (well, perhaps localStorage... ;-)).
 
> > Per-repository settings makes sense for _server-side_; different 
> > repositories may have different character: some might be geographically
> > distributed, some might have all contributors in single timezone.
> 
> Agreed, since I've been arguing for the client side work, I think we are
> on the same page here.

[...]

> Also keeping in mind that miroformats are almost explicitly for the
> benefit of search engines, which I'm not sure if a majority of gitweb
> installations care about, it may not be worthwhile to actually go down
> that road.

Right.
 
> > > > Note that JavaScript mangling of dates is quite independent on whether
> > > > dates are displayed in GMT/UTC or in author / committer / tagger timezone
> > > > like for current 'localtime' feature.
> > >
> > > I wasn't intending to change the current formatting of the times we have
> > > in place currently.  Just allowing people to change what time was being
> > > displayed.
> > 
> > I wanted to say that JavaScript post-processing of dates is a bit 
> > orthogonal to David's server-side localtime (or similar) feature.
> 
> It's a different direction, yes, but it's definitely not orthogonal.  If
> you do this on the server side, as David originally suggested, you can
> create a mess in other places (external proxies, external web caches,
> gitweb-caching, etc).  I like his idea, just not where it's implemented
> at current.  If you leave the server side alone, and deal with the
> processing of the dates on the client side you alleviate all of the
> server side gotchas, while only introducing one or two gotchas to the
> client side.
> 
> This isn't an orthogonal discussion, it's a discussion of where the
> right place / right solution to accomplish the same goal is.

Well, perhaps it is not orthogonal discussion, but you can those two
_slightly different_ solutions:
 * server-side, per-repository config, no client choice
 * client-side, JavaScript enhancements, global per-client choice,
   client choice stored in cookie

might work together... though it probably doesn't make much sense.

-- 
Jakub Narebski
Poland

[PATCH 0/1] Gitweb: Change timezone

[John 'Warthog9' Hawley]

This is just a javascript implementation of Kevin's localtime
feature.  It's pretty straight forward, though date handling in
Javascript is, special (head bangingly so).

This should be good to run on any browser, with the safe fallback
of UTC being the default output should Javascript not work / be
available.

John 'Warthog9' Hawley (1):
  gitweb: javascript ability to adjust time based on timezone

 gitweb/gitweb.perl              |   64 +++++++++++++++-
 gitweb/static/js/common-defs.js |   12 +++
 gitweb/static/js/common-lib.js  |   32 ++++++++
 gitweb/static/js/cookies.js     |   35 +++++++++
 gitweb/static/js/date.js        |  160 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 300 insertions(+), 3 deletions(-)
 create mode 100644 gitweb/static/js/common-defs.js
 create mode 100644 gitweb/static/js/common-lib.js
 create mode 100644 gitweb/static/js/cookies.js
 create mode 100644 gitweb/static/js/date.js

-- 
1.7.2.3

[PATCH 1/1] gitweb: javascript ability to adjust time based on timezone

[John 'Warthog9' Hawley]

This patch is based on Kevin Cernekee's <cernekee@gmail.com>
patch series entitled "gitweb: introduce localtime feature".  While
Kevin's patch changed the server side output so that the timezone
was output from gitweb itself, this has a number of drawbacks, in
particular with respect to gitweb-caching.

This patch takes the same basic goal, display the appropriate times
in a given timezone, and implements it in Javascript.  This requires
adding / using a new class, dtcommit, which is based on the
dtstart/dtend microformats.  Appropriate commit dates are wrapped in
a span with this class, and a title of the time in ISO8601 format.

enabling this will then add several javascript files to be loaded,
a default timezone to be selected and a single onloadTZSetup()
function to be called.

This will place a "+" to the right of an appropriate time and clicking
on it will give you a drop down to choose the timezone to change to.

All changes are saved to a Javascript cookie, so page changes and
browser closure / reopening retains the last known setting used.

Fallback (should Javascript not be enabled) is to treat dates as
they have been and display them, only, in UTC.

Valid timezones, currently, are:

utc
local
-1200
-1100
...
+1100
+1200

With each timezone being +1hr to the previous.  The code is capable of
handling fractional timezones, but those have not been added.

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

Based-on-code-from: Kevin Cernekee <cernekee@gmail.com>
Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
---
 gitweb/gitweb.perl              |   64 +++++++++++++++-
 gitweb/static/js/common-defs.js |   12 +++
 gitweb/static/js/common-lib.js  |   32 ++++++++
 gitweb/static/js/cookies.js     |   35 +++++++++
 gitweb/static/js/date.js        |  160 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 300 insertions(+), 3 deletions(-)
 create mode 100644 gitweb/static/js/common-defs.js
 create mode 100644 gitweb/static/js/common-lib.js
 create mode 100644 gitweb/static/js/cookies.js
 create mode 100644 gitweb/static/js/date.js

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 9dccfb0..2a662d8 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -261,6 +261,49 @@ our %highlight_ext = (
 	map { $_ => 'xml' } qw(xhtml html htm),
 );
 
+
+# Enable / Disable the ability for gitweb to use a small amount of 
+# javascript, along with a javascript cookie, to display commit
+# times in the preffered timezone.  Time zones can be pre-set 
+# by specifying it in $jslocaltime with one of the following:
+#
+#	utc
+#	local
+#	-1200
+#	-1100
+#	-1000
+#	-0900
+#	-0800
+#	-0700
+#	-0600
+#	-0500
+#	-0400
+#	-0300
+#	-0200
+#	-0100
+#	+0000
+#	+0100
+#	+0200
+#	+0300
+#	+0400
+#	+0500
+#	+0600
+#	+0700
+#	+0800
+#	+0900
+#	+1000
+#	+1100
+#	+1200
+#
+# utc is identical to +0000, and local will use the
+# timezone that the browser thinks it is in.
+#
+# Enabled by default.
+#
+# To disable set to null or ''
+
+our $jslocaltime = 'local';
+
 # You define site-wide feature defaults here; override them with
 # $GITWEB_CONFIG as necessary.
 our %feature = (
@@ -504,6 +547,7 @@ our %feature = (
 		'sub' => sub { feature_bool('remote_heads', @_) },
 		'override' => 0,
 		'default' => [0]},
+
 );
 
 sub gitweb_get_feature {
@@ -3728,6 +3772,19 @@ sub git_footer_html {
 		      qq!</script>\n!;
 	}
 
+	print "<!-- jslocaltime: $jslocaltime -->";
+	if( $jslocaltime ne '' ){
+		my $js_path = File::Basename::dirname($javascript);
+		print qq!<script type="text/javascript" src="!. esc_url( $js_path ."/js/common-defs.js" )	.qq!"></script>\n!;
+		print qq!<script type="text/javascript" src="!. esc_url( $js_path ."/js/common-lib.js" )	.qq!"></script>\n!;
+		print qq!<script type="text/javascript" src="!. esc_url( $js_path ."/js/date.js" )		.qq!"></script>\n!;
+		print qq!<script type="text/javascript" src="!. esc_url( $js_path ."/js/cookies.js" )		.qq!"></script>\n!;
+		print "<script type=\"text/javascript\">\n".
+				"var tzDefault = \"$jslocaltime\";\n".
+				"onloadTZSetup();\n".
+		      "</script>";
+	}
+
 	print "</body>\n" .
 	      "</html>";
 }
@@ -3956,7 +4013,7 @@ sub git_print_authorship {
 	my %ad = parse_date($co->{'author_epoch'}, $co->{'author_tz'});
 	print "<$tag class=\"author_date\">" .
 	      format_search_author($author, "author", esc_html($author)) .
-	      " [$ad{'rfc2822'}";
+	      " [<span class=\"dtcommit\" title=\"$ad{'iso-8601'}\">$ad{'rfc2822'}</span>";
 	print_local_time(%ad) if ($opts{-localtime});
 	print "]" . git_get_avatar($co->{'author_email'}, -pad_before => 1)
 		  . "</$tag>\n";
@@ -3983,7 +4040,7 @@ sub git_print_authorship_rows {
 		      git_get_avatar($co->{"${who}_email"}, -size => 'double') .
 		      "</td></tr>\n" .
 		      "<tr>" .
-		      "<td></td><td> $wd{'rfc2822'}";
+		      "<td></td><td> <span class=\"dtcommit\" title=\"$wd{'iso-8601'}\">$wd{'rfc2822'}</span>";
 		print_local_time(%wd);
 		print "</td>" .
 		      "</tr>\n";
@@ -5395,7 +5452,8 @@ sub git_summary {
 	      "<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";
+		print "<tr id=\"metadata_lchange\"><td>last change</td>\n";
+		print "<td><span class=\"dtcommit\" title=\"$cd{'iso-8601'}\">$cd{'rfc2822'}</span></td></tr>\n";
 	}
 
 	# use per project git URL list in $projectroot/$project/cloneurl
diff --git a/gitweb/static/js/common-defs.js b/gitweb/static/js/common-defs.js
new file mode 100644
index 0000000..23a1cc2
--- /dev/null
+++ b/gitweb/static/js/common-defs.js
@@ -0,0 +1,12 @@
+// Copyright (C) 2011, John 'Warthog9' Hawley <warthog9@eaglescrag.net>
+//
+//
+// JavaScript common globals for gitweb (git web interface).
+// @license GPLv2 or later
+//
+
+// Common universal values get set here
+// Things like cookie names, and other just
+// basic global variables.
+
+getwebCookieTZOffset = "tzOffset";
diff --git a/gitweb/static/js/common-lib.js b/gitweb/static/js/common-lib.js
new file mode 100644
index 0000000..2ce65ec
--- /dev/null
+++ b/gitweb/static/js/common-lib.js
@@ -0,0 +1,32 @@
+// Copyright (C) 2011, John 'Warthog9' Hawley <warthog9@eaglescrag.net>
+//
+//
+// JavaScript date handling code for gitweb (git web interface).
+// @license GPLv2 or later
+//
+//
+function findElementsByClassName( className ) {
+	if( document.getElementsByClassName == undefined) {
+		var hasClassName = new RegExp("(?:^|\\s)" + className + "(?:$|\\s)");
+		var allElements = document.getElementsByTagName("*");
+		var foundElements = [];
+
+		var element = null;
+		for (var x = 0; (element = allElements[x]) != null; x++) {
+			var curClass = element.className;
+			if(
+				curClass				// If we've actually got something
+				&&
+				curClass.indexOf(className) != -1	// And it has a valid index, I.E. not -1
+				&&
+				hasClassName.test(curClass)		// and the regex passes
+			) {
+				foundElements.push(element);		// push it onto the results stack
+			}
+		}
+
+		return foundElements;
+	}else{
+		return document.getElementsByClassName( className );
+	}
+}
diff --git a/gitweb/static/js/cookies.js b/gitweb/static/js/cookies.js
new file mode 100644
index 0000000..8c8f7aa
--- /dev/null
+++ b/gitweb/static/js/cookies.js
@@ -0,0 +1,35 @@
+// Copyright (C) 2011, John 'Warthog9' Hawley <warthog9@eaglescrag.net>
+//
+//
+// JavaScript cookie handling code for gitweb (git web interface).
+// @license GPLv2 or later
+//
+
+function setCookieExp( name, value, expires ){
+	var expDate = new Date( expires.toString() );
+	expires = expDate.toUTCString;
+	document.cookie = escape(name) +"="+ escape(value) +";"+ expDate.toUTCString() +";path=/";
+}
+
+function setCookie( name, value ){
+	var txtCookie = name +"=\""+ value +"\";path=/";
+	document.cookie = txtCookie;
+}
+
+function getCookie( name ){
+	var allCookies = document.cookie.split(";");
+	var value = "";
+
+	for( var x = 0; x < allCookies.length; x++ ){
+		var brokenCookie = allCookies[x].split("=",2);
+		var hasName = new RegExp("^\\s*" + name + "\\s*$");
+		if(
+			hasName.test(brokenCookie[0])	// Check for the name of the cookie based on the regex
+			&&
+			brokenCookie.length == 2	// Just making sure there is something to actually return here
+		){
+			return unescape(brokenCookie[1]);
+		}
+	}
+	return null;
+}
diff --git a/gitweb/static/js/date.js b/gitweb/static/js/date.js
new file mode 100644
index 0000000..a6d6f81
--- /dev/null
+++ b/gitweb/static/js/date.js
@@ -0,0 +1,160 @@
+// Copyright (C) 2011, John 'Warthog9' Hawley <warthog9@eaglescrag.net>
+//
+//
+// JavaScript date handling code for gitweb (git web interface).
+// @license GPLv2 or later
+//
+
+function onloadTZSetup(){
+	addChangeTZ();
+	tzChangeNS( tzDefault );
+	checkTZCookie();
+}
+
+function addChangeTZ() {
+		var txtClassesFound = "";
+		var classesFound = findElementsByClassName( "dtcommit" );
+		txtClassesFound += "Length: "+ classesFound.length +"<br>\n";
+		for ( x = 0; x < classesFound.length; x++){
+			curElement = classesFound[x];
+			txtClassesFound += "<br>\n"+ x +" - "+ curElement.nodeName  +" - "+ curElement.title +" - "+ curElement.innerHTML +"<br>\n";
+			var strExtra = " <span onclick=\"clickDate(event.target);\" title=\"+\">+</span>"
+			curElement.innerHTML = curElement.innerHTML + strExtra;
+		}
+}
+
+function checkTZCookie(){
+	var preSetTZ = getCookie( getwebCookieTZOffset );
+	if(
+		preSetTZ != null
+		&&
+		preSetTZ.length != 0
+	){
+		tzChange( preSetTZ );
+	}
+}
+
+function formatTZ( tzOffset ) {
+		var posNeg = "+";
+		if( tzOffset < 0 ){
+			posNeg = "-";
+		}
+		tzOffset = Math.sqrt( Math.pow( tzOffset, 2 ) );
+		if( tzOffset < 100 ){
+			tzOffset = tzOffset * 100;
+		}
+		for( y = tzOffset.toString().length + 1; y <= 4; y++ ){
+			tzOffset = "0"+ tzOffset;
+		}
+		return posNeg + tzOffset;
+}
+
+function dateOutput( objDate ) {
+	return dateOutputTZ( objDate, "0" );
+}
+
+function dateOutputTZ( objDate, tzOffset ) {
+	var strDate = "";
+	var daysOfWeek = [ "Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat" ];
+	var monthsOfYr = [ "Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec" ];
+
+	if( tzOffset == "utc" ){
+		tzOffset = 0;
+	}else if( tzOffset == "local" ){
+		var tempDate = new Date();
+		tzOffset = tempDate.getTimezoneOffset() * -1 / 60 * 100; 
+		tzOffset = formatTZ( tzOffset );
+	}
+
+	var msPerHr = 1000 * 60 * 60;	// 1000ms/sec * 60sec/min * 60sec/hr
+	var toDateTime = new Date();
+	toDateTime.setTime( objDate.getTime() + ( tzOffset / 100 * msPerHr ) );
+
+	// Current Date Formatting:
+	// Fri, 19 Dec 2008 11:35:33 +0000
+	strDate += daysOfWeek[ toDateTime.getUTCDay() ] +", ";
+	strDate += toDateTime.getUTCDate() +" ";
+	strDate += monthsOfYr[ toDateTime.getUTCMonth() ] +" ";
+	strDate += toDateTime.getUTCFullYear() +" ";
+	strDate += toDateTime.getUTCHours() +":";
+	strDate += toDateTime.getUTCMinutes() +":";
+	strDate += toDateTime.getUTCSeconds() +" ";
+
+	strDate += tzOffset;
+
+	return strDate;
+}
+
+function tzChange( tzOffset ){
+	return tzChangeSNS( tzOffset, true );
+}
+
+function tzChangeNS( tzOffset ){
+	return tzChangeSNS( tzOffset, false );
+}
+
+function tzChangeSNS( tzOffset, set ){
+	var txtClassesFound = "";
+	var classesFound = findElementsByClassName( "dtcommit" );
+	for ( x = 0; x < classesFound.length; x++){
+		curElement = classesFound[x];
+		var origDateTime = new Date( curElement.title );
+		curElement.innerHTML = dateOutputTZ(origDateTime, tzOffset);
+	}
+	var tzExpDate = new Date();
+	tzExpDate.setDate( tzExpDate.getDate() + 180 );
+	if( set == true ){
+		setCookieExp( getwebCookieTZOffset, tzOffset, tzExpDate.toUTCString() );
+	}
+	addChangeTZ();
+}
+
+function clickDate( clkEvent ) {
+	if( clkEvent.title == "+" ){
+		clkEvent.title="-";
+
+		var preSetTZ = getCookie( getwebCookieTZOffset );
+		
+		var arrSelected = new Array();
+		var offsetArr = 14;
+		arrSelected[0] = " ";
+		arrSelected[1] = " ";
+		if( preSetTZ == "utc" ) {
+			arrSelected[0] = " selected=\"selected\" ";
+		} else if( preSetTZ == "local" ){
+			arrSelected[1] = " selected=\"selected\" ";
+		}
+		for( x = -12; x <= 12; x++){
+			arrSelected[x + offsetArr] = "";
+			if( ( x * 100 ) == preSetTZ ){
+				arrSelected[x + offsetArr] = " selected=\"selected\" ";
+			}
+		}
+		var txtTzSelect = " \
+<span style=\"width: 10%;background-color: grey;\">- \
+<table border=\"1\">\
+	<tr>\
+		<td>\
+			Time Zone:\
+		</td>\
+		<td>\
+			<select name=\"tzoffset\" onchange=\"tzChange(this.value);\">\
+				<option "+ arrSelected[0] +" value=\"utc\">UTC/GMT</option>\
+				<option "+ arrSelected[1] +" value=\"local\">Local (per browser)</option>";
+		for( x = -12; x <= 12; x++){
+			var tzOffset = formatTZ( x );
+			txtTzSelect +="					<option "+ arrSelected[x + offsetArr] +"value=\""+ tzOffset +"\">"+ tzOffset +"</option>";
+		}
+		txtTzSelect += "\
+			</select>\
+		</td>\
+	</tr>\
+</table>\
+</span>";
+		clkEvent.innerHTML = txtTzSelect;
+	}else{
+		clkEvent.parentNode.title="+";
+		clkEvent.parentNode.innerHTML = "+";
+	}
+}
+
-- 
1.7.2.3

Re: [PATCH 1/1] gitweb: javascript ability to adjust time based on timezone

[Kevin Cernekee]

[-- Attachment #1: Type: text/plain, Size: 2801 bytes --]

On Wed, Mar 23, 2011 at 5:08 PM, John 'Warthog9' Hawley
<warthog9@eaglescrag.net> wrote:
> This patch takes the same basic goal, display the appropriate times
> in a given timezone, and implements it in Javascript.  This requires
> adding / using a new class, dtcommit, which is based on the
> dtstart/dtend microformats.  Appropriate commit dates are wrapped in
> a span with this class, and a title of the time in ISO8601 format.

John,

Thanks for coding this up.  I tested it on a couple of different
browsers and wanted to share my observations with you.

First, the easy stuff:

1) "git am" complains about whitespace violations

2) HH:MM:SS times need zero padding; otherwise you see:

Tue, 8 Mar 2011 20:29:9 -0700

3) Some of the Javascript functions are double-indented, others single-indented.

4) IE6 does not seem to like ISO 8601 format:

x = new Date("2011-03-09T03:29:09Z");

This sets all fields to NaN.  I suspect that getTime() values
(milliseconds since 1970-01-01) are more portable.

I have attached a trivial patch for these four items; it applies on
top of your original submission.


Some other things that popped up:

5) Some timezone offsets are not a whole number of hours.  Bangalore
time is GMT +0530, for instance.

6) Most U.S. timezones honor daylight savings, so they could be
something like -0700 for part of the year, and -0800 for the rest of
the year.  Picking the "local" option would automatically adjust for
this, but DST limits the usefulness of permanently storing a fixed TZ
offset in the cookie.

7) Looking at a pre-DST commit after DST (or vice versa) can be a
little confusing:

Tue, 8 Mar 2011 20:29:09 -0700 + (19:29 -0800)

I'm not sure which time to believe.  (Although it's likely that a few
weeks after a commit, the exact hour doesn't matter.)

8) The " + " popup menu is a little quirky.  On FF 3.6 it partially
collapses after selecting a value from the dropdown.  On IE6 it shows
"Error in parsing value for 'display'" and does not render.  On Opera
11 it seemed to work OK.

Firefox breakage: http://img217.imageshack.us/f/firefoxa.png/

I'm wondering if there might be a better place on the page to put the
TZ selection.  It isn't immediately obvious to the user what the extra
" + " does, and it seems to cause some issues.

If you decide to keep it where it is, you might want to consider
absolute or fixed positioning so that other elements do not wrap
around it.  IOW it would work more like the dropdown menus on many
sites.

The timezone fixup javascript seemed to work reasonably well, except
for the hiccup with IE6.  Maybe it would be worth splitting this into
two patches: one to rewrite the timestamps, and a second one to add
the TZ selection interface.

[-- Attachment #2: 0001-gitweb-minor-fixups-to-javascript-localtime-feature.patch --]
[-- Type: application/octet-stream, Size: 6656 bytes --]

From fe8d5ba1d3922cd368691d47bb5f605b9e2bed58 Mon Sep 17 00:00:00 2001
From: Kevin Cernekee <cernekee@gmail.com>
Date: Wed, 23 Mar 2011 22:00:31 -0700
Subject: [PATCH] gitweb: minor fixups to javascript localtime feature

Fix whitespace, indentation, HH:MM:SS zero padding, IE6 compatibility.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
---
 gitweb/gitweb.perl       |   11 ++++---
 gitweb/static/js/date.js |   63 +++++++++++++++++++++++++--------------------
 2 files changed, 41 insertions(+), 33 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index dfceaf6..1d5e970 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -262,9 +262,9 @@ our %highlight_ext = (
 );
 
 
-# Enable / Disable the ability for gitweb to use a small amount of 
+# Enable / Disable the ability for gitweb to use a small amount of
 # javascript, along with a javascript cookie, to display commit
-# times in the preffered timezone.  Time zones can be pre-set 
+# times in the preferred timezone.  Time zones can be pre-set
 # by specifying it in $jslocaltime with one of the following:
 #
 #	utc
@@ -2956,6 +2956,7 @@ sub parse_date {
 	                     $mday, $months[$mon], $hour ,$min;
 	$date{'iso-8601'}  = sprintf "%04d-%02d-%02dT%02d:%02d:%02dZ",
 	                     1900+$year, 1+$mon, $mday, $hour ,$min, $sec;
+	$date{'getTime'}   = $epoch * 1000;
 
 	$tz =~ m/^([+\-][0-9][0-9])([0-9][0-9])$/;
 	my $local = $epoch + ((int $1 + ($2/60)) * 3600);
@@ -4013,7 +4014,7 @@ sub git_print_authorship {
 	my %ad = parse_date($co->{'author_epoch'}, $co->{'author_tz'});
 	print "<$tag class=\"author_date\">" .
 	      format_search_author($author, "author", esc_html($author)) .
-	      " [<span class=\"dtcommit\" title=\"$ad{'iso-8601'}\">$ad{'rfc2822'}</span>";
+	      " [<span class=\"dtcommit\" title=\"$ad{'getTime'}\">$ad{'rfc2822'}</span>";
 	print_local_time(%ad) if ($opts{-localtime});
 	print "]" . git_get_avatar($co->{'author_email'}, -pad_before => 1)
 		  . "</$tag>\n";
@@ -4040,7 +4041,7 @@ sub git_print_authorship_rows {
 		      git_get_avatar($co->{"${who}_email"}, -size => 'double') .
 		      "</td></tr>\n" .
 		      "<tr>" .
-		      "<td></td><td> <span class=\"dtcommit\" title=\"$wd{'iso-8601'}\">$wd{'rfc2822'}</span>";
+		      "<td></td><td> <span class=\"dtcommit\" title=\"$wd{'getTime'}\">$wd{'rfc2822'}</span>";
 		print_local_time(%wd);
 		print "</td>" .
 		      "</tr>\n";
@@ -5454,7 +5455,7 @@ sub git_summary {
 	      "<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>\n";
-		print "<td><span class=\"dtcommit\" title=\"$cd{'iso-8601'}\">$cd{'rfc2822'}</span></td></tr>\n";
+		print "<td><span class=\"dtcommit\" title=\"$cd{'getTime'}\">$cd{'rfc2822'}</span></td></tr>\n";
 	}
 
 	# use per project git URL list in $projectroot/$project/cloneurl
diff --git a/gitweb/static/js/date.js b/gitweb/static/js/date.js
index a6d6f81..1728bda 100644
--- a/gitweb/static/js/date.js
+++ b/gitweb/static/js/date.js
@@ -12,15 +12,15 @@ function onloadTZSetup(){
 }
 
 function addChangeTZ() {
-		var txtClassesFound = "";
-		var classesFound = findElementsByClassName( "dtcommit" );
-		txtClassesFound += "Length: "+ classesFound.length +"<br>\n";
-		for ( x = 0; x < classesFound.length; x++){
-			curElement = classesFound[x];
-			txtClassesFound += "<br>\n"+ x +" - "+ curElement.nodeName  +" - "+ curElement.title +" - "+ curElement.innerHTML +"<br>\n";
-			var strExtra = " <span onclick=\"clickDate(event.target);\" title=\"+\">+</span>"
-			curElement.innerHTML = curElement.innerHTML + strExtra;
-		}
+	var txtClassesFound = "";
+	var classesFound = findElementsByClassName( "dtcommit" );
+	txtClassesFound += "Length: "+ classesFound.length +"<br>\n";
+	for ( x = 0; x < classesFound.length; x++){
+		curElement = classesFound[x];
+		txtClassesFound += "<br>\n"+ x +" - "+ curElement.nodeName  +" - "+ curElement.title +" - "+ curElement.innerHTML +"<br>\n";
+		var strExtra = " <span onclick=\"clickDate(event.target);\" title=\"+\">+</span>"
+		curElement.innerHTML = curElement.innerHTML + strExtra;
+	}
 }
 
 function checkTZCookie(){
@@ -35,18 +35,26 @@ function checkTZCookie(){
 }
 
 function formatTZ( tzOffset ) {
-		var posNeg = "+";
-		if( tzOffset < 0 ){
-			posNeg = "-";
-		}
-		tzOffset = Math.sqrt( Math.pow( tzOffset, 2 ) );
-		if( tzOffset < 100 ){
-			tzOffset = tzOffset * 100;
-		}
-		for( y = tzOffset.toString().length + 1; y <= 4; y++ ){
-			tzOffset = "0"+ tzOffset;
-		}
-		return posNeg + tzOffset;
+	var posNeg = "+";
+	if( tzOffset < 0 ){
+		posNeg = "-";
+	}
+	tzOffset = Math.sqrt( Math.pow( tzOffset, 2 ) );
+	if( tzOffset < 100 ){
+		tzOffset = tzOffset * 100;
+	}
+	for( y = tzOffset.toString().length + 1; y <= 4; y++ ){
+		tzOffset = "0"+ tzOffset;
+	}
+	return posNeg + tzOffset;
+}
+
+function padTime( val ) {
+	if( val < 10 ){
+		return "0" + val.toString();
+	} else {
+		return val.toString();
+	}
 }
 
 function dateOutput( objDate ) {
@@ -62,7 +70,7 @@ function dateOutputTZ( objDate, tzOffset ) {
 		tzOffset = 0;
 	}else if( tzOffset == "local" ){
 		var tempDate = new Date();
-		tzOffset = tempDate.getTimezoneOffset() * -1 / 60 * 100; 
+		tzOffset = tempDate.getTimezoneOffset() * -1 / 60 * 100;
 		tzOffset = formatTZ( tzOffset );
 	}
 
@@ -76,9 +84,9 @@ function dateOutputTZ( objDate, tzOffset ) {
 	strDate += toDateTime.getUTCDate() +" ";
 	strDate += monthsOfYr[ toDateTime.getUTCMonth() ] +" ";
 	strDate += toDateTime.getUTCFullYear() +" ";
-	strDate += toDateTime.getUTCHours() +":";
-	strDate += toDateTime.getUTCMinutes() +":";
-	strDate += toDateTime.getUTCSeconds() +" ";
+	strDate += padTime(toDateTime.getUTCHours()) +":";
+	strDate += padTime(toDateTime.getUTCMinutes()) +":";
+	strDate += padTime(toDateTime.getUTCSeconds()) +" ";
 
 	strDate += tzOffset;
 
@@ -98,7 +106,7 @@ function tzChangeSNS( tzOffset, set ){
 	var classesFound = findElementsByClassName( "dtcommit" );
 	for ( x = 0; x < classesFound.length; x++){
 		curElement = classesFound[x];
-		var origDateTime = new Date( curElement.title );
+		var origDateTime = new Date( parseInt(curElement.title) );
 		curElement.innerHTML = dateOutputTZ(origDateTime, tzOffset);
 	}
 	var tzExpDate = new Date();
@@ -114,7 +122,7 @@ function clickDate( clkEvent ) {
 		clkEvent.title="-";
 
 		var preSetTZ = getCookie( getwebCookieTZOffset );
-		
+
 		var arrSelected = new Array();
 		var offsetArr = 14;
 		arrSelected[0] = " ";
@@ -157,4 +165,3 @@ function clickDate( clkEvent ) {
 		clkEvent.parentNode.innerHTML = "+";
 	}
 }
-
-- 
1.7.4.1

Re: [PATCH 1/1] gitweb: javascript ability to adjust time based on timezone

[J.H.]

Going to make some quick comments on this, try and get things more along
tomorrow (mainly just wanted to get this out today so the commentary
could start)

On 03/23/2011 10:23 PM, Kevin Cernekee wrote:
> On Wed, Mar 23, 2011 at 5:08 PM, John 'Warthog9' Hawley
> <warthog9@eaglescrag.net> wrote:
>> This patch takes the same basic goal, display the appropriate times
>> in a given timezone, and implements it in Javascript.  This requires
>> adding / using a new class, dtcommit, which is based on the
>> dtstart/dtend microformats.  Appropriate commit dates are wrapped in
>> a span with this class, and a title of the time in ISO8601 format.
> 
> John,
> 
> Thanks for coding this up.  I tested it on a couple of different
> browsers and wanted to share my observations with you.
> 
> First, the easy stuff:
> 
> 1) "git am" complains about whitespace violations

I'll take a look at that, there wasn't any when I created it...

> 2) HH:MM:SS times need zero padding; otherwise you see:
> 
> Tue, 8 Mar 2011 20:29:9 -0700

That's a bit more exciting, the specs I have say that shouldn't happen
(but then again this is Javascript so all bets are off).  Ok I see
what's wrong, the documentation isn't quite as clear - yeah may just
need to add a proper lpad() function to the common-lib.js, why that
isn't standard in Javascript...

> 3) Some of the Javascript functions are double-indented, others single-indented.

I'm not sure what you are seeing here, all of the javascript files I
have on my machine, "function" starts in column 0 of a new line, if you
are seeing anything other than that you've got something wrong somewhere
(mail client adding random spaces?)

for the record:

$ sha1sum *
56eefe528306bfa9d8e3aad9c6a379227e8d9c72  common-defs.js
c2379d9440fa471879e99317c0bfdc1607759927  common-lib.js
901c3705d0629b3836f4fab56694c4e2120cb036  cookies.js
cb220d94a5e762d2bd53af47040769e22ebf513b  date.js

If you aren't getting that, you have something wrong.

> 4) IE6 does not seem to like ISO 8601 format:
> 
> x = new Date("2011-03-09T03:29:09Z");
> 
> This sets all fields to NaN.  I suspect that getTime() values
> (milliseconds since 1970-01-01) are more portable.

That's actually an issue, my understanding (as well as the understanding
of all of the microformats that are out there) is that ISO 8601 is the
"correct" format that these things should be in.  If IE6 can't handle
that (and I'll admit I don't have trivial access to older IEs for
testing right now), then there is a *LOT* of Javascript out there that
is just broken.

Going a bit further, I would draw the line in the sand for gitweb
supporting IE as a browser somewhere around IE 7 or 8 at this point as well:

http://www.w3schools.com/browsers/browsers_explorer.asp

IE 5, 6 & 7 look to have negligible market share at best as it is, and
I'm not sure it's worth trying to go back and support IE6 or anything prior.

> I have attached a trivial patch for these four items; it applies on
> top of your original submission.

I'll dig into that tomorrow.

> Some other things that popped up:
> 
> 5) Some timezone offsets are not a whole number of hours.  Bangalore
> time is GMT +0530, for instance.

The code is capable of handling this, it's mainly an issue of interface,
which I'll bring up in response to 6 & 7

> 
> 6) Most U.S. timezones honor daylight savings, so they could be
> something like -0700 for part of the year, and -0800 for the rest of
> the year.  Picking the "local" option would automatically adjust for
> this, but DST limits the usefulness of permanently storing a fixed TZ
> offset in the cookie.

If you choose local, you will always get the local that the machine is
set at.  More on that in #7 answer.

> 
> 7) Looking at a pre-DST commit after DST (or vice versa) can be a
> little confusing:
> 
> Tue, 8 Mar 2011 20:29:09 -0700 + (19:29 -0800)
> 
> I'm not sure which time to believe.  (Although it's likely that a few
> weeks after a commit, the exact hour doesn't matter.)

Ok this is a bit of an answer to questions 5, 6 & 7 - the simple thing
is, time zones are unbelievably complicated, esoteric and just a
freaking nightmare.  In the US alone there are 4 time zones + DST + a
pile of exceptions that don't follow DST or follow DST in weird ways
(like it starts an hour later, or earlier, etc).

Trying to encompass all of those rules, which change on a frequently
enough basis that I'm not sure we want to spend all of our time fixing
and updating timezone issues, but this makes things:

	1) going to be impossible (see 3)
	2) going to require a *MUCH* more complicated user interface
	3) going to require that Javascript have a date library that
	   can actually handle timezones in some sane manor.

I would wager that the most used / useful setting for most people will
be to select 'local' and run with that (which is what the default is set
to) since that can take advantage of the system's inbuilt idea of what
the correct utc offset should be.

What I've provided now is the obvious and simple interface, and while it
ignores the more esoteric situations (and dst all together), the core of
the algorithm will happily take any offset you want and it will work.
Heck at one point in my testing I accidentally was using a 5 minute
offset, which was slightly amusing.

Adding in the 1/2hr offsets would not be hard, and it was something I
had thought about earlier today in fact.  Basically in the for loop that
creates the drop down add in some special cases to get the correct half
hour offsets, but at it's core you are still choosing the offset manually.

The only way you could get all of the timezones is to more or less mimic
the interface that every OS uses to select your time zones: provide a
clickable map, or a very long list of cities to choose from.

However even if we made the user interface significantly better (I'll
admit I kinda like it's simplicity and straightforwardness) we have the
problem that Javascript's inbuilt date class is woefully inadequate, and
we would either have to build our own, or go to a 3rd party and make
this feature dependent on outside javascript libraries.

I'm ok with the later, but I'm not keen on doing the former if we can
help it.  Why?  I'm not prepared to deal with the 1722 timezones that
exist, each with their own exciting rules.

> 8) The " + " popup menu is a little quirky.  On FF 3.6 it partially
> collapses after selecting a value from the dropdown.  On IE6 it shows
> "Error in parsing value for 'display'" and does not render.  On Opera
> 11 it seemed to work OK.
> 
> Firefox breakage: http://img217.imageshack.us/f/firefoxa.png/

I hadn't seen it that badly in Chrome (what I've been testing with), but
I'll admit the way I've implemented the onclick with that can be messy.
 I need to look around and find a few more ways to handle that and see
what I can come up with.

> I'm wondering if there might be a better place on the page to put the
> TZ selection.  It isn't immediately obvious to the user what the extra
> " + " does, and it seems to cause some issues.

I haven't really come up with a better place, there's no settings
dialogue for gitweb, and I wold agree that the + isn't really indicative
(just seemed like a good way to get it in there for now).  I had thought
of using a graphical icon, maybe like the (i) for information perhaps,
but yeah, not sure how to handle that currently.

> If you decide to keep it where it is, you might want to consider
> absolute or fixed positioning so that other elements do not wrap
> around it.  IOW it would work more like the dropdown menus on many
> sites.

That actually gives me an idea on how to handle that better, well at
least one possible way.  I'm open to suggestions on how to improve that
interface, I'm a backend server person for the most part, not a UI
designer or HCI expert.

I'm happy to mock things up or try things out, but I don't have any
better suggestions for how to handle that :-/

> The timezone fixup javascript seemed to work reasonably well, except
> for the hiccup with IE6.  Maybe it would be worth splitting this into
> two patches: one to rewrite the timestamps, and a second one to add
> the TZ selection interface.

Dunno, this is a pretty small and self contained set of patches as it is
and separating the selection interface only really removes two functions
from the first commit.  I'll look into it though, see if there's a
reasonably logical way to break this up into smaller pieces.

- John

Re: [PATCH 1/1] gitweb: javascript ability to adjust time based on timezone

[Jakub Narebski]

On Thu, 24 Mar 2011, J.H. wrote:
> On 03/23/2011 10:23 PM, Kevin Cernekee wrote:
> > On Wed, Mar 23, 2011 at 5:08 PM, John 'Warthog9' Hawley
> > <warthog9@eaglescrag.net> wrote:

> > > This patch takes the same basic goal, display the appropriate times
> > > in a given timezone, and implements it in Javascript.  This requires
> > > adding / using a new class, dtcommit, which is based on the
> > > dtstart/dtend microformats.  Appropriate commit dates are wrapped in
> > > a span with this class, and a title of the time in ISO8601 format.

[...]
> > 4) IE6 does not seem to like ISO 8601 format:
> > 
> > x = new Date("2011-03-09T03:29:09Z");
> > 
> > This sets all fields to NaN.  I suspect that getTime() values
> > (milliseconds since 1970-01-01) are more portable.
> 
> That's actually an issue, my understanding (as well as the understanding
> of all of the microformats that are out there) is that ISO 8601 is the
> "correct" format that these things should be in.  If IE6 can't handle
> that (and I'll admit I don't have trivial access to older IEs for
> testing right now), then there is a *LOT* of Javascript out there that
> is just broken.
> 
> Going a bit further, I would draw the line in the sand for gitweb
> supporting IE as a browser somewhere around IE 7 or 8 at this point as well:
> 
> http://www.w3schools.com/browsers/browsers_explorer.asp
> 
> IE 5, 6 & 7 look to have negligible market share at best as it is, and
> I'm not sure it's worth trying to go back and support IE6 or anything prior.

Another solution would be check if browser supports required JavaScript
features, and if not behave as if JavaScript was disabled (graceful
degradation).

[...]
> > Some other things that popped up:
> > 
> > 5) Some timezone offsets are not a whole number of hours.  Bangalore
> > time is GMT +0530, for instance.
> 
> The code is capable of handling this, it's mainly an issue of interface,
> which I'll bring up in response to 6 & 7

This is not issue of only interface: date.js has 'tzOffset / 100 * msPerHr'
and this do not support fractional offsets.  +0530 is 5.5 hours, not 5.3.

-- 
Jakub Narebski
Poland

Re: [PATCH 1/1] gitweb: javascript ability to adjust time based on timezone

[Jakub Narebski]

On Thu, 24 Mar 2011, Kevin Cernekee wrote:
> On Wed, Mar 23, 2011 at 5:08 PM, John 'Warthog9' Hawley
> <warthog9@eaglescrag.net> wrote:

> > This patch takes the same basic goal, display the appropriate times
> > in a given timezone, and implements it in Javascript.  This requires
> > adding / using a new class, dtcommit, which is based on the
> > dtstart/dtend microformats.  Appropriate commit dates are wrapped in
> > a span with this class, and a title of the time in ISO8601 format.
> 
> John,
> 
> Thanks for coding this up.  I tested it on a couple of different
> browsers and wanted to share my observations with you.

I wonder if there is any site that allows to check JavaScript for
compatibility with different browsers...

> 
> First, the easy stuff:
> 
> 1) "git am" complains about whitespace violations

It would be more helpful if you wrote here what are those whitespace
violations.

> 
> 2) HH:MM:SS times need zero padding; otherwise you see:
> 
> Tue, 8 Mar 2011 20:29:9 -0700

There is even padLeft function in gitweb.js ready to be (re)used...

[...] 
> 4) IE6 does not seem to like ISO 8601 format:
> 
> x = new Date("2011-03-09T03:29:09Z");
> 
> This sets all fields to NaN.  I suspect that getTime() values
> (milliseconds since 1970-01-01) are more portable.

Do you mean using epoch in title attribute, or fallback to parsing
ISO 8601 UTC format with regexps?

> I have attached a trivial patch for these four items; it applies on
> top of your original submission.

It would be much, much easier to review your patch if you either
put it inline at the end of your email, separating it from the rest
of email with e.g. "-- >8 --" separator (so called 'scissors'),
or at least using 'text/plain' as mimetype, '8bit' and not 'base64'
encoding, and perhaps 'disposition=inline' rather than 
'disposition=attachement'.

[...]
> 6) Most U.S. timezones honor daylight savings, so they could be
> something like -0700 for part of the year, and -0800 for the rest of
> the year.  Picking the "local" option would automatically adjust for
> this, but DST limits the usefulness of permanently storing a fixed TZ
> offset in the cookie.

Dealing with DST (zoneinfo library) is simply too hard for JavaScript 
IMHO.  What we could do is to store "local" in cookie, not a fixed TZ
offset (or perhaps store both as to not recalculate it).


> 8) The " + " popup menu is a little quirky.  On FF 3.6 it partially
> collapses after selecting a value from the dropdown.  On IE6 it shows
> "Error in parsing value for 'display'" and does not render.  On Opera
> 11 it seemed to work OK.
> 
> Firefox breakage: http://img217.imageshack.us/f/firefoxa.png/
> 
> I'm wondering if there might be a better place on the page to put the
> TZ selection.  It isn't immediately obvious to the user what the extra
> " + " does, and it seems to cause some issues.

Hmmm... perhaps a 'config' page?

-- 
Jakub Narebski
Poland

Re: [PATCH 1/1] gitweb: javascript ability to adjust time based on timezone

[Kevin Cernekee]

2011/3/24 Jakub Narebski <jnareb@gmail.com>:
>> 4) IE6 does not seem to like ISO 8601 format:
>>
>> x = new Date("2011-03-09T03:29:09Z");
>>
>> This sets all fields to NaN.  I suspect that getTime() values
>> (milliseconds since 1970-01-01) are more portable.
>
> Do you mean using epoch in title attribute, or fallback to parsing
> ISO 8601 UTC format with regexps?

getTime() format is $epoch * 1000.  When I switched to that format,
all 3 of my browsers were able to handle it.

I really don't think relying on "new Date(iso8601_timestamp)" is a
good idea, but I guess the string parsing approach would work:

http://webcloud.se/log/JavaScript-and-ISO-8601/

> Dealing with DST (zoneinfo library) is simply too hard for JavaScript
> IMHO.  What we could do is to store "local" in cookie, not a fixed TZ
> offset (or perhaps store both as to not recalculate it).

I agree, and I do not have a comprehensive solution for handling
non-local timezones.

> Hmmm... perhaps a 'config' page?

Any thoughts on what other user-configurable items might get added in
the future, and whether they will be on the client side or server
side?

Re: [PATCH 1/1] gitweb: javascript ability to adjust time based on timezone

[J.H.]

On 03/24/2011 03:00 PM, Kevin Cernekee wrote:
> 2011/3/24 Jakub Narebski <jnareb@gmail.com>:
>>> 4) IE6 does not seem to like ISO 8601 format:
>>>
>>> x = new Date("2011-03-09T03:29:09Z");
>>>
>>> This sets all fields to NaN.  I suspect that getTime() values
>>> (milliseconds since 1970-01-01) are more portable.
>>
>> Do you mean using epoch in title attribute, or fallback to parsing
>> ISO 8601 UTC format with regexps?
> 
> getTime() format is $epoch * 1000.  When I switched to that format,
> all 3 of my browsers were able to handle it.
> 
> I really don't think relying on "new Date(iso8601_timestamp)" is a
> good idea, but I guess the string parsing approach would work:
> 
> http://webcloud.se/log/JavaScript-and-ISO-8601/

Looking at that, MS provides VM images for compatability testing with
older IEs so I'm at least able to see the problem directly.  There's
also badness in using epoch directly, screen readers will handle that
particularly badly (which is one of the reasons the microformats
generally shifted to a slightly, if more painful to read from
Javascript, method)

>> Dealing with DST (zoneinfo library) is simply too hard for JavaScript
>> IMHO.  What we could do is to store "local" in cookie, not a fixed TZ
>> offset (or perhaps store both as to not recalculate it).
> 
> I agree, and I do not have a comprehensive solution for handling
> non-local timezones.
> 
>> Hmmm... perhaps a 'config' page?
> 
> Any thoughts on what other user-configurable items might get added in
> the future, and whether they will be on the client side or server
> side?

Any configurations that would happen there will all be client side, for
a lot of reasons gitweb will not be able to have anything user level
configurable from the server side (well at least any of the larger
gitweb installs, which is what I'm primarily focused on).  That said
there's a lot we could do with Javascript from the client side, and that
was one way of handling to change I had thought of - just didn't seem
useful since we currently would only have this.

I'm fine with the idea however (I envisioned it as some larger drop down
that more or less filled the whole page and set various cookies for it's
storage of things).  But moving in this direction should be a conscious
and definitive move that these kinds of changes will be client side, and
will involve more and more Javascript.  Assuming that our fallback
non-javascript version of the page still works fine, and is usable, this
is a good plan overall but it is a shift from where we have been with
respect to Gitweb.

- John 'Warthog9' Hawley

Re: [PATCH 1/1] gitweb: javascript ability to adjust time based on timezone

[J.H.]

On 03/24/2011 01:19 PM, Jakub Narebski wrote:
> On Thu, 24 Mar 2011, Kevin Cernekee wrote:
>> On Wed, Mar 23, 2011 at 5:08 PM, John 'Warthog9' Hawley
>> <warthog9@eaglescrag.net> wrote:
> 
>>> This patch takes the same basic goal, display the appropriate times
>>> in a given timezone, and implements it in Javascript.  This requires
>>> adding / using a new class, dtcommit, which is based on the
>>> dtstart/dtend microformats.  Appropriate commit dates are wrapped in
>>> a span with this class, and a title of the time in ISO8601 format.
>>
>> John,
>>
>> Thanks for coding this up.  I tested it on a couple of different
>> browsers and wanted to share my observations with you.
> 
> I wonder if there is any site that allows to check JavaScript for
> compatibility with different browsers...

Not that I know of, would be useful but probably horrifically
complicated.  When I was doing Javascript and web stuff as my day job,
about half of what you would write were wrappers to work around the
brokenness on various browsers (mostly IE - ohhh IE...)

>> First, the easy stuff:
>>
>> 1) "git am" complains about whitespace violations
> 
> It would be more helpful if you wrote here what are those whitespace
> violations.
> 
>>
>> 2) HH:MM:SS times need zero padding; otherwise you see:
>>
>> Tue, 8 Mar 2011 20:29:9 -0700
> 
> There is even padLeft function in gitweb.js ready to be (re)used...

I'd rather use one that exists than recreate it, probably worth moving
that to common-lib.js, but that is a separate commit.

Sorry about that, didn't notice it in testing (it took me a while to
find one of my local repos with a commit i nthe first 10 minutes or so),
and the documentation I was reading implied that should have been padded.

> [...] 
>> 4) IE6 does not seem to like ISO 8601 format:
>>
>> x = new Date("2011-03-09T03:29:09Z");
>>
>> This sets all fields to NaN.  I suspect that getTime() values
>> (milliseconds since 1970-01-01) are more portable.
> 
> Do you mean using epoch in title attribute, or fallback to parsing
> ISO 8601 UTC format with regexps?

Parsing it with regexps is doable if completely inelegant, that said
this is Javascript...

Looky that MS provides Virtual PC images for compat testing for IE6
http://www.microsoft.com/downloads/en/details.aspx?FamilyID=21eabb90-958f-4b64-b5f1-73d0a413c8ef&displaylang=en

Ok reading through the documentation I can find for MS and their
Date.parse (
http://msdn.microsoft.com/en-us/library/dctx55bc(v=VS7.1).aspx ) and
some quick experimentation:

2011-03-09 - breaks
2011/03/09 - works

2011/03/09T01:01:01  - works, sorta - the hour comes out as 10 vs. 01
2011/03/09 01:01:01  - works, hour is correct
2011/03/09 01:01:01Z - works and seems to get the TZ correct
2011/03/09T01:01:01Z - breaks

Not really sure what the "right" way to fix this is going to end up
being.  Suppose our options are:

1) Try and find a format that is generally accepted and parseable on all
the browsers
2) Declare IE6 an unsupported browser
3) Trap what browser we are on and do a regex parsing of the string and
do appropriate sets or a more verbose format that works everywhere
4) Use a regex and sets for every browser.
5) Switch all the embedded times over to epochs in the title=""'s

4 is probably the "most" right that I can see of those options, and
avoids possible other browser inconsistencies with respect to date
parsing.  I'm going to code that up (it shouldn't be terribly
complicated), unless someone likes one of the other ideas better.
Suppose we could even go down the more complicate microformat route and
just write some code to parse that, just more complicated all the way
around.

5 has the appeal that everything handles the epoch correctly, but it
does have the downside of being less readable in the html code (and
shifts us slightly further away from the accepted "standard" of the
microcode formats)

[...]
>> 6) Most U.S. timezones honor daylight savings, so they could be
>> something like -0700 for part of the year, and -0800 for the rest of
>> the year.  Picking the "local" option would automatically adjust for
>> this, but DST limits the usefulness of permanently storing a fixed TZ
>> offset in the cookie.
> 
> Dealing with DST (zoneinfo library) is simply too hard for JavaScript 
> IMHO.  What we could do is to store "local" in cookie, not a fixed TZ
> offset (or perhaps store both as to not recalculate it).

I already provide the option for "local" in the cookie, the other fixed
timesets are more for people wanting to be more explicit or look at time
zones from a different location.

>> 8) The " + " popup menu is a little quirky.  On FF 3.6 it partially
>> collapses after selecting a value from the dropdown.  On IE6 it shows
>> "Error in parsing value for 'display'" and does not render.  On Opera
>> 11 it seemed to work OK.
>>
>> Firefox breakage: http://img217.imageshack.us/f/firefoxa.png/
>>
>> I'm wondering if there might be a better place on the page to put the
>> TZ selection.  It isn't immediately obvious to the user what the extra
>> " + " does, and it seems to cause some issues.
> 
> Hmmm... perhaps a 'config' page?

Already made my comments on that in the other e-mail.

- John 'Warthog9' Hawley

Re: [PATCH 1/1] gitweb: javascript ability to adjust time based on timezone

[Jakub Narebski]

On Fri, 25 Mar 2011, J.H. wrote:
> On 03/24/2011 01:19 PM, Jakub Narebski wrote:
>> On Thu, 24 Mar 2011, Kevin Cernekee wrote:

>>> 4) IE6 does not seem to like ISO 8601 format:
>>>
>>> x = new Date("2011-03-09T03:29:09Z");
>>>
>>> This sets all fields to NaN.  I suspect that getTime() values
>>> (milliseconds since 1970-01-01) are more portable.
>> 
>> Do you mean using epoch in title attribute, or fallback to parsing
>> ISO 8601 UTC format with regexps?
> 
> Parsing it with regexps is doable if completely inelegant, that said
> this is Javascript...
> 
> Looky that MS provides Virtual PC images for compat testing for IE6
> http://www.microsoft.com/downloads/en/details.aspx?FamilyID=21eabb90-958f-4b64-b5f1-73d0a413c8ef&displaylang=en
> 
> Ok reading through the documentation I can find for MS and their
> Date.parse (
> http://msdn.microsoft.com/en-us/library/dctx55bc(v=VS7.1).aspx ) and
> some quick experimentation:
> 
> 2011-03-09 - breaks
> 2011/03/09 - works

I really don't like using '/' as a separator; not only this is not
ISO-8601, but it is easy to confuse with insane American way of
writing dates with MM/DD/YYYY (day in the middle).
 
> 2011/03/09T01:01:01  - works, sorta - the hour comes out as 10 vs. 01
> 2011/03/09 01:01:01  - works, hour is correct
> 2011/03/09 01:01:01Z - works and seems to get the TZ correct
> 2011/03/09T01:01:01Z - breaks

If it worked in all (or almost all) web browsers with

  2011-03-09 01:01:01Z - works and seems to get the TZ correct

then I would say go for it - this variant of ISO-8601 with ' ' instead
of 'T' to separate date part from time part is more human-readable.
 
> Not really sure what the "right" way to fix this is going to end up
> being.  Suppose our options are:
> 
> 1) Try and find a format that is generally accepted and parseable on all
> the browsers
> 2) Declare IE6 an unsupported browser
> 3) Trap what browser we are on and do a regex parsing of the string and
> do appropriate sets or a more verbose format that works everywhere

If web browser returned undefined value if they can't parse date, then
we could do regexp parsing based on this, not on user-string (which is
in most cases wrong solution).

> 4) Use a regex and sets for every browser.
> 5) Switch all the embedded times over to epochs in the title=""'s
> 
> 4 is probably the "most" right that I can see of those options, and
> avoids possible other browser inconsistencies with respect to date
> parsing.  I'm going to code that up (it shouldn't be terribly
> complicated), unless someone likes one of the other ideas better.
> Suppose we could even go down the more complicate microformat route and
> just write some code to parse that, just more complicated all the way
> around.

Right.

> 5 has the appeal that everything handles the epoch correctly, but it
> does have the downside of being less readable in the html code (and
> shifts us slightly further away from the accepted "standard" of the
> microcode formats)

Well, we could always use non-standard 'epoch' attribute, or something...
 
-- 
Jakub Narebski
Poland

Re: [PATCH 1/1] gitweb: javascript ability to adjust time based on timezone

[Jakub Narebski]

On Thu, 24 Mar 2011, John 'Warthog9' Hawley wrote:

> This patch is based on Kevin Cernekee's <cernekee@gmail.com>
> patch series entitled "gitweb: introduce localtime feature".  While
> Kevin's patch changed the server side output so that the timezone
> was output from gitweb itself, this has a number of drawbacks, in
> particular with respect to gitweb-caching.

Excuse me, but the above is not true, Kevin Cernekee's patches, 
_as they are implemented_, would not interfere with [future] gitweb
caching.  What might and probably would interfere with output caching
is hypothetical vaguely proposed series that would implement on server
choosing of timezone on client stored in a cookie.  But Kevin's patches
do not do this.

Any server-side implemented feature that depends only on server-side
configuration, and always create exactly the same view for the same
URL (assuming that repository didn't change), wouldn't interfere with
gitweb-caching.


I agree that implementing what this patch does on server-side would
make gitweb-caching implementation harder, and reduce gains from
caching output.
 
> This patch takes the same basic goal, display the appropriate times
> in a given timezone, and implements it in Javascript.  This requires
> adding / using a new class, dtcommit, which is based on the
> dtstart/dtend microformats.  Appropriate commit dates are wrapped in
> a span with this class, and a title of the time in ISO8601 format.

Is it ISO8601 format with timezone, or in UTC?

Note that currently this pattern is discouraged for accesibility reasons;
http://microformats.org/wiki/datetime-design-pattern says:

  Note: Some accessibility-issues have been raised with Datetime Design
  Pattern, that have been addressed with the completion of the 
  value-class-pattern.

But using value-class-pattern would make it harder for JavaScript to
handle, and at least in short form does not provide us with original
timezone.

> 
> Enabling this will then add several javascript files to be loaded,

While I agree that for maintenance reasons JavaScript files should be
split, most web best practice pages[1][2][3] recommends to combine
JavaScript files for performance.  

[1]: http://developer.yahoo.com/performance/rules.html
     "Minimize HTTP Requests" section
[2]: http://code.google.com/speed/articles/include-scripts-properly.html
     "1. Combine external JavaScript files"
[3]: http://javascript-reference.info/speed-up-your-javascript-load-time.htm
     "Combine Your Files" section.

See also comments below.

But as an RFC / weatherballoon patch it is all right.

> a default timezone to be selected and a single onloadTZSetup()
> function to be called.

What about 'window.onload = fixLinks;' that gitweb used to use to be
able to detect by server-side code if JavaScript-only views (such as
'blame_incremental') should/could be used in place of non-JavaScript
ones?  Ah, I see that it is solved in different way: instead of
window.onload, embedded JavaScript snippet is used.

> 
> This will place a "+" to the right of an appropriate time and clicking
> on it will give you a drop down to choose the timezone to change to.

User interface part would probably be bikeshedded to death... ;-P

> 
> All changes are saved to a Javascript cookie, so page changes and
> browser closure / reopening retains the last known setting used.

Good... assuming that we respect user's choice to not allow cookies.

> 
> Fallback (should Javascript not be enabled) is to treat dates as
> they have been and display them, only, in UTC.

That is sane solution.

> 
> Valid timezones, currently, are:
> 
> utc
> local
> -1200
> -1100
> ...
> +1100
> +1200

I guess that it would be difficult to support 'original' / 'asis' timezone,
i.e. the timezone that author / committer / tagger used, and which is saved
in git object... and it wouldn't be more useful than 'local', I guess.

> 
> With each timezone being +1hr to the previous.  The code is capable of
> handling fractional timezones, but those have not been added.

NOTE: I think that current gitweb code (parse_date / format_date) doesn't
handle negative fractional timezones correctly.  

	$tz =~ m/^([+\-][0-9][0-9])([0-9][0-9])$/;
	my $local = $epoch + ((int $1 + ($2/60)) * 3600);


Though git.git repository doesn't contain negative fractional timezones...
it does contain positive fractional timezones however.

  $ git log --pretty='%ai%n%ci' --all | cut -d' ' -f 3 | sort -n | uniq

Could anyone check it e.g. on Linux kernel repository?

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

I guess other pages (like e.g. feed pages, i.e. 'rss' and 'atom' views)
are not affected?

> 
> Based-on-code-from: Kevin Cernekee <cernekee@gmail.com>

Is it Based-on-code-from, or Idea-by / Inspired-by?  ;-)

> Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
> ---
>  gitweb/gitweb.perl              |   64 +++++++++++++++-
>  gitweb/static/js/common-defs.js |   12 +++
>  gitweb/static/js/common-lib.js  |   32 ++++++++
>  gitweb/static/js/cookies.js     |   35 +++++++++
>  gitweb/static/js/date.js        |  160 +++++++++++++++++++++++++++++++++++++++
>  5 files changed, 300 insertions(+), 3 deletions(-)
>  create mode 100644 gitweb/static/js/common-defs.js
>  create mode 100644 gitweb/static/js/common-lib.js
>  create mode 100644 gitweb/static/js/cookies.js
>  create mode 100644 gitweb/static/js/date.js

[...]
> @@ -3728,6 +3772,19 @@ sub git_footer_html {
>  		      qq!</script>\n!;
>  	}
>  
> +	print "<!-- jslocaltime: $jslocaltime -->";

Is the above line leftover debugging?

> +	if( $jslocaltime ne '' ){
> +		my $js_path = File::Basename::dirname($javascript);
> +		print qq!<script type="text/javascript" src="!. esc_url( $js_path ."/js/common-defs.js" )	.qq!"></script>\n!;
> +		print qq!<script type="text/javascript" src="!. esc_url( $js_path ."/js/common-lib.js" )	.qq!"></script>\n!;
> +		print qq!<script type="text/javascript" src="!. esc_url( $js_path ."/js/date.js" )		.qq!"></script>\n!;
> +		print qq!<script type="text/javascript" src="!. esc_url( $js_path ."/js/cookies.js" )		.qq!"></script>\n!;
> +		print "<script type=\"text/javascript\">\n".
> +				"var tzDefault = \"$jslocaltime\";\n".
> +				"onloadTZSetup();\n".
> +		      "</script>";
> +	}

First, it is recommended for better performance to consolidate JavaScript
files into single file (or load them dynamically).  gitweb/Makefile could
do this, though it would mean that installing gitweb by hand would be even
more difficult.  But this can be left for later commit.

Second, I guess that some of functions from current gitweb.js could be
moved to js/common-lib.js.  But that can be left for later commit.

Third, I wonder if instead of creating our own library, it wouldn't be
better if gitweb used (perhaps loaded from external source, like Google)
some JavaScript library, like e.g. jQuery.

> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 9dccfb0..2a662d8 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -261,6 +261,49 @@ our %highlight_ext = (
>  	map { $_ => 'xml' } qw(xhtml html htm),
>  );
>  
> +
> +# Enable / Disable the ability for gitweb to use a small amount of 
> +# javascript, along with a javascript cookie, to display commit
> +# times in the preffered timezone.  Time zones can be pre-set 
> +# by specifying it in $jslocaltime with one of the following:
> +#
> +#	utc
> +#	local

See above about lack of 'original' / 'asis' here.

> +#	-1200
> +#	-1100
> +#	-1000
> +#	-0900
> +#	-0800
> +#	-0700
> +#	-0600
> +#	-0500
> +#	-0400
> +#	-0300
> +#	-0200
> +#	-0100
> +#	+0000
> +#	+0100
> +#	+0200
> +#	+0300
> +#	+0400
> +#	+0500
> +#	+0600
> +#	+0700
> +#	+0800
> +#	+0900
> +#	+1000
> +#	+1100
> +#	+1200

Must we list all those timezones explicitely?

> +#
> +# utc is identical to +0000, and local will use the
> +# timezone that the browser thinks it is in.
> +#
> +# Enabled by default.
> +#
> +# To disable set to null or ''

I guess it is "To disable set to undef or ''", isn't it?  BTW. would
any false-ish value (undef, '', 0, 0.0, ()) work?

> +
> +our $jslocaltime = 'local';

Hmmm... I wonder if it wouldn't be better to use non-overridable %feature
for this, instead of introducing yet another [global] variable...


> @@ -504,6 +547,7 @@ our %feature = (
>  		'sub' => sub { feature_bool('remote_heads', @_) },
>  		'override' => 0,
>  		'default' => [0]},
> +
>  );
>  
>  sub gitweb_get_feature {

> +
>  	print "</body>\n" .
>  	      "</html>";
>  }

Stray change?

> -	      " [$ad{'rfc2822'}";
> +	      " [<span class=\"dtcommit\" title=\"$ad{'iso-8601'}\">$ad{'rfc2822'}</span>";
[...]
> -		      "<td></td><td> $wd{'rfc2822'}";
> +		      "<td></td><td> <span class=\"dtcommit\" title=\"$wd{'iso-8601'}\">$wd{'rfc2822'}</span>";
[...]
> -		print "<tr id=\"metadata_lchange\"><td>last change</td><td>$cd{'rfc2822'}</td></tr>\n";
> +		print "<tr id=\"metadata_lchange\"><td>last change</td>\n";
> +		print "<td><span class=\"dtcommit\" title=\"$cd{'iso-8601'}\">$cd{'rfc2822'}</span></td></tr>\n";

This code repetition just begs for a helper subroutine, like e.g.

  -	      " [$ad{'rfc2822'}";
  +	      " [" . format_timestamp($ad, 'rfc2822');

or

  +	      " [" . format_timestamp($ad, -format=>'rfc2822');

> diff --git a/gitweb/static/js/common-defs.js b/gitweb/static/js/common-defs.js
> new file mode 100644
> index 0000000..23a1cc2
> --- /dev/null
> +++ b/gitweb/static/js/common-defs.js
> @@ -0,0 +1,12 @@
> +// Copyright (C) 2011, John 'Warthog9' Hawley <warthog9@eaglescrag.net>
> +//
> +//
> +// JavaScript common globals for gitweb (git web interface).
> +// @license GPLv2 or later
> +//
> +
> +// Common universal values get set here
> +// Things like cookie names, and other just
> +// basic global variables.
> +
> +getwebCookieTZOffset = "tzOffset";
   ^^^^^^
        \- typo, probably should be "gitweb" here

Is it really worth adding extra JavaScript file?

> diff --git a/gitweb/static/js/common-lib.js b/gitweb/static/js/common-lib.js
> new file mode 100644
> index 0000000..2ce65ec
> --- /dev/null
> +++ b/gitweb/static/js/common-lib.js
> @@ -0,0 +1,32 @@
> +// Copyright (C) 2011, John 'Warthog9' Hawley <warthog9@eaglescrag.net>
> +//
> +//
> +// JavaScript date handling code for gitweb (git web interface).
> +// @license GPLv2 or later
> +//
> +//
> +function findElementsByClassName( className ) {
> +	if( document.getElementsByClassName == undefined) {
> +		var hasClassName = new RegExp("(?:^|\\s)" + className + "(?:$|\\s)");
> +		var allElements = document.getElementsByTagName("*");
> +		var foundElements = [];
> +
> +		var element = null;
> +		for (var x = 0; (element = allElements[x]) != null; x++) {
> +			var curClass = element.className;
> +			if(
> +				curClass				// If we've actually got something
> +				&&
> +				curClass.indexOf(className) != -1	// And it has a valid index, I.E. not -1
> +				&&
> +				hasClassName.test(curClass)		// and the regex passes
> +			) {
> +				foundElements.push(element);		// push it onto the results stack
> +			}
> +		}
> +
> +		return foundElements;
> +	}else{
> +		return document.getElementsByClassName( className );
> +	}
> +}

Hmmm... this really begs for some JavaScript library, like e.g. jQuery.
Is there any sane web browser that doesn't support getElementsByTagName?

BTW. is it your own code?

> diff --git a/gitweb/static/js/cookies.js b/gitweb/static/js/cookies.js
> new file mode 100644
> index 0000000..8c8f7aa
> --- /dev/null
> +++ b/gitweb/static/js/cookies.js
> @@ -0,0 +1,35 @@
> +// Copyright (C) 2011, John 'Warthog9' Hawley <warthog9@eaglescrag.net>
> +//
> +//
> +// JavaScript cookie handling code for gitweb (git web interface).
> +// @license GPLv2 or later
> +//
> +
> +function setCookieExp( name, value, expires ){
> +	var expDate = new Date( expires.toString() );
> +	expires = expDate.toUTCString;
> +	document.cookie = escape(name) +"="+ escape(value) +";"+ expDate.toUTCString() +";path=/";

Shouldn't it be

  +	document.cookie = escape(name) +"="+ escape(value) +";expires="+ expDate.toUTCString() +";path=/";

> +}
> +
> +function setCookie( name, value ){
> +	var txtCookie = name +"=\""+ value +"\";path=/";
> +	document.cookie = txtCookie;
> +}

Hmmm... why setCookie doesn't escape name and value, but surrounds value
with doublequotes '"', contrary to what setCookieExp does?

Should we set 'path' to anything in a cookie?

> +
> +function getCookie( name ){
> +	var allCookies = document.cookie.split(";");
> +	var value = "";
> +
> +	for( var x = 0; x < allCookies.length; x++ ){
> +		var brokenCookie = allCookies[x].split("=",2);
> +		var hasName = new RegExp("^\\s*" + name + "\\s*$");

Note: I am not sure if using '/regex/' instead of 'new RegExp("regex")'
isn't preferred way of dealing with regular expressions in JavaScript.

> +		if(
> +			hasName.test(brokenCookie[0])	// Check for the name of the cookie based on the regex
> +			&&
> +			brokenCookie.length == 2	// Just making sure there is something to actually return here
> +		){
> +			return unescape(brokenCookie[1]);
> +		}
> +	}
> +	return null;
> +}

Please do not use RegExp where string comparison would be enough.  Also,
escape cookie name first (or unescape what you got from browser):

  +function getCookie(name)
  +{
  +	var allCookies = document.cookie.split("; ");
  +
  +	for (var x = 0; x < allCookies.length; x++ ) {
  +		var oneCookie = allCookies[x].split("=", 2);
  +		if (oneCookie[0] == escape(name)) {
  +			return unescape(oneCookie[1]);
  +		}
  +	}
  +	return null;
  +}


> diff --git a/gitweb/static/js/date.js b/gitweb/static/js/date.js
> new file mode 100644
> index 0000000..a6d6f81
> --- /dev/null
> +++ b/gitweb/static/js/date.js
> @@ -0,0 +1,160 @@
> +// Copyright (C) 2011, John 'Warthog9' Hawley <warthog9@eaglescrag.net>
> +//
> +//
> +// JavaScript date handling code for gitweb (git web interface).
> +// @license GPLv2 or later
> +//
> +
> +function onloadTZSetup(){
> +	addChangeTZ();
> +	tzChangeNS( tzDefault );
> +	checkTZCookie();
> +}

Shouldn't we check cookie first, so that we can pre-select current
choice of timezone?

> +
> +function addChangeTZ() {

A bit of bikeshedding: perhaps there is a better name for adding
UI to change timezone used in all timestamps than addChangeTZ?

> +		var txtClassesFound = "";

Leftover debugging aid?

> +		var classesFound = findElementsByClassName( "dtcommit" );
> +		txtClassesFound += "Length: "+ classesFound.length +"<br>\n";
> +		for ( x = 0; x < classesFound.length; x++){
> +			curElement = classesFound[x];
> +			txtClassesFound += "<br>\n"+ x +" - "+ curElement.nodeName  +" - "+ curElement.title +" - "+ curElement.innerHTML +"<br>\n";
> +			var strExtra = " <span onclick=\"clickDate(event.target);\" title=\"+\">+</span>"

Errr... shouldn't 'title' attribute explain what this "+" is about instead?

> +			curElement.innerHTML = curElement.innerHTML + strExtra;
> +		}
> +}

Note for the future: instead of using 'onclick' handler for each event,
perhaps a better solution, though requiring more JavaScript knowledge,
is to make use of event bubbling?

But I think, user interface considerations aside, that is a good enough
solution for this/first commit.

> +
> +function checkTZCookie(){
> +	var preSetTZ = getCookie( getwebCookieTZOffset );

"preSetTZ", not "presetTZ"?  "getwebCookieTZOffset", not 
"gitwebCookieTZOffset"?

> +	if(
> +		preSetTZ != null
> +		&&
> +		preSetTZ.length != 0
> +	){
> +		tzChange( preSetTZ );
> +	}
> +}
> +
> +function formatTZ( tzOffset ) {

What does this function _do_; what are the arguments and what it returns?
There is no comment describing this function.  

I guess that it converts timezone offset as number to string.

> +		var posNeg = "+";
> +		if( tzOffset < 0 ){
> +			posNeg = "-";
> +		}

"posNeg" and not e.g. "sign"?

> +		tzOffset = Math.sqrt( Math.pow( tzOffset, 2 ) );

Uhhh?  Using something like this for abs?  WTF?

> +		if( tzOffset < 100 ){
> +			tzOffset = tzOffset * 100;
> +		}
> +		for( y = tzOffset.toString().length + 1; y <= 4; y++ ){
> +			tzOffset = "0"+ tzOffset;
> +		}

Why not use padLeft from gitweb.js here (perhaps moved to js/common-lib.js),
i.e. 

  +		tzOffset = padLeft(tzOffset, 4, '0');

> +		return posNeg + tzOffset;
> +}


> +
> +function dateOutput( objDate ) {
> +	return dateOutputTZ( objDate, "0" );
> +}

Do we really need this offset function?

> +
> +function dateOutputTZ( objDate, tzOffset ) {
> +	var strDate = "";
> +	var daysOfWeek = [ "Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat" ];
> +	var monthsOfYr = [ "Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec" ];
> +
> +	if( tzOffset == "utc" ){
> +		tzOffset = 0;
> +	}else if( tzOffset == "local" ){
> +		var tempDate = new Date();
> +		tzOffset = tempDate.getTimezoneOffset() * -1 / 60 * 100; 
> +		tzOffset = formatTZ( tzOffset );

I think it might be good idea to keep return value of getTimezoneOffset,
to not recalculate it again and again.  But it might be not good because
those two values (string timezone and offset in minutes) must be kept
in sync...

> +	}

We probably should extract getting local timezone to separate function,
and save its result somewhere so that we don't have to do this calculation
for each date, only once per invocation.

> +
> +	var msPerHr = 1000 * 60 * 60;	// 1000ms/sec * 60sec/min * 60sec/hr
> +	var toDateTime = new Date();

Perhaps "offsetDate" or "dateWithOffset" would be a better name for
this variable?

> +	toDateTime.setTime( objDate.getTime() + ( tzOffset / 100 * msPerHr ) );

This doesn't deal with fractional timezones, contrary what you said
in the commit message, isn't it?

> +
> +	// Current Date Formatting:
> +	// Fri, 19 Dec 2008 11:35:33 +0000
> +	strDate += daysOfWeek[ toDateTime.getUTCDay() ] +", ";
> +	strDate += toDateTime.getUTCDate() +" ";
> +	strDate += monthsOfYr[ toDateTime.getUTCMonth() ] +" ";
> +	strDate += toDateTime.getUTCFullYear() +" ";
> +	strDate += toDateTime.getUTCHours() +":";
> +	strDate += toDateTime.getUTCMinutes() +":";
> +	strDate += toDateTime.getUTCSeconds() +" ";

Don't you need to '0'-pad hours, minutes and seconds?

> +
> +	strDate += tzOffset;

Note for the future: composing a string by repeated concatenation is not
the best solution - it is faster to create array and join it to single
string at the end.  But this could be left for late commit.

Also I'd prefer if formatting of RFC2822 date was put in a separate
function.

> +
> +	return strDate;
> +}
> +
> +function tzChange( tzOffset ){
> +	return tzChangeSNS( tzOffset, true );
> +}
> +
> +function tzChangeNS( tzOffset ){
> +	return tzChangeSNS( tzOffset, false );
> +}

What are those wrappers needed for?

> +
> +function tzChangeSNS( tzOffset, set ){
> +	var txtClassesFound = "";
> +	var classesFound = findElementsByClassName( "dtcommit" );
> +	for ( x = 0; x < classesFound.length; x++){
> +		curElement = classesFound[x];
> +		var origDateTime = new Date( curElement.title );

I wonder if using 'new Date( Date.parse(curElement.title) )' would make
it more portable...

...or perhaps use regexp to parse date as fallback.

> +		curElement.innerHTML = dateOutputTZ(origDateTime, tzOffset);
> +	}
> +	var tzExpDate = new Date();
> +	tzExpDate.setDate( tzExpDate.getDate() + 180 );

Why magic 180?

> +	if( set == true ){
> +		setCookieExp( getwebCookieTZOffset, tzOffset, tzExpDate.toUTCString() );
> +	}
> +	addChangeTZ();

Do we need, and should we set expiration date for this cookie?

> +}
> +
> +function clickDate( clkEvent ) {
> +	if( clkEvent.title == "+" ){
> +		clkEvent.title="-";

Nice trick to allow expansion and contracting...

> +
> +		var preSetTZ = getCookie( getwebCookieTZOffset );
> +		
> +		var arrSelected = new Array();

I think the preferred way is to use

  +		var arrSelected = [];

> +		var offsetArr = 14;
> +		arrSelected[0] = " ";
> +		arrSelected[1] = " ";
> +		if( preSetTZ == "utc" ) {
> +			arrSelected[0] = " selected=\"selected\" ";
> +		} else if( preSetTZ == "local" ){
> +			arrSelected[1] = " selected=\"selected\" ";
> +		}
> +		for( x = -12; x <= 12; x++){
> +			arrSelected[x + offsetArr] = "";
> +			if( ( x * 100 ) == preSetTZ ){
> +				arrSelected[x + offsetArr] = " selected=\"selected\" ";
> +			}
> +		}
> +		var txtTzSelect = " \
> +<span style=\"width: 10%;background-color: grey;\">- \

Probably needs some absolute or relative positioning, to avoid reflow.

> +<table border=\"1\">\
> +	<tr>\
> +		<td>\
> +			Time Zone:\
> +		</td>\
> +		<td>\
> +			<select name=\"tzoffset\" onchange=\"tzChange(this.value);\">\
> +				<option "+ arrSelected[0] +" value=\"utc\">UTC/GMT</option>\
> +				<option "+ arrSelected[1] +" value=\"local\">Local (per browser)</option>";
> +		for( x = -12; x <= 12; x++){
> +			var tzOffset = formatTZ( x );
> +			txtTzSelect +="					<option "+ arrSelected[x + offsetArr] +"value=\""+ tzOffset +"\">"+ tzOffset +"</option>";
> +		}
> +		txtTzSelect += "\
> +			</select>\
> +		</td>\
> +	</tr>\
> +</table>\
> +</span>";
> +		clkEvent.innerHTML = txtTzSelect;
> +	}else{
> +		clkEvent.parentNode.title="+";
> +		clkEvent.parentNode.innerHTML = "+";
> +	}
> +}

All right, probably using innerHTML instead of constructing DOM is 
a better solution, or at least more portable.  Well, easier anyway.

-- 
Jakub Narebski
Poland

[PATCH (BUGFIX)] gitweb: Fix handling of fractional timezones in parse_date

[Jakub Narebski]

Fractional timezones, like -0330 (NST used in Canada) or +0430
(Afghanistan, Iran DST), were not handled properly in parse_date; this
means values such as 'minute_local' and 'iso-tz' were not generated
correctly.

This was caused by two mistakes:

* sign of timezone was applied only to hour part of offset, and not
  as it should be also to minutes part (this affected only negative
  fractional timezones).

* 'int $h + $m/60' is 'int($h + $m/60)' and not 'int($h) + $m/60',
  so fractional part was discarded altogether ($h is hours, $m is
  minutes, which is always less than 60).


Note that positive fractional timezones +0430, +0530 and +1030 can be
found as authortime in git.git repository itself.

For example http://repo.or.cz/w/git.git/commit/88d50e7 had authortime
of "Fri, 8 Jan 2010 18:48:07 +0000 (23:48 +0530)", which is not marked
with 'atnight', when "git show 88d50e7" gives correct author date of
"Sat Jan 9 00:18:07 2010 +0530".

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
On Thu, 24 Mar 2011, Jakub Narebski wrote:
> On Thu, 24 Mar 2011, John 'Warthog9' Hawley wrote:

> > With each timezone being +1hr to the previous.  The code is capable of
> > handling fractional timezones, but those have not been added.

No, it isn't (see my other email).
 
> NOTE: I think that current gitweb code (parse_date / format_date) doesn't
> handle negative fractional timezones correctly.  
> 
>         $tz =~ m/^([+\-][0-9][0-9])([0-9][0-9])$/;
>         my $local = $epoch + ((int $1 + ($2/60)) * 3600);

This patch fixed this bug (actually two bugs ;-)).

> Though git.git repository doesn't contain negative fractional timezones...
> it does contain positive fractional timezones however.
> 
>   $ git log --pretty='%ai%n%ci' --all | cut -d' ' -f 3 | sort -n | uniq
> 
> Could anyone check it e.g. on Linux kernel repository?

Anyone?

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 0178633..256062e 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2921,8 +2921,10 @@ sub parse_date {
 	$date{'iso-8601'}  = sprintf "%04d-%02d-%02dT%02d:%02d:%02dZ",
 	                     1900+$year, 1+$mon, $mday, $hour ,$min, $sec;
 
-	$tz =~ m/^([+\-][0-9][0-9])([0-9][0-9])$/;
-	my $local = $epoch + ((int $1 + ($2/60)) * 3600);
+	my ($tz_sign, $tz_hour, $tz_min) =
+		($tz =~ m/^([+\-])([0-9][0-9])([0-9][0-9])$/);
+	$tz_sign = ($tz_sign eq '-' ? -1 : +1);
+	my $local = $epoch + $tz_sign*($tz_hour + ($tz_min/60.0))*3600;
 	($sec, $min, $hour, $mday, $mon, $year, $wday, $yday) = gmtime($local);
 	$date{'hour_local'} = $hour;
 	$date{'minute_local'} = $min;
-- 
1.7.3

Re: [PATCH (BUGFIX)] gitweb: Fix handling of fractional timezones in parse_date

[Kevin Cernekee]

2011/3/25 Jakub Narebski <jnareb@gmail.com>:
> @@ -2921,8 +2921,10 @@ sub parse_date {
>        $date{'iso-8601'}  = sprintf "%04d-%02d-%02dT%02d:%02d:%02dZ",
>                             1900+$year, 1+$mon, $mday, $hour ,$min, $sec;
>
> -       $tz =~ m/^([+\-][0-9][0-9])([0-9][0-9])$/;
> -       my $local = $epoch + ((int $1 + ($2/60)) * 3600);
> +       my ($tz_sign, $tz_hour, $tz_min) =
> +               ($tz =~ m/^([+\-])([0-9][0-9])([0-9][0-9])$/);

It's just a matter of personal preference, but I would find this
regexp slightly easier to read:

+               ($tz =~ m/^([+\-])([0-9]{2})([0-9]{2})$/);

> +       $tz_sign = ($tz_sign eq '-' ? -1 : +1);
> +       my $local = $epoch + $tz_sign*($tz_hour + ($tz_min/60.0))*3600;

If you wanted to avoid floats, you could do something like:

+       my $local = $epoch + $tz_sign * ($tz_hour * 3600 + $tz_min * 60);

[PATCH (BUGFIX) v2] gitweb: Fix handling of fractional timezones in parse_date

[Jakub Narebski]

On Fri, 25 Mar 2011, Kevin Cernekee wrote:
> 2011/3/25 Jakub Narebski <jnareb@gmail.com>:
> > @@ -2921,8 +2921,10 @@ sub parse_date {
> >        $date{'iso-8601'}  = sprintf "%04d-%02d-%02dT%02d:%02d:%02dZ",
> >                             1900+$year, 1+$mon, $mday, $hour ,$min, $sec;
> >
> > -       $tz =~ m/^([+\-][0-9][0-9])([0-9][0-9])$/;
> > -       my $local = $epoch + ((int $1 + ($2/60)) * 3600);
> > +       my ($tz_sign, $tz_hour, $tz_min) =
> > +               ($tz =~ m/^([+\-])([0-9][0-9])([0-9][0-9])$/);
> 
> It's just a matter of personal preference, but I would find this
> regexp slightly easier to read:
> 
> +               ($tz =~ m/^([+\-])([0-9]{2})([0-9]{2})$/);

I went for minimal changes, same as with the change below.

> > +       $tz_sign = ($tz_sign eq '-' ? -1 : +1);
> > +       my $local = $epoch + $tz_sign*($tz_hour + ($tz_min/60.0))*3600;
> 
> If you wanted to avoid floats, you could do something like:
> 
> +       my $local = $epoch + $tz_sign * ($tz_hour * 3600 + $tz_min * 60);

Note that because valid $tz_min can be only 00, 30 or 45, see 
e.g. http://en.wikipedia.org/wiki/List_of_time_zones_by_UTC_offset 
therefore version using floats would not introduce any rounding
errors: 0, 0.5 and 0.75 can be represented exactly as 2-base float.

Anyway below is patch with above changes:

-- >8 --
Subject: [PATCH] gitweb: Fix handling of fractional timezones in parse_date

Fractional timezones, like -0330 (NST used in Canada) or +0430
(Afghanistan, Iran DST), were not handled properly in parse_date; this
means values such as 'minute_local' and 'iso-tz' were not generated
correctly.

This was caused by two mistakes:

* sign of timezone was applied only to hour part of offset, and not
  as it should be also to minutes part (this affected only negative
  fractional timezones).

* 'int $h + $m/60' is 'int($h + $m/60)' and not 'int($h) + $m/60',
  so fractional part was discarded altogether ($h is hours, $m is
  minutes, which is always less than 60).


Note that positive fractional timezones +0430, +0530 and +1030 can be
found as authortime in git.git repository itself.

For example http://repo.or.cz/w/git.git/commit/88d50e7 had authortime
of "Fri, 8 Jan 2010 18:48:07 +0000 (23:48 +0530)", which is not marked
with 'atnight', when "git show 88d50e7" gives correct author date of
"Sat Jan 9 00:18:07 2010 +0530".

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 0178633..7b9f90b 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2921,8 +2921,10 @@ sub parse_date {
 	$date{'iso-8601'}  = sprintf "%04d-%02d-%02dT%02d:%02d:%02dZ",
 	                     1900+$year, 1+$mon, $mday, $hour ,$min, $sec;
 
-	$tz =~ m/^([+\-][0-9][0-9])([0-9][0-9])$/;
-	my $local = $epoch + ((int $1 + ($2/60)) * 3600);
+	my ($tz_sign, $tz_hour, $tz_min) =
+		($tz =~ m/^([+\-])([0-9]{2})([0-9]{2})$/);
+	$tz_sign = ($tz_sign eq '-' ? -1 : +1);
+	my $local = $epoch + $tz_sign*($tz_hour*3600 + $tz_min*60);
 	($sec, $min, $hour, $mday, $mon, $year, $wday, $yday) = gmtime($local);
 	$date{'hour_local'} = $hour;
 	$date{'minute_local'} = $min;
-- 
1.7.3

Re: [PATCH (BUGFIX)] gitweb: Fix handling of fractional timezones in parse_date

[Junio C Hamano]

Kevin Cernekee <cernekee@gmail.com> writes:

> It's just a matter of personal preference, but I would find this
> regexp slightly easier to read:
>
> +               ($tz =~ m/^([+\-])([0-9]{2})([0-9]{2})$/);

I'd say "^([-+])(\d\d)(\d\d)$" makes it the most clear.

>> +       $tz_sign = ($tz_sign eq '-' ? -1 : +1);
>> +       my $local = $epoch + $tz_sign*($tz_hour + ($tz_min/60.0))*3600;
>
> If you wanted to avoid floats, you could do something like:
>
> +       my $local = $epoch + $tz_sign * ($tz_hour * 3600 + $tz_min * 60);

That is not just float-avoidance, but is much more logical.

	(($h * 60) + $m) * 60

may be even more logical and more readable, though.

Care to re-roll the patch?

Re: [PATCH (BUGFIX)] gitweb: Fix handling of fractional timezones in parse_date

[Jakub Narebski]

Junio C Hamano wrote:
> Kevin Cernekee <cernekee@gmail.com> writes:
> 
> > It's just a matter of personal preference, but I would find this
> > regexp slightly easier to read:
> >
> > +               ($tz =~ m/^([+\-])([0-9]{2})([0-9]{2})$/);
> 
> I'd say "^([-+])(\d\d)(\d\d)$" makes it the most clear.

But what does 'digit character' mean?  Is "\d" Unicode-aware, because
if it is it might match other digits than 0-9?

#perl says:

<rindolf> ShadeHawk: hi.
<rindolf> ShadeHawk: \d matches Unicode digits in unicode contexts I think.
<rindolf> ShadeHawk: like the ones used for Written Arabic.
<rindolf> ShadeHawk: let's see.
<rindolf> perlbot: eval: ["٣" =~ /\d/ ? "Match" : "Nomatch"]
<perlbot> rindolf: ["Match"]
<rindolf> Yay!
<rindolf> That's three in http://en.wikipedia.org/wiki/Hindu%E2%80%93Arabic_numeral_system

Anyway gitweb uses \d in various regexps, so it shouldn't be worse
than it is now.  

[...]

> Care to re-roll the patch?

Will do. 

-- 
Jakub Narebski
ShadeHawk on #git and #perl
Poland

[PATCH (BUGFIX) v3] gitweb: Fix handling of fractional timezones in parse_date

[Jakub Narebski]

Fractional timezones, like -0330 (NST used in Canada) or +0430
(Afghanistan, Iran DST), were not handled properly in parse_date; this
means values such as 'minute_local' and 'iso-tz' were not generated
correctly.

This was caused by two mistakes:

* sign of timezone was applied only to hour part of offset, and not
  as it should be also to minutes part (this affected only negative
  fractional timezones).

* 'int $h + $m/60' is 'int($h + $m/60)' and not 'int($h) + $m/60',
  so fractional part was discarded altogether ($h is hours, $m is
  minutes, which is always less than 60).


Note that positive fractional timezones +0430, +0530 and +1030 can be
found as authortime in git.git repository itself.

For example http://repo.or.cz/w/git.git/commit/88d50e7 had authortime
of "Fri, 8 Jan 2010 18:48:07 +0000 (23:48 +0530)", which is not marked
with 'atnight', when "git show 88d50e7" gives correct author date of
"Sat Jan 9 00:18:07 2010 +0530".

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Junio C Hamano wrote:
> Kevin Cernekee <cernekee@gmail.com> writes:
> 
> > It's just a matter of personal preference, but I would find this
> > regexp slightly easier to read:
> >
> > +               ($tz =~ m/^([+\-])([0-9]{2})([0-9]{2})$/);
> 
> I'd say "^([-+])(\d\d)(\d\d)$" makes it the most clear.
> 
> > > +       $tz_sign = ($tz_sign eq '-' ? -1 : +1);
> > > +       my $local = $epoch + $tz_sign*($tz_hour + ($tz_min/60.0))*3600;
> >
> > If you wanted to avoid floats, you could do something like:
> >
> > +       my $local = $epoch + $tz_sign * ($tz_hour * 3600 + $tz_min * 60);
> 
> That is not just float-avoidance, but is much more logical.
> 
> 	(($h * 60) + $m) * 60
> 
> may be even more logical and more readable, though.
> 
> Care to re-roll the patch?

Re-rolled.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 0178633..ee69ea6 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2921,8 +2921,10 @@ sub parse_date {
 	$date{'iso-8601'}  = sprintf "%04d-%02d-%02dT%02d:%02d:%02dZ",
 	                     1900+$year, 1+$mon, $mday, $hour ,$min, $sec;
 
-	$tz =~ m/^([+\-][0-9][0-9])([0-9][0-9])$/;
-	my $local = $epoch + ((int $1 + ($2/60)) * 3600);
+	my ($tz_sign, $tz_hour, $tz_min) =
+		($tz =~ m/^([-+])(\d\d)(\d\d)$/);
+	$tz_sign = ($tz_sign eq '-' ? -1 : +1);
+	my $local = $epoch + $tz_sign*((($tz_hour*60) + $tz_min)*60);
 	($sec, $min, $hour, $mday, $mon, $year, $wday, $yday) = gmtime($local);
 	$date{'hour_local'} = $hour;
 	$date{'minute_local'} = $min;
-- 
1.7.3

Re: [PATCH 1/2] gitweb: rename parse_date() to format_date()

[Jakub Narebski]

On Sat, 19 Mar 2011, Kevin Cernekee wrote:

> 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");

First, one would expect parse_date() to return either list of year, 
month, etc., e.g. [2011, 3, 19, ...], or a hash with named
date fragments, e.g. {'year' => 2011, 'month' => 3, ...}.

Almost all other parse_* subroutines in gitweb (parse_commit, parse_tag,
parse_difftree_raw_line, parse_ls_tree_line, with the sole exception of
parse_from_to_diffinfo with perhaps a bit misleading name) return hash
of parsed and pre-formatted fragments.

So if parse_date(1300505805) returned

   (
     'hour' => 3,
     'minute' => 36,
     ...
   )

_without_ the 'rfc2822', 'mday-time' (unused), 'iso-8601', 'iso-tz',
it would be not misnamed parse_*.


Second, it is _date_ because it parses / processes dates in git internal
format.  Perhaps it should be called *_raw_date, or *_git_date, or *_epoch,
or *_timestamp and not *_date.
 
> But instead, gitweb's parse_date works more like:
> 
> &parse_date(1300505805) = {

Note: parse_date takes second argument, which is numeric timezone (defaults
to '-0000'), so one usually uses

  parse_date(1300505805, '+0200')

>         'hour' => 3,
>         'minute' => 36,
>         ...
>         'rfc2822' => 'Sat, 19 Mar 2011 03:36:45 +0000',
>         ...
> }

What I would like to see is to split parsing date from generating formatted
dates into e.g. parse_date and formatted_date, and replace calls to parse_date
with calls to formatted_date (which uses parse_date internally).
 
> Rename the function [to format_date] to improve clarity.  
> No change to functionality. 

But name *format_date* is _not_ a good name.  All other format_* 
subroutines in gitweb return _formatted string_, not a hash which contains
many formatted strings.

It is hard to come up with really good name, but perhaps one from the ;ist
below would be good idea:

 - formatted_date
 - process_date, process_raw_date, process_git_date
 - process_epoch, process_timestamp
 - date_formats
 - from_epoch, date_from_epoch

Any other ideas?
 
> Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
> ---
>  gitweb/gitweb.perl |   18 +++++++++---------
>  1 files changed, 9 insertions(+), 9 deletions(-)

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

Hmm, we should probably fix it so it reads

  -		my %ad = parse_date($co{'author_epoch'});
  +		my %ad = formatted_date($co{'author_epoch'}, $co{'author_tz'});

Especially when preparing for 'localtime' feature.

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

Similarly here.

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

Same here.

-- 
Jakub Narebski
Poland

Re: [PATCH 1/2] gitweb: rename parse_date() to format_date()

[Jon Seymour]

On Sat, Mar 19, 2011 at 9:33 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Sat, 19 Mar 2011, Kevin Cernekee wrote:
>
>  - formatted_date
>  - process_date, process_raw_date, process_git_date
>  - process_epoch, process_timestamp
>  - date_formats
>  - from_epoch, date_from_epoch
>

How about: decompose_date?

jon.

Re: [PATCH 1/2] gitweb: rename parse_date() to format_date()

[Junio C Hamano]

Kevin Cernekee <cernekee@gmail.com> writes:

> 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) = {
>         'hour' => 3,
>         'minute' => 36,
>         ...
>         'rfc2822' => 'Sat, 19 Mar 2011 03:36:45 +0000',
>         ...
> }
>
> Rename the function to improve clarity.  No change to functionality.

Except for a small detail that the function also takes the "-0800"
timezone string, I think this is a good change.

The word "parse" makes the readers expect that its input is something
"format" would have produced for use by others (often humans, but there
are functions that parse xml ;-).