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

From: Jakub Narebski <jnareb@gmail.com>
Date: Mar 19, 2011, 15:18 UTC
Message-ID: <201103191618.29826.jnareb@gmail.com>
In-Reply-To: <dab08d0ff27b0f571a17ed4f1ab0f39b@localhost>

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.

[...]

Show 8 quoted lines

> 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)
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[...]

Show 15 quoted lines

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

Show 6 quoted lines

>             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