Re: [PATCH 2/3] Allow Git::get_tz_offset to properly handle DST boundary times

[Ben Walton <bdwalton@gmail.com>]

On Thu, Jan 17, 2013 at 7:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ben Walton <bdwalton@gmail.com> writes:
>
>> The Git::get_tz_offset is meant to provide a workalike replacement for
>> the GNU strftime %z format specifier.  The algorithm used failed to
>> properly handle DST boundary cases.
>>
>> For example, the unix time 1162105199 in CST6CDT saw set_tz_offset
>> improperly return -0600 instead of -0500.
>>
>> TZ=CST6CDT date -d @1162105199 +"%c %z"
>> Sun 29 Oct 2006 01:59:59 AM CDT -0500
>>
>> $ zdump -v /usr/share/zoneinfo/CST6CDT | grep 2006
>> /usr/share/zoneinfo/CST6CDT  Sun Apr  2 07:59:59 2006 UTC = Sun Apr  2
>> 01:59:59 2006 CST isdst=0 gmtoff=-21600
>> /usr/share/zoneinfo/CST6CDT  Sun Apr  2 08:00:00 2006 UTC = Sun Apr  2
>> 03:00:00 2006 CDT isdst=1 gmtoff=-18000
>> /usr/share/zoneinfo/CST6CDT  Sun Oct 29 06:59:59 2006 UTC = Sun Oct 29
>> 01:59:59 2006 CDT isdst=1 gmtoff=-18000
>> /usr/share/zoneinfo/CST6CDT  Sun Oct 29 07:00:00 2006 UTC = Sun Oct 29
>> 01:00:00 2006 CST isdst=0 gmtoff=-21600
>>
>> To determine how many hours/minutes away from GMT a particular time
>> was, we calculated the gmtime() of the requested time value and then
>> used Time::Local's timelocal() function to turn the GMT-based time
>> back into a scalar value representing seconds from the epoch.  Because
>> GMT has no daylight savings time, timelocal() cannot handle the
>> ambiguous times that occur at DST boundaries since there are two
>> possible correct results.
>>
>> To work around the ambiguity at these boundaries, we must take into
>> account the pre and post conversion values for is_dst as provided by
>> both the original time value and the value that has been run through
>> timelocal().  If the is_dst field of the two times disagree then we
>> must modify the value returned from timelocal() by an hour in the
>> correct direction.
>
> It seems to me that it is a very roundabout way.  It may be correct,
> but it is unclear why the magic +/-3600 shift is the right solution
> and I suspect even you wouldn't notice if I sent you back your patch
> with a slight change to swap $gm += 3600 and $gm -= 3600 lines ;-).
>
> For that timestamp in question, the human-readable representation of
> gmtime($t) and localtime($t) look like these two strings:
>
>         my $t = 1162105199;
>         print gmtime($t), "\n";    # Sun Oct 29 06:59:59 2006
>         print localtime($t), "\n"; # Sun Oct 29 01:59:59 2006
>
> As a human, you can easily see that these two stringified timestamps
> look 5 hours apart.  Think how you managed to do so.
>
> If we convert these back to the seconds-since-epoch, as if these
> broken-down times were both in a single timezone that does not have
> any DST issues, you can get the offset (in seconds) by subtraction,
> and that is essentially the same as the way in which your eyes saw
> they are 5 hours apart, no?  In other words, why do you need to run
> timelocal() at all?
>
>         my $t = 1162105199;
>         my $lct = timegm(localtime($t));
>         # of course, timegm(gmtime($t)) == $t
>
>         my $minutes = int(($lct - $t)/60);
>         my $sign "+";
>         if ($minutes < 0) {
>                 $sign = "-";
>                 $minutes = -$minutes;
>         }
>         my $hours = int($minutes/60);
>         $minutes -= $hours * 60;
>         sprintf("%s%02d%02d", $sign, $hours, $minutes);
>
> Confused...
>
>>
>> Signed-off-by: Ben Walton <bdwalton@gmail.com>
>> ---
>>  perl/Git.pm |   20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/perl/Git.pm b/perl/Git.pm
>> index 5649bcc..788b9b4 100644
>> --- a/perl/Git.pm
>> +++ b/perl/Git.pm
>> @@ -528,7 +528,27 @@ If TIME is not supplied, the current local time is used.
>>  sub get_tz_offset {
>>       # some systmes don't handle or mishandle %z, so be creative.
>>       my $t = shift || time;
>> +     # timelocal() has a problem when it comes to DST ambiguity so
>> +     # times that are on a DST boundary cannot be properly converted
>> +     # using it.  we will possibly adjust its result depending on whehter
>> +     # pre and post conversions agree on DST
>>       my $gm = timelocal(gmtime($t));
>> +
>> +     # we need to know whether we were originally in DST or not
>> +     my $orig_dst = (localtime($t))[8];
>> +     # and also whether timelocal thinks we're in DST
>> +     my $conv_dst = (localtime($gm))[8];
>> +
>> +     # re-adjust $gm based on the DST value for the two times we're
>> +     # handling.
>> +     if ($orig_dst != $conv_dst) {
>> +             if ($orig_dst == 1) {
>> +                     $gm -= 3600;
>> +             } else {
>> +                     $gm += 3600;
>> +             }
>> +     }
>> +
>>       my $sign = qw( + + - )[ $t <=> $gm ];
>>       return sprintf("%s%02d%02d", $sign, (gmtime(abs($t - $gm)))[2,1]);
>>  }


Sorry for the slow response, I didn't have a good chance to look at
this until now.  You are correct; your solution appears simpler and
also avoids the oddball 1/2 hour DST shift.  I can re-roll the series
with your code (and credit for it) or you can apply you change on top
of my series...whichever is easiest for you.

Thanks
-Ben
--
---------------------------------------------------------------------------------------------------------------------------
Take the risk of thinking for yourself.  Much more happiness,
truth, beauty and wisdom will come to you that way.

-Christopher Hitchens
---------------------------------------------------------------------------------------------------------------------------