Re: [PATCH v4 2/2] gitweb: introduce localtime feature

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 19, 2011, 17:56 UTC
Message-ID: <7v39mjro38.fsf@alter.siamese.dyndns.org>
In-Reply-To: <dab08d0ff27b0f571a17ed4f1ab0f39b@localhost>

Kevin Cernekee <cernekee@gmail.com> writes:

Show 6 quoted lines

> -sub format_local_time {
> -	my $localtime = '';
> -	my %date = @_;
> -	if ($date{'hour_local'} < 6) {
> -		$localtime .= sprintf(" (<span class=\"atnight\">%02d:%02d</span> %s)",
> -			$date{'hour_local'}, $date{'minute_local'}, $date{'tz_local'});

Looks like we used to only paint HH:MM part but...

Show 37 quoted lines

> +# Returns an RFC 2822 timestamp string, which may contain HTML.
> +# If $use_localtime is 0, don't do anything special.
> +# If $use_localtime is 1, add an alternate HH:MM timestamp in parentheses at
> +# the end.  If $feature{'localtime'} is enabled this looks like:
> +#   Thu, 17 Mar 2011 18:28:57 -0700 (01:28 +0000)
> +# Otherwise, it looks like:
> +#   Fri, 18 Mar 2011 01:28:57 +0000 (18:28 -0700)
> +# If $use_localtime is 1, this will also apply the "atnight" style to
> +# local times between 00:00 and 05:59.
> +sub timestamp_html {
> +	my %date = %{$_[0]};
> +	my $use_localtime = $_[1];
> +	my $timestamp;
> +	my $alt_time;
> +
> +	if (gitweb_check_feature('localtime')) {
> +		$timestamp = $date{'rfc2822_local'};
> +		if ($use_localtime && $date{'hour_local'} < 6) {
> +			$timestamp = "<span class=\"atnight\">" .
> +			             $timestamp .
> +			             "</span>";
> +		}
> +		$alt_time = sprintf(" (%02d:%02d %s)",
> +		                    $date{'hour'}, $date{'minute'}, "+0000");
>  	} else {
> -		$localtime .= sprintf(" (%02d:%02d %s)",
> -			$date{'hour_local'}, $date{'minute_local'}, $date{'tz_local'});
> +		$timestamp = $date{'rfc2822'};
> +		$alt_time = sprintf(" (%02d:%02d %s)",
> +				    $date{'hour_local'},
> +				    $date{'minute_local'},
> +				    $date{'tz_local'});
> +		if ($use_localtime && $date{'hour_local'} < 6) {
> +			$alt_time = "<span class=\"atnight\">" .
> +			            $alt_time .
> +			            "</span>";
> +		}

... we now paint the whole line, which I personally think is a friendly move for color challenged people (me included---a larger span of text painted in different colors tends to help you still notice it better using value/brightness difference, even if your hue perception is weaker than other people). But it is a change from the old behaviour and might be worth stating in the log message.

> -	return $localtime;
> +	if ($use_localtime) {
> +		$timestamp .= $alt_time;
> +	}

You kept -localtime nobody uses? If you dig the history, you would notice that it is merely a remnant of f88bafa (gitweb: uniform author info for commit and commitdiff, 2009-06-30) that forgot to remove the feature while removing the last callsite, and I don't think anybody even was against such a change.

I'd suggest to remove $alt_time codepath from this function. Nobody will notice.

Show 17 quoted lines

>  # Outputs the author name and date in long form
> @@ -3956,10 +3999,9 @@ sub git_print_authorship {
>  	my %ad = format_date($co->{'author_epoch'}, $co->{'author_tz'});
>  	print "<$tag class=\"author_date\">" .
>  	      format_search_author($author, "author", esc_html($author)) .
> -	      " [$ad{'rfc2822'}";
> -	print_local_time(%ad) if ($opts{-localtime});
> -	print "]" . git_get_avatar($co->{'author_email'}, -pad_before => 1)
> -		  . "</$tag>\n";
> +	      " [" . timestamp_html(\%ad, 0) . "] ".
> ...
>  # Outputs table rows containing the full author or committer information,
> @@ -3983,9 +4025,9 @@ sub git_print_authorship_rows {
> -		print "</td>" .
> +		      "<td></td><td> " .
> +		      timestamp_html(\%wd, 1) .
> +		      "</td>" .

While I don't agree with Jakub about the presense / absence of "[]" around it (that is not a part of "timestamp", but is how the caller wants to prepare the space to plug a timestamp string in), I do agree that the second parameter "use-localtime" looks funny, not because it is an unnamed parameter (which I personally think is fine) but because it isn't about the localness of the displayed time anymore. Your 'localtime' feature alone controls in which timezone the timestamp is shown, and this only controls the use of 'atnight' highlighting. The parameter needs to be renamed, and perhaps you may clarify it further by making it a keyword argument as Jakub suggests.

Other than that, I think this round is very nicely done.

This is a complete tangent, but I wonder if it makes sense to keep the output from print_authorship plain (no atnight markings) while painting the output from print_authorship_rows.

I would understand it if the design _were_ to paint late-nite commits differently when we list multiple commits in a row (e.g. summary view) to make them easier to stand out (for amusement purposes), and not to paint the timestamp when we show only one commit to avoid distraction. But that doesn't seem to be the case.