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

From: Kevin Cernekee <cernekee@gmail.com>
Date: Mar 19, 2011, 21:22 UTC
Message-ID: <AANLkTikw7_9Q0MgVZZxX_dXvcxoXfx5VEmRQkVH_k1YC@mail.gmail.com>
In-Reply-To: <201103192209.29759.jnareb@gmail.com>

On Sat, Mar 19, 2011 at 2:09 PM, Jakub Narebski <jnareb@gmail.com> wrote:

Show 13 quoted lines

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

Oh OK, I did not notice this or intend to change the formatting.  My bad.

Show 11 quoted lines

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

I can fix up v4 if Junio thinks this is a better way to go...

It does make the code a little more complicated, though. If we paint the entire string, it can be done once toward the end of the function, rather than twice (two different ways) inside the localtime / no-localtime clauses.

I am largely indifferent to how "atnight" is handled but it would be good to arrive at a consensus.

Show 10 quoted lines

>> 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 = @_;

This did not work for me.  It interprets "shift" as a variable name.

Show 6 quoted lines

> Or just use hash reference for $date:
>
>  sub timestamp_html {
>      my ($date, %opts) = @_;
>
> and use $date->{'sth'}.

That is what I did on the latest 3/3 (change "atnight" highlighting) patch. Seemed to work OK.

Thanks.