On Thu, Jan 17, 2013 at 7:09 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Ben Walton <bdwalton@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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
---------------------------------------------------------------------------------------------------------------------------
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html