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

[Jakub Narebski <jnareb@gmail.com>]

On Sat, 19 Mar 2011, Kevin Cernekee wrote:
> On Sat, Mar 19, 2011 at 10:56 AM, Junio C Hamano <gitster@pobox.com> wrote:

> > Looks like we used to only paint HH:MM part but...
> > ... 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.
> 
> For the $feature{'localtime'} disabled case, the coloring is the same as before.

No, it is not the same.  It used to be

  Wed, 16 Mar 2011 07:02:42 +0000 (02:02 -0500)
                                   ^^^^^

and now it is

  Wed, 16 Mar 2011 07:02:42 +0000 (02:02 -0500)
                                  ^^^^^^^^^^^^^

which is I also think improvement,... but should be mentioned in the
commit message.
 
> I will paint the whole line in the next spin, and mention it in the
> commit message.

I think current solution of using

  Wed, 16 Mar 2011 02:02:42 -0500 (07:02:42 +0000)
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

when 'localtime' feature is enabled is better than painting the whole:
you are now painting the _local_ part, i.e the part responsible for
"atnight" warning.


[...]
> $use_localtime indicates whether or not to add the " (hh:mm -zzzz)" at
> the end.  This also enables the atnight coloring.
> 
> This argument name was suggested in an earlier post and I guess I took
> it a little too literally...
> 
> Do you think it would be a good idea to take two separate options:
> -atnight for the variable coloring, and -alt_time (or some other name)
> to show " (hh:mm -zzzz)" after the RFC 2822 string?

Perhaps name this parameter -long.  Or simply always use the long form.

> Or maybe take one option, named something like "-commitpage", to
> indicate that it is a format specific to that view?  If it is not
> specified, the caller gets back an uncolored RFC 2822 date.
> 
> Also, is there a cleaner way of writing this?
> 
> sub timestamp_html {
>     my %date = %{$_[0]};
>     shift;
>     my %opts = @_;

  sub timestamp_html {
      my %date = %{ shift };
      my %opts = @_;


> 
> Or should I pass in the options as a hash reference, more like $cgi->a():
> 
> sub timestamp_html {
>     my %date = %{$_[0]};
>     my %opts = %{$_[1]};

Or just use hash reference for $date:

  sub timestamp_html {
      my ($date, %opts) = @_;

and use $date->{'sth'}.

-- 
Jakub Narebski
Poland