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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 18, 2011, 20:48 UTC
Message-ID: <7vmxksw3x8.fsf@alter.siamese.dyndns.org>
In-Reply-To: <201103181846.04979.jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> writes:

Show 8 quoted lines

> 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.

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?

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";

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;
	}

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?

Show 35 quoted lines

>  gitweb/gitweb.perl |   16 ++++++++++++----
>  1 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index cdc2a96..5bda0a8 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -4003,15 +4003,23 @@ sub git_print_authorship_rows {
>  		my %wd = parse_date($co->{"${who}_epoch"}, $co->{"${who}_tz"});
>  		print "<tr><td>$who</td><td>" .
>  		      format_search_author($co->{"${who}_name"}, $who,
> -			       esc_html($co->{"${who}_name"})) . " " .
> +		                           esc_html($co->{"${who}_name"})) . " " .
>  		      format_search_author($co->{"${who}_email"}, $who,
> -			       esc_html("<" . $co->{"${who}_email"} . ">")) .
> +		                           esc_html("<" . $co->{"${who}_email"} . ">")) .
>  		      "</td><td rowspan=\"2\">" .
>  		      git_get_avatar($co->{"${who}_email"}, -size => 'double') .
>  		      "</td></tr>\n" .
>  		      "<tr>" .
> -		      "<td></td><td> $wd{'rfc2822'}";
> -		print_local_time(%wd) if !gitweb_check_feature('localtime');
> +		      "<td></td><td> ";
> +		if (gitweb_check_feature('localtime')) {
> +			if ($wd{'hour_local'} < 6) {
> +				print "<span class=\"atnight\">$wd{'rfc2822'}</span>";
> +			} else {
> +				print $wd{'rfc2822'};
> +			}
> +		} else {
> +			print $wd{'rfc2822'} . format_local_time(%wd);
> +		}
>  		print "</td>" .
>  		      "</tr>\n";
>  	}