Re: [PATCH 1/2] gitweb: rename parse_date() to format_date()

[Jakub Narebski <jnareb@gmail.com>]

On Sat, 19 Mar 2011, Kevin Cernekee wrote:

> One might reasonably expect a function named parse_date() to be used
> for something along these lines:
> 
> $unix_time_t = parse_date("2011-03-19");

First, one would expect parse_date() to return either list of year, 
month, etc., e.g. [2011, 3, 19, ...], or a hash with named
date fragments, e.g. {'year' => 2011, 'month' => 3, ...}.

Almost all other parse_* subroutines in gitweb (parse_commit, parse_tag,
parse_difftree_raw_line, parse_ls_tree_line, with the sole exception of
parse_from_to_diffinfo with perhaps a bit misleading name) return hash
of parsed and pre-formatted fragments.

So if parse_date(1300505805) returned

   (
     'hour' => 3,
     'minute' => 36,
     ...
   )

_without_ the 'rfc2822', 'mday-time' (unused), 'iso-8601', 'iso-tz',
it would be not misnamed parse_*.


Second, it is _date_ because it parses / processes dates in git internal
format.  Perhaps it should be called *_raw_date, or *_git_date, or *_epoch,
or *_timestamp and not *_date.
 
> But instead, gitweb's parse_date works more like:
> 
> &parse_date(1300505805) = {

Note: parse_date takes second argument, which is numeric timezone (defaults
to '-0000'), so one usually uses

  parse_date(1300505805, '+0200')

>         'hour' => 3,
>         'minute' => 36,
>         ...
>         'rfc2822' => 'Sat, 19 Mar 2011 03:36:45 +0000',
>         ...
> }

What I would like to see is to split parsing date from generating formatted
dates into e.g. parse_date and formatted_date, and replace calls to parse_date
with calls to formatted_date (which uses parse_date internally).
 
> Rename the function [to format_date] to improve clarity.  
> No change to functionality. 

But name *format_date* is _not_ a good name.  All other format_* 
subroutines in gitweb return _formatted string_, not a hash which contains
many formatted strings.

It is hard to come up with really good name, but perhaps one from the ;ist
below would be good idea:

 - formatted_date
 - process_date, process_raw_date, process_git_date
 - process_epoch, process_timestamp
 - date_formats
 - from_epoch, date_from_epoch

Any other ideas?
 
> Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
> ---
>  gitweb/gitweb.perl |   18 +++++++++---------
>  1 files changed, 9 insertions(+), 9 deletions(-)

> @@ -4906,7 +4906,7 @@ sub git_log_body {
>  		next if !%co;
>  		my $commit = $co{'id'};
>  		my $ref = format_ref_marker($refs, $commit);
> -		my %ad = parse_date($co{'author_epoch'});
> +		my %ad = format_date($co{'author_epoch'});
>  		git_print_header_div('commit',
>  		               "<span class=\"age\">$co{'age_string'}</span>" .
>  		               esc_html($co{'title'}) . $ref,

Hmm, we should probably fix it so it reads

  -		my %ad = parse_date($co{'author_epoch'});
  +		my %ad = formatted_date($co{'author_epoch'}, $co{'author_tz'});

Especially when preparing for 'localtime' feature.

> @@ -7064,7 +7064,7 @@ sub git_feed {
>  	if (defined($commitlist[0])) {
>  		%latest_commit = %{$commitlist[0]};
>  		my $latest_epoch = $latest_commit{'committer_epoch'};
> -		%latest_date   = parse_date($latest_epoch);
> +		%latest_date   = format_date($latest_epoch);
>  		my $if_modified = $cgi->http('IF_MODIFIED_SINCE');
>  		if (defined $if_modified) {
>  			my $since;

Similarly here.

> @@ -7195,7 +7195,7 @@ XML
>  		if (($i >= 20) && ((time - $co{'author_epoch'}) > 48*60*60)) {
>  			last;
>  		}
> -		my %cd = parse_date($co{'author_epoch'});
> +		my %cd = format_date($co{'author_epoch'});
>  
>  		# get list of changed files
>  		open my $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,

Same here.

-- 
Jakub Narebski
Poland