Re: [PATCH v3 2/3] gitweb: introduce localtime feature

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

Jakub Narebski <jnareb@gmail.com> writes:

Show 29 quoted lines

> From: Kevin Cernekee <cernekee@gmail.com>
>
> 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@gmail.com>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>

Thanks for moving the explanation up into the log message. Much easier to understand the motivation.

Show 12 quoted lines

> @@ -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'?

Show 9 quoted lines

> @@ -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?