Re: [PATCH v3 2/3] gitweb: introduce localtime feature

[Jakub Narebski <jnareb@gmail.com>]

On Fri, 18 March 2011, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:

> > With this feature enabled, all timestamps are shown in the local
> > timezone instead of GMT.  The timezone is taken from the appropriate
> > timezone string stored in the commit object.
> >
> > This is useful if most of contributors (to a project) are based in a
> > single office, all within the same timezone.  In such case local time
> > is more useful than GMT / UTC time that gitweb uses by default, and
> > which is better choice for geographically scattered contributors.
[...]

> Thanks for moving the explanation up into the log message.  Much easier to
> understand the motivation.
 
> > @@ -2930,6 +2943,12 @@ sub parse_date {
> >  	$date{'iso-tz'} = sprintf("%04d-%02d-%02d %02d:%02d:%02d %s",
> >  	                          1900+$year, $mon+1, $mday,
> >  	                          $hour, $min, $sec, $tz);
> > +
> > +	if (gitweb_check_feature('localtime')) {
> > +		$date{'rfc2822'} = sprintf "%s, %d %s %4d %02d:%02d:%02d $tz",
> > +		                   $days[$wday], $mday, $months[$mon],
> > +		                   1900+$year, $hour ,$min, $sec;
> > +	}
> >  	return %date;
> >  }

> (hint: when reviewing, look a bit wider outside the context
> provided by the patch):

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

> Two comments: 
> 
>  - This gets seconds-since-epoch and returns a bag of pieces of formatted
>    timestamp (some are mere elements like "hour", some are full timestamp
>    like "iso-8601").  Doesn't sound like "parse"-date, does it?

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

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

>  - It looks somewhat ugly to unconditionally assign to 'rfc2822' first
>    (before the context of the hunk) and then overwrite it.  Wouldn't it be
>    more useful later to have a separate 'rfc2822_local' field, just like
>    existing 'hour_local' and 'minute_local' are counterparts for 'hour'
>    and 'minute'?

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

This would not be needed with proposed by you refactoring that makes
parse_date do only parsing of timestamp + timezone.
 
> > @@ -3992,7 +4011,7 @@ sub git_print_authorship_rows {
> >  		      "</td></tr>\n" .
> >  		      "<tr>" .
> >  		      "<td></td><td> $wd{'rfc2822'}";
> > -		print_local_time(%wd);
> > +		print_local_time(%wd) if !gitweb_check_feature('localtime');
> >  		print "</td>" .
> >  		      "</tr>\n";
> >  	}
> 
> Very confusing.  "Ok, we print local time. --ah, wait, only when localtime
> feature is not used???"
> 
> It turns out that the hijacking of $wd{'rfc2822'} made above already gives
> us the local time so this patch turns the meaning of print-local-time used
> here into additionally-print-local-time.

You are right that is very confusing.

> 
> Both call sites to print_local_time() follow this pattern:
> 
> 	print "... some string ..." .
>         	"... that is sometimes long ..." .
>                 "... and more but ends with $bag{'rfc2822'}";
> 	print_local_time(%bag); # perhaps if "some condition";
> 	print "... more string ...";
> 
> I am referring to "if (${opts-localtime})" in the existing code and
> "if !gitweb_c_f('localtime')" in this patch as "some condition".
> 
> It appears to me that it may be a better idea to hide the "rfc2822" part
> as an implementation detail behind a helper function, to make the above
> pattern to look perhaps like this:
> 
> 	print "... some string ..." .
>         	"... that is sometimes long ..." .
>                 "... and more but ends with " .
> 		timestamp_string(%bag, "some condition") .
> 	        "... more string ...";

Yes, it is better idea.


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

Those formats are:

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

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

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

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

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

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

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


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

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

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

-- 
Jakub Narebski
Poland