On Fri, 18 March 2011, Junio C Hamano wrote:
> Jakub Narebski <jnareb@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html