Jakub Narebski <jnareb@xxxxxxxxx> writes:
> From: Kevin Cernekee <cernekee@xxxxxxxxx>
>
> 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.
>
> This change does not affect relative timestamps (e.g. "5 hours ago"),
> and neither 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/comitter,
> marking localtime with "atnight" as appropriate; after this commit
> gitweb shows only local time. Marking localtime with "atnight" when
> needed is left for subsequent commit.
>
> Signed-off-by: Kevin Cernekee <cernekee@xxxxxxxxx>
> Signed-off-by: Jakub Narebski <jnareb@xxxxxxxxx>
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;
> }
Two comments (hint: when reviewing, look a bit wider outside the context
provided by the patch):
- 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?
- 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'?
> @@ -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.
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 ...";
Hmm?
--
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