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

2013-01-20 Thread Ben Walton
On Thu, Jan 17, 2013 at 7:09 PM, Junio C Hamano gits...@pobox.com wrote:
 Ben Walton bdwal...@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 bdwal...@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
--

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

2013-01-20 Thread Junio C Hamano
Ben Walton bdwal...@gmail.com writes:

 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.

Please reroll, as I do not have patience either to set up a test
case and verify the end result is correct, or to come up with a test
case for it.  For this particular case, I think the identification
of the issue weighs more than the implementation for fix it, so
please retain the authorship for the fix; mentioning taking the
implementation idea from Junio in the log message is the right
amount of credit due to me.

Thanks.


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2013-01-17 Thread Junio C Hamano
Ben Walton bdwal...@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 bdwal...@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]);
  }
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html