Re: [PATCH -1/3] gitweb: Always call parse_date with timezone parameter

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 20, 2011, 00:27 UTC
Message-ID: <7v39miws8p.fsf@alter.siamese.dyndns.org>
In-Reply-To: <AANLkTimV7vvD0PTMejydiyW_CeUH0cuQ-2+PnRqjzob5@mail.gmail.com>

Kevin Cernekee <cernekee@gmail.com> writes:

Show 7 quoted lines

> On Sat, Mar 19, 2011 at 3:18 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> ...
> Should be 'author_tz'
>
> Looking at the master branch, I don't see %ad actually getting used
> anywhere?  Maybe it is safe to delete the line entirely, since
> git_print_authorship() calls parse_date() itself.

Thanks for being careful, I agree "while at it, remove an unnecessary call" would be better.

Show 6 quoted lines

>> @@ -7064,7 +7064,7 @@ sub git_feed {
> ...
> Should be 'committer_tz'
>
> I would agree that it isn't such a good thing for
> $latest_date{'rfc2822_local'} to be set to GMT in this case.

I would say it isn't good in _any_ case.

If the localtime conversion logic _were_ very expensive, and if the function that fills 'rfc2822_local' _were_ written to allow callers that do not need local time by passing undef to its tz parameter to avoid the computation cost, it may make sense to allow 'rfc2822_local' to be undef, but neither is true. This callsite was simply buggy.

> Am I correct in interpreting "PATCH -1/3" as: "apply this before
> Kevin's set of 3 patches?"

I had the same reaction; judging from the contents, I now realize that is what was meant by "minus one", but originally I read that hyphen as "I don't know how many iterations we had so instead of writing v$N I am just striking it out" ;-)