Re: [PATCH 3/3 (alternate)] gitweb: Mark "atnight" author/committer times also for 'localtime'

From: Jakub Narebski <jnareb@gmail.com>
Date: Mar 18, 2011, 22:28 UTC
Message-ID: <201103182328.19141.jnareb@gmail.com>
In-Reply-To: <7vmxksw3x8.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:

Show 13 quoted lines

> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > Kevin, how about something like this instead?  This preserves _intent_
> > for why there is local time beside GMT time when 'localtime' is disabled
> > better, I think.
> >
> > Junio and Kevin, I am not sure if authorship should remain with Kevin,
> > or should it revert to me; the solution is quite different.
> > About no-change to git_print_authorship: alternate solution would be
> > to remove support for -localtime option, like in original patches.
> 
> I don't think it is worth anything to keep dead code that anybody
> exercises to support -localtime option that nobody asks.

Right. Especially that refactoring would significantly change this code.

Show 6 quoted lines

> I thought we were getting closer (especially if you consider my suggestion
> to the earlier round, but obviously I am biased), but this looks far worse
> than your previous clean-up of Kevin's patch.  What is the point of
> duplicating the atnight logic her?  Why not kill the useless helper
> function "print-local-time", and instead enhance "format-local-time" so
> that whatever this added code does is performed there when the caller asks?

You are right.  I concentrated on the fact that IMVHO Kevin's version of
this 3/3 solved wrong issue (the problem is not GMT + local, but atnight
which needs localtime), but forgot to stop to think how ugly and
duplicated the code is... or at least mark this patch as RFC, request
for comments on resulting look.

Show 7 quoted lines

> Then the caller here would look more or less like:
> 
> 	print "<tr><td>$who</td>" .
>             "... author name, email, avatar ..." .
> 	      "<td></td><td>" .
>             format_timestamp(\%wd, gitweb_check_feature('localtime')) .
> 	      "</td></tr>\n";

Right.

Show 22 quoted lines

> 
> and format_timestamp would be like
> 
> 	sub format_timestamp {
> 		my %date = %$_[0];
>       	my $use_localtime = $_[1];
> 		my $localtime, $ret, $nite;
> 
> 		$nite = ($date{'hour_local'} < 6);
> 
> 		if ($use_localtime) {
> 			$ret = $date{'rfc2822_local'};
>       		if ($nite) {
>                         	$ret = sprintf("<span class='atnight'>%s</span>", $ret);
> 			}
> 		} else {
> 			... what the current format_local_time does to set
> 	        	... including the spanning part
>               	$ret = "$date{'rfc2822'} ($localtime)";
> 		}
> 		return $ret;
> 	}

Well, if we go this route, and assuming that parse_date does only parsing
and we use separate subroutine for generating date in an rfc2822 format,
then we could mark only time with "atnight" also when 'localtime' feature
is enabled.

> Wouldn't it be much cleaner?  You can then clean up the other call site of
> print_local_time in git_print_authorship using the same helper function
> (presumably you would always pass 0 to $use_localtime there), no?

Right. Well, I'd have to think a bit about API for format_timestamp, but it looks like good direction.

-- 
Jakub Narebski
Poland