Re: [PATCH v3 2/3] gitweb: introduce localtime feature
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Mar 18, 2011, 21:58 UTC
- Message-ID
- <201103182258.43368.jnareb@gmail.com>
- In-Reply-To
- <7vy64cwal7.fsf@alter.siamese.dyndns.org>
On Fri, 18 March 2011, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
Show 8 quoted lines
> > 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.
Show 12 quoted lines
> > @@ -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.
Show 5 quoted lines
> 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.
Show 5 quoted lines
> - 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.
Show 16 quoted lines
> > @@ -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.
Show 21 quoted lines
>
> 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