From: Kevin Cernekee Date: Sat, 19 Mar 2011 21:22:21 GMT Subject: Re: [PATCH v4 2/2] gitweb: introduce localtime feature Message-ID: In-Reply-To: <201103192209.29759.jnareb@gmail.com> On Sat, Mar 19, 2011 at 2:09 PM, Jakub Narebski wrote: >> 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. >> 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. >> 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. > 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.