{"thread":{"id":"32641","subject":"[PATCH 1/3] Move Git::SVN::get_tz to Git::get_tz_offset","startedAt":"2013-01-15T23:10:02Z","lastAt":"2013-01-20T21:03:54Z","messageCount":13,"participants":["Ben Walton","Junio C Hamano","Chris Rorvick"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"206986","messageId":"1358291405-10173-1-git-send-email-bdwalton@gmail.com","threadId":"32641","inReplyTo":null,"subject":"[PATCH 0/3] Fix a portability issue with git-cvsimport","fromName":"Ben Walton","fromEmail":"bdwalton@gmail.com","sentAt":"2013-01-15T23:10:02Z","receivedAt":"2013-01-15T23:10:02Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"This patch series started as a quick fix for the use of %s and %z in\ngit-cvsimport but grew slightly when I realized that the get_tz\n(get_tz_offset after this series) function used by Git::SVN didn't\nproperly handle DST boundary conditions.\n\nI realize that Eric Raymond is working to deprecate the current\niteration of git-cvsimport so this series may be only partially\nworthwhile. (If the cvsps 2 vs 3 issue does require a fallback\ngit-cvsimport script then maybe the whole series is still valid?) I'm\nnot attached to the current git-cvsimport so if the third patch isn't\nuseful then maybe the only the second patch is worthwhile (modified to\ncorrect the function in its current location).\n\nCurrently, the DST boundary functionality is exercised by the\ngit-cvsimport tests. If those go away as part of Eric's work then\nanother test that monitors this condition should be added. I can do\nthat as part of this series if it seems the right way to go.\n\nBen Walton (3):\n Move Git::SVN::get_tz to Git::get_tz_offset\n Allow Git::get_tz_offset to properly handle DST boundary times\n Avoid non-portable strftime format specifiers in git-cvsimport\n\n git-cvsimport.perl | 5 ++++-\n perl/Git.pm | 43 +++++++++++++++++++++++++++++++++++++++++++\n perl/Git/SVN.pm | 12 ++----------\n perl/Git/SVN/Log.pm | 8 ++++++--\n 4 files changed, 55 insertions(+), 13 deletions(-)\n\n-- \n1.7.10.4\n"},{"id":"206985","messageId":"1358291405-10173-2-git-send-email-bdwalton@gmail.com","threadId":"32641","inReplyTo":"1358291405-10173-1-git-send-email-bdwalton@gmail.com","subject":"[PATCH 1/3] Move Git::SVN::get_tz to Git::get_tz_offset","fromName":"Ben Walton","fromEmail":"bdwalton@gmail.com","sentAt":"2013-01-15T23:10:03Z","receivedAt":"2013-01-15T23:10:03Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"This function has utility outside of the SVN module for any routine\nthat needs the equivalent of GNU strftime's %z formatting option.\nMove it to the top-level Git.pm so that non-SVN modules don't need to\nimport the SVN module to use it.\n\nThe rename makes the purpose of the function clearer.\n\nSigned-off-by: Ben Walton \n---\n perl/Git.pm | 23 +++++++++++++++++++++++\n perl/Git/SVN.pm | 12 ++----------\n perl/Git/SVN/Log.pm | 8 ++++++--\n 3 files changed, 31 insertions(+), 12 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 931047c..5649bcc 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -59,6 +59,7 @@ require Exporter;\n command_bidi_pipe command_close_bidi_pipe\n version exec_path html_path hash_object git_cmd_try\n remote_refs prompt\n+ get_tz_offset\n temp_acquire temp_release temp_reset temp_path);\n \n \n@@ -102,6 +103,7 @@ use Error qw(:try);\n use Cwd qw(abs_path cwd);\n use IPC::Open2 qw(open2);\n use Fcntl qw(SEEK_SET SEEK_CUR);\n+use Time::Local qw(timelocal);\n }\n \n \n@@ -511,6 +513,27 @@ C). Useful mostly only internally.\n \n sub html_path { command_oneline('--html-path') }\n \n+\n+=item get_tz_offset ( TIME )\n+\n+Return the time zone offset from GMT in the form +/-HHMM where HH is\n+the number of hours from GMT and MM is the number of minutes. This is\n+the equivalent of what strftime(\"%z\", ...) would provide on a GNU\n+platform.\n+\n+If TIME is not supplied, the current local time is used.\n+\n+=cut\n+\n+sub get_tz_offset {\n+\t# some systmes don't handle or mishandle %z, so be creative.\n+\tmy $t = shift || time;\n+\tmy $gm = timelocal(gmtime($t));\n+\tmy $sign = qw( + + - )[ $t <=> $gm ];\n+\treturn sprintf(\"%s%02d%02d\", $sign, (gmtime(abs($t - $gm)))[2,1]);\n+}\n+\n+\n =item prompt ( PROMPT , ISPASSWORD )\n \n Query user C and return answer from user.\ndiff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm\nindex 59215fa..8c84560 100644\n--- a/perl/Git/SVN.pm\n+++ b/perl/Git/SVN.pm\n@@ -11,7 +11,6 @@ use Carp qw/croak/;\n use File::Path qw/mkpath/;\n use File::Copy qw/copy/;\n use IPC::Open3;\n-use Time::Local;\n use Memoize; # core since 5.8.0, Jul 2002\n use Memoize::Storable;\n use POSIX qw(:signal_h);\n@@ -22,6 +21,7 @@ use Git qw(\n command_noisy\n command_output_pipe\n command_close_pipe\n+ get_tz_offset\n );\n use Git::SVN::Utils qw(\n \tfatal\n@@ -1311,14 +1311,6 @@ sub get_untracked {\n \t\\@out;\n }\n \n-sub get_tz {\n-\t# some systmes don't handle or mishandle %z, so be creative.\n-\tmy $t = shift || time;\n-\tmy $gm = timelocal(gmtime($t));\n-\tmy $sign = qw( + + - )[ $t <=> $gm ];\n-\treturn sprintf(\"%s%02d%02d\", $sign, (gmtime(abs($t - $gm)))[2,1]);\n-}\n-\n # parse_svn_date(DATE)\n # --------------------\n # Given a date (in UTC) from Subversion, return a string in the format\n@@ -1351,7 +1343,7 @@ sub parse_svn_date {\n \t\t\tdelete $ENV{TZ};\n \t\t}\n \n-\t\tmy $our_TZ = get_tz();\n+\t\tmy $our_TZ = get_tz_offset();\n \n \t\t# This converts $epoch_in_UTC into our local timezone.\n \t\tmy ($sec, $min, $hour, $mday, $mon, $year,\ndiff --git a/perl/Git/SVN/Log.pm b/perl/Git/SVN/Log.pm\nindex 3cc1c6f..3f8350a 100644\n--- a/perl/Git/SVN/Log.pm\n+++ b/perl/Git/SVN/Log.pm\n@@ -2,7 +2,11 @@ package Git::SVN::Log;\n use strict;\n use warnings;\n use Git::SVN::Utils qw(fatal);\n-use Git qw(command command_oneline command_output_pipe command_close_pipe);\n+use Git qw(command\n+ command_oneline\n+ command_output_pipe\n+ command_close_pipe\n+ get_tz_offset);\n use POSIX qw/strftime/;\n use constant commit_log_separator => ('-' x 72) . \"\\n\";\n use vars qw/$TZ $limit $color $pager $non_recursive $verbose $oneline\n@@ -119,7 +123,7 @@ sub run_pager {\n sub format_svn_date {\n \tmy $t = shift || time;\n \trequire Git::SVN;\n-\tmy $gmoff = Git::SVN::get_tz($t);\n+\tmy $gmoff = get_tz_offset($t);\n \treturn strftime(\"%Y-%m-%d %H:%M:%S $gmoff (%a, %d %b %Y)\", localtime($t));\n }\n \n-- \n1.7.10.4\n"},{"id":"206988","messageId":"1358291405-10173-3-git-send-email-bdwalton@gmail.com","threadId":"32641","inReplyTo":"1358291405-10173-1-git-send-email-bdwalton@gmail.com","subject":"[PATCH 2/3] Allow Git::get_tz_offset to properly handle DST boundary times","fromName":"Ben Walton","fromEmail":"bdwalton@gmail.com","sentAt":"2013-01-15T23:10:04Z","receivedAt":"2013-01-15T23:10:04Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"The Git::get_tz_offset is meant to provide a workalike replacement for\nthe GNU strftime %z format specifier. The algorithm used failed to\nproperly handle DST boundary cases.\n\nFor example, the unix time 1162105199 in CST6CDT saw set_tz_offset\nimproperly return -0600 instead of -0500.\n\nTZ=CST6CDT date -d @1162105199 +\"%c %z\"\nSun 29 Oct 2006 01:59:59 AM CDT -0500\n\n$ zdump -v /usr/share/zoneinfo/CST6CDT | grep 2006\n/usr/share/zoneinfo/CST6CDT Sun Apr 2 07:59:59 2006 UTC = Sun Apr 2\n01:59:59 2006 CST isdst=0 gmtoff=-21600\n/usr/share/zoneinfo/CST6CDT Sun Apr 2 08:00:00 2006 UTC = Sun Apr 2\n03:00:00 2006 CDT isdst=1 gmtoff=-18000\n/usr/share/zoneinfo/CST6CDT Sun Oct 29 06:59:59 2006 UTC = Sun Oct 29\n01:59:59 2006 CDT isdst=1 gmtoff=-18000\n/usr/share/zoneinfo/CST6CDT Sun Oct 29 07:00:00 2006 UTC = Sun Oct 29\n01:00:00 2006 CST isdst=0 gmtoff=-21600\n\nTo determine how many hours/minutes away from GMT a particular time\nwas, we calculated the gmtime() of the requested time value and then\nused Time::Local's timelocal() function to turn the GMT-based time\nback into a scalar value representing seconds from the epoch. Because\nGMT has no daylight savings time, timelocal() cannot handle the\nambiguous times that occur at DST boundaries since there are two\npossible correct results.\n\nTo work around the ambiguity at these boundaries, we must take into\naccount the pre and post conversion values for is_dst as provided by\nboth the original time value and the value that has been run through\ntimelocal(). If the is_dst field of the two times disagree then we\nmust modify the value returned from timelocal() by an hour in the\ncorrect direction.\n\nSigned-off-by: Ben Walton \n---\n perl/Git.pm | 20 ++++++++++++++++++++\n 1 file changed, 20 insertions(+)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 5649bcc..788b9b4 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -528,7 +528,27 @@ If TIME is not supplied, the current local time is used.\n sub get_tz_offset {\n \t# some systmes don't handle or mishandle %z, so be creative.\n \tmy $t = shift || time;\n+\t# timelocal() has a problem when it comes to DST ambiguity so\n+\t# times that are on a DST boundary cannot be properly converted\n+\t# using it. we will possibly adjust its result depending on whehter\n+\t# pre and post conversions agree on DST\n \tmy $gm = timelocal(gmtime($t));\n+\n+\t# we need to know whether we were originally in DST or not\n+\tmy $orig_dst = (localtime($t))[8];\n+\t# and also whether timelocal thinks we're in DST\n+\tmy $conv_dst = (localtime($gm))[8];\n+\n+\t# re-adjust $gm based on the DST value for the two times we're\n+\t# handling.\n+\tif ($orig_dst != $conv_dst) {\n+\t\tif ($orig_dst == 1) {\n+\t\t\t$gm -= 3600;\n+\t\t} else {\n+\t\t\t$gm += 3600;\n+\t\t}\n+\t}\n+\n \tmy $sign = qw( + + - )[ $t <=> $gm ];\n \treturn sprintf(\"%s%02d%02d\", $sign, (gmtime(abs($t - $gm)))[2,1]);\n }\n-- \n1.7.10.4\n"},{"id":"206987","messageId":"1358291405-10173-4-git-send-email-bdwalton@gmail.com","threadId":"32641","inReplyTo":"1358291405-10173-1-git-send-email-bdwalton@gmail.com","subject":"[PATCH 3/3] Avoid non-portable strftime format specifiers in git-cvsimport","fromName":"Ben Walton","fromEmail":"bdwalton@gmail.com","sentAt":"2013-01-15T23:10:05Z","receivedAt":"2013-01-15T23:10:05Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"Neither %s or %z are portable strftime format specifiers. There is no\nneed for %s in git-cvsimport as the supplied time is already in\nseconds since the epoch. For %z, use the function get_tz_offset\nprovided by Git.pm instead.\n\nSigned-off-by: Ben Walton \n---\n git-cvsimport.perl | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/git-cvsimport.perl b/git-cvsimport.perl\nindex 0a31ebd..344f120 100755\n--- a/git-cvsimport.perl\n+++ b/git-cvsimport.perl\n@@ -26,6 +26,7 @@ use IO::Socket;\n use IO::Pipe;\n use POSIX qw(strftime tzset dup2 ENOENT);\n use IPC::Open2;\n+use Git qw(get_tz_offset);\n \n $SIG{'PIPE'}=\"IGNORE\";\n set_timezone('UTC');\n@@ -864,7 +865,9 @@ sub commit {\n \t}\n \n \tset_timezone($author_tz);\n-\tmy $commit_date = strftime(\"%s %z\", localtime($date));\n+\t# $date is in the seconds since epoch format\n+\tmy $tz_offset = get_tz_offset($date);\n+\tmy $commit_date = \"$date $tz_offset\";\n \tset_timezone('UTC');\n \t$ENV{GIT_AUTHOR_NAME} = $author_name;\n \t$ENV{GIT_AUTHOR_EMAIL} = $author_email;\n-- \n1.7.10.4\n"},{"id":"206991","messageId":"7vobgq107t.fsf@alter.siamese.dyndns.org","threadId":"32641","inReplyTo":"1358291405-10173-1-git-send-email-bdwalton@gmail.com","subject":"Re: [PATCH 0/3] Fix a portability issue with git-cvsimport","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-15T23:43:34Z","receivedAt":"2013-01-15T23:43:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Walton writes:\n\n> This patch series started as a quick fix for the use of %s and %z in\n> git-cvsimport but grew slightly when I realized that the get_tz\n> (get_tz_offset after this series) function used by Git::SVN didn't\n> properly handle DST boundary conditions.\n>\n> I realize that Eric Raymond is working to deprecate the current\n> iteration of git-cvsimport so this series may be only partially\n> worthwhile. (If the cvsps 2 vs 3 issue does require a fallback\n> git-cvsimport script then maybe the whole series is still valid?)\n\nThere is my reroll of Eric's patch [*1*], that is in 'pu'. The topic\nends at 12b3541 (t9600: adjust for new cvsimport, 2013-01-13).\n\nI think the folks on the traditional Git side prefer the approach\ntaken by it to keep the old one under cvsimport-2 while adding\nEric's as cvsimport-3 and have a separate version switcher wrapper\n[*2*, *3*]. Also Chris Rorvick, a contributor to cvsps-3 & new\ncvsimport combo, who already has patches to Eric's version, agrees\nthat it is a foundation we can build on together [*4*].\n\nEric hasn't spoken on the topic yet, but I think what the rest of us\nagreed may be a reasonable starting point.\n\nI think I can apply your patches on top of 12b3541 with \"am -3\" and\nhave it automatically update git-cvsimport-2.perl ;-)\n\n\n[References]\n\n*1* http://thread.gmane.org/gmane.comp.version-control.git/213170/focus=213460\n*2* http://thread.gmane.org/gmane.comp.version-control.git/213170/focus=213432\n*3* http://thread.gmane.org/gmane.comp.version-control.git/213170/focus=213466\n*4* http://thread.gmane.org/gmane.comp.version-control.git/213537/focus=213595\n"},{"id":"207002","messageId":"CAEUsAPZakGKUmQWrsTaF1cpbQm0Y4C3sDxCWD_i1gkQxeC-bRQ@mail.gmail.com","threadId":"32641","inReplyTo":"1358291405-10173-4-git-send-email-bdwalton@gmail.com","subject":"Re: [PATCH 3/3] Avoid non-portable strftime format specifiers in git-cvsimport","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2013-01-16T01:53:37Z","receivedAt":"2013-01-16T01:53:37Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"On Tue, Jan 15, 2013 at 5:10 PM, Ben Walton wrote:\n> Neither %s or %z are portable strftime format specifiers. There is no\n> need for %s in git-cvsimport as the supplied time is already in\n> seconds since the epoch. For %z, use the function get_tz_offset\n> provided by Git.pm instead.\n\nOut of curiosity, which platforms are affected? Assuming DST is a 1\nhour shift (patch 2/3) is not necessarily portable either, though this\ncurrently appears to only affect a small island off of the coast of\nAustralia. :-)\n\nChris\n"},{"id":"207044","messageId":"CAP30j153s970=2WKqxWTVGRAaJ9jEXg9ETF8OFU=-nDK=BAxfg@mail.gmail.com","threadId":"32641","inReplyTo":"CAEUsAPZakGKUmQWrsTaF1cpbQm0Y4C3sDxCWD_i1gkQxeC-bRQ@mail.gmail.com","subject":"Re: [PATCH 3/3] Avoid non-portable strftime format specifiers in git-cvsimport","fromName":"Ben Walton","fromEmail":"bdwalton@gmail.com","sentAt":"2013-01-16T10:38:09Z","receivedAt":"2013-01-16T10:38:09Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"On Wed, Jan 16, 2013 at 1:53 AM, Chris Rorvick wrote:\n> On Tue, Jan 15, 2013 at 5:10 PM, Ben Walton wrote:\n>> Neither %s or %z are portable strftime format specifiers. There is no\n>> need for %s in git-cvsimport as the supplied time is already in\n>> seconds since the epoch. For %z, use the function get_tz_offset\n>> provided by Git.pm instead.\n>\n> Out of curiosity, which platforms are affected? Assuming DST is a 1\n> hour shift (patch 2/3) is not necessarily portable either, though this\n> currently appears to only affect a small island off of the coast of\n> Australia. :-)\n\nMy primary motivation on this change was for solaris. %s isn't\nsupported in 10 (not sure about 11) and %z was only added in 10. The\nissue affects other older platforms as well.\n\nGood point about the 1 hour assumption. Is it worth hacking in\nadditional logic to handle Lord Howe Island? I think it's likely a\ncase of \"in for a penny, in for a pound\" but that could lead to\nmadness when it comes to time zones. Either way, the function behaves\nbetter now than before.\n\n(I wasn't aware of the half hour oddball wrt to DST, so I learned\nsomething new here too!)\n\nThanks\n-Ben\n--\n---------------------------------------------------------------------------------------------------------------------------\nTake the risk of thinking for yourself. Much more happiness,\ntruth, beauty and wisdom will come to you that way.\n\n-Christopher Hitchens\n---------------------------------------------------------------------------------------------------------------------------\n"},{"id":"207057","messageId":"7vehhlyw90.fsf@alter.siamese.dyndns.org","threadId":"32641","inReplyTo":"1358291405-10173-2-git-send-email-bdwalton@gmail.com","subject":"Re: [PATCH 1/3] Move Git::SVN::get_tz to Git::get_tz_offset","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-16T15:37:31Z","receivedAt":"2013-01-16T15:37:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Walton writes:\n\n> +sub get_tz_offset {\n> +\t# some systmes don't handle or mishandle %z, so be creative.\n\nHmph. I wonder if we can use %z if it is handled correctly and fall\nback to this code only on platforms that are broken?\n"},{"id":"207103","messageId":"CAP30j164UD9gNRbZ=uCQjgpDODWnGtYmHcWES2P=YPryL=FbZA@mail.gmail.com","threadId":"32641","inReplyTo":"7vehhlyw90.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/3] Move Git::SVN::get_tz to Git::get_tz_offset","fromName":"Ben Walton","fromEmail":"bdwalton@gmail.com","sentAt":"2013-01-16T20:16:55Z","receivedAt":"2013-01-16T20:16:55Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"On Wed, Jan 16, 2013 at 3:37 PM, Junio C Hamano wrote:\n> Ben Walton writes:\n>\n>> +sub get_tz_offset {\n>> + # some systmes don't handle or mishandle %z, so be creative.\n>\n> Hmph. I wonder if we can use %z if it is handled correctly and fall\n> back to this code only on platforms that are broken?\n\nThat would be perfectly acceptable to me. The reason I set it up to\nalways run through this function here is that when I originally added\nthis function for git-svn, I'd made it conditional and Eric Wong\npreferred that the function be used exclusively[1]. I opted to take\nthe same approach here to keep things congrous.\n\nIf it were to be conditional, I think I'd add a variable to the build\nsystem and have the code leverage that at runtime instead of the\ntry/except approach I attempted in 2009.\n\nThanks\n-Ben\n\n[1] http://lists-archives.com/git/683572-git-svn-fix-for-systems-without-strftime-z.html\n--\n---------------------------------------------------------------------------------------------------------------------------\nTake the risk of thinking for yourself. Much more happiness,\ntruth, beauty and wisdom will come to you that way.\n\n-Christopher Hitchens\n---------------------------------------------------------------------------------------------------------------------------\n"},{"id":"207105","messageId":"7vehhkx3ul.fsf@alter.siamese.dyndns.org","threadId":"32641","inReplyTo":"CAP30j164UD9gNRbZ=uCQjgpDODWnGtYmHcWES2P=YPryL=FbZA@mail.gmail.com","subject":"Re: [PATCH 1/3] Move Git::SVN::get_tz to Git::get_tz_offset","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-16T20:36:18Z","receivedAt":"2013-01-16T20:36:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Walton writes:\n\n> On Wed, Jan 16, 2013 at 3:37 PM, Junio C Hamano wrote:\n>> Ben Walton writes:\n>>\n>>> +sub get_tz_offset {\n>>> + # some systmes don't handle or mishandle %z, so be creative.\n>>\n>> Hmph. I wonder if we can use %z if it is handled correctly and fall\n>> back to this code only on platforms that are broken?\n>\n> That would be perfectly acceptable to me. The reason I set it up to\n> always run through this function here is that when I originally added\n> this function for git-svn, I'd made it conditional and Eric Wong\n> preferred that the function be used exclusively[1]. I opted to take\n> the same approach here to keep things congrous.\n>\n> If it were to be conditional, I think I'd add a variable to the build\n> system and have the code leverage that at runtime instead of the\n> try/except approach I attempted in 2009.\n\nIf the code was originally unconditional for a reason (and I think\nbeing bug-to-bug compatible across platforms is actually a good\nthing in a tool like importers), I would not object to it. Thanks\nfor the back-story.\n"},{"id":"207163","messageId":"7vy5frtymt.fsf@alter.siamese.dyndns.org","threadId":"32641","inReplyTo":"1358291405-10173-3-git-send-email-bdwalton@gmail.com","subject":"Re: [PATCH 2/3] Allow Git::get_tz_offset to properly handle DST boundary times","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-17T19:09:30Z","receivedAt":"2013-01-17T19:09:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Walton writes:\n\n> The Git::get_tz_offset is meant to provide a workalike replacement for\n> the GNU strftime %z format specifier. The algorithm used failed to\n> properly handle DST boundary cases.\n>\n> For example, the unix time 1162105199 in CST6CDT saw set_tz_offset\n> improperly return -0600 instead of -0500.\n>\n> TZ=CST6CDT date -d @1162105199 +\"%c %z\"\n> Sun 29 Oct 2006 01:59:59 AM CDT -0500\n>\n> $ zdump -v /usr/share/zoneinfo/CST6CDT | grep 2006\n> /usr/share/zoneinfo/CST6CDT Sun Apr 2 07:59:59 2006 UTC = Sun Apr 2\n> 01:59:59 2006 CST isdst=0 gmtoff=-21600\n> /usr/share/zoneinfo/CST6CDT Sun Apr 2 08:00:00 2006 UTC = Sun Apr 2\n> 03:00:00 2006 CDT isdst=1 gmtoff=-18000\n> /usr/share/zoneinfo/CST6CDT Sun Oct 29 06:59:59 2006 UTC = Sun Oct 29\n> 01:59:59 2006 CDT isdst=1 gmtoff=-18000\n> /usr/share/zoneinfo/CST6CDT Sun Oct 29 07:00:00 2006 UTC = Sun Oct 29\n> 01:00:00 2006 CST isdst=0 gmtoff=-21600\n>\n> To determine how many hours/minutes away from GMT a particular time\n> was, we calculated the gmtime() of the requested time value and then\n> used Time::Local's timelocal() function to turn the GMT-based time\n> back into a scalar value representing seconds from the epoch. Because\n> GMT has no daylight savings time, timelocal() cannot handle the\n> ambiguous times that occur at DST boundaries since there are two\n> possible correct results.\n>\n> To work around the ambiguity at these boundaries, we must take into\n> account the pre and post conversion values for is_dst as provided by\n> both the original time value and the value that has been run through\n> timelocal(). If the is_dst field of the two times disagree then we\n> must modify the value returned from timelocal() by an hour in the\n> correct direction.\n\nIt seems to me that it is a very roundabout way. It may be correct,\nbut it is unclear why the magic +/-3600 shift is the right solution\nand I suspect even you wouldn't notice if I sent you back your patch\nwith a slight change to swap $gm += 3600 and $gm -= 3600 lines ;-).\n\nFor that timestamp in question, the human-readable representation of\ngmtime($t) and localtime($t) look like these two strings:\n\n\tmy $t = 1162105199;\n\tprint gmtime($t), \"\\n\"; # Sun Oct 29 06:59:59 2006\n print localtime($t), \"\\n\"; # Sun Oct 29 01:59:59 2006\n\nAs a human, you can easily see that these two stringified timestamps\nlook 5 hours apart. Think how you managed to do so.\n\nIf we convert these back to the seconds-since-epoch, as if these\nbroken-down times were both in a single timezone that does not have\nany DST issues, you can get the offset (in seconds) by subtraction,\nand that is essentially the same as the way in which your eyes saw\nthey are 5 hours apart, no? In other words, why do you need to run\ntimelocal() at all?\n\n\tmy $t = 1162105199;\n my $lct = timegm(localtime($t)); \n # of course, timegm(gmtime($t)) == $t\n\n\tmy $minutes = int(($lct - $t)/60);\n my $sign \"+\";\n if ($minutes < 0) {\n\t\t$sign = \"-\";\n $minutes = -$minutes;\n\t}\n my $hours = int($minutes/60);\n $minutes -= $hours * 60;\n sprintf(\"%s%02d%02d\", $sign, $hours, $minutes);\n\nConfused...\n\n>\n> Signed-off-by: Ben Walton \n> ---\n> perl/Git.pm | 20 ++++++++++++++++++++\n> 1 file changed, 20 insertions(+)\n>\n> diff --git a/perl/Git.pm b/perl/Git.pm\n> index 5649bcc..788b9b4 100644\n> --- a/perl/Git.pm\n> +++ b/perl/Git.pm\n> @@ -528,7 +528,27 @@ If TIME is not supplied, the current local time is used.\n> sub get_tz_offset {\n> \t# some systmes don't handle or mishandle %z, so be creative.\n> \tmy $t = shift || time;\n> +\t# timelocal() has a problem when it comes to DST ambiguity so\n> +\t# times that are on a DST boundary cannot be properly converted\n> +\t# using it. we will possibly adjust its result depending on whehter\n> +\t# pre and post conversions agree on DST\n> \tmy $gm = timelocal(gmtime($t));\n> +\n> +\t# we need to know whether we were originally in DST or not\n> +\tmy $orig_dst = (localtime($t))[8];\n> +\t# and also whether timelocal thinks we're in DST\n> +\tmy $conv_dst = (localtime($gm))[8];\n> +\n> +\t# re-adjust $gm based on the DST value for the two times we're\n> +\t# handling.\n> +\tif ($orig_dst != $conv_dst) {\n> +\t\tif ($orig_dst == 1) {\n> +\t\t\t$gm -= 3600;\n> +\t\t} else {\n> +\t\t\t$gm += 3600;\n> +\t\t}\n> +\t}\n> +\n> \tmy $sign = qw( + + - )[ $t <=> $gm ];\n> \treturn sprintf(\"%s%02d%02d\", $sign, (gmtime(abs($t - $gm)))[2,1]);\n> }\n"},{"id":"207327","messageId":"CAP30j14Og7YLaZj0dbpAhUHFfuy0Y=bEn_3EqGzxR5PRA7vQXA@mail.gmail.com","threadId":"32641","inReplyTo":"7vy5frtymt.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] Allow Git::get_tz_offset to properly handle DST boundary times","fromName":"Ben Walton","fromEmail":"bdwalton@gmail.com","sentAt":"2013-01-20T20:06:13Z","receivedAt":"2013-01-20T20:06:13Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"On Thu, Jan 17, 2013 at 7:09 PM, Junio C Hamano wrote:\n> Ben Walton writes:\n>\n>> The Git::get_tz_offset is meant to provide a workalike replacement for\n>> the GNU strftime %z format specifier. The algorithm used failed to\n>> properly handle DST boundary cases.\n>>\n>> For example, the unix time 1162105199 in CST6CDT saw set_tz_offset\n>> improperly return -0600 instead of -0500.\n>>\n>> TZ=CST6CDT date -d @1162105199 +\"%c %z\"\n>> Sun 29 Oct 2006 01:59:59 AM CDT -0500\n>>\n>> $ zdump -v /usr/share/zoneinfo/CST6CDT | grep 2006\n>> /usr/share/zoneinfo/CST6CDT Sun Apr 2 07:59:59 2006 UTC = Sun Apr 2\n>> 01:59:59 2006 CST isdst=0 gmtoff=-21600\n>> /usr/share/zoneinfo/CST6CDT Sun Apr 2 08:00:00 2006 UTC = Sun Apr 2\n>> 03:00:00 2006 CDT isdst=1 gmtoff=-18000\n>> /usr/share/zoneinfo/CST6CDT Sun Oct 29 06:59:59 2006 UTC = Sun Oct 29\n>> 01:59:59 2006 CDT isdst=1 gmtoff=-18000\n>> /usr/share/zoneinfo/CST6CDT Sun Oct 29 07:00:00 2006 UTC = Sun Oct 29\n>> 01:00:00 2006 CST isdst=0 gmtoff=-21600\n>>\n>> To determine how many hours/minutes away from GMT a particular time\n>> was, we calculated the gmtime() of the requested time value and then\n>> used Time::Local's timelocal() function to turn the GMT-based time\n>> back into a scalar value representing seconds from the epoch. Because\n>> GMT has no daylight savings time, timelocal() cannot handle the\n>> ambiguous times that occur at DST boundaries since there are two\n>> possible correct results.\n>>\n>> To work around the ambiguity at these boundaries, we must take into\n>> account the pre and post conversion values for is_dst as provided by\n>> both the original time value and the value that has been run through\n>> timelocal(). If the is_dst field of the two times disagree then we\n>> must modify the value returned from timelocal() by an hour in the\n>> correct direction.\n>\n> It seems to me that it is a very roundabout way. It may be correct,\n> but it is unclear why the magic +/-3600 shift is the right solution\n> and I suspect even you wouldn't notice if I sent you back your patch\n> with a slight change to swap $gm += 3600 and $gm -= 3600 lines ;-).\n>\n> For that timestamp in question, the human-readable representation of\n> gmtime($t) and localtime($t) look like these two strings:\n>\n> my $t = 1162105199;\n> print gmtime($t), \"\\n\"; # Sun Oct 29 06:59:59 2006\n> print localtime($t), \"\\n\"; # Sun Oct 29 01:59:59 2006\n>\n> As a human, you can easily see that these two stringified timestamps\n> look 5 hours apart. Think how you managed to do so.\n>\n> If we convert these back to the seconds-since-epoch, as if these\n> broken-down times were both in a single timezone that does not have\n> any DST issues, you can get the offset (in seconds) by subtraction,\n> and that is essentially the same as the way in which your eyes saw\n> they are 5 hours apart, no? In other words, why do you need to run\n> timelocal() at all?\n>\n> my $t = 1162105199;\n> my $lct = timegm(localtime($t));\n> # of course, timegm(gmtime($t)) == $t\n>\n> my $minutes = int(($lct - $t)/60);\n> my $sign \"+\";\n> if ($minutes < 0) {\n> $sign = \"-\";\n> $minutes = -$minutes;\n> }\n> my $hours = int($minutes/60);\n> $minutes -= $hours * 60;\n> sprintf(\"%s%02d%02d\", $sign, $hours, $minutes);\n>\n> Confused...\n>\n>>\n>> Signed-off-by: Ben Walton \n>> ---\n>> perl/Git.pm | 20 ++++++++++++++++++++\n>> 1 file changed, 20 insertions(+)\n>>\n>> diff --git a/perl/Git.pm b/perl/Git.pm\n>> index 5649bcc..788b9b4 100644\n>> --- a/perl/Git.pm\n>> +++ b/perl/Git.pm\n>> @@ -528,7 +528,27 @@ If TIME is not supplied, the current local time is used.\n>> sub get_tz_offset {\n>> # some systmes don't handle or mishandle %z, so be creative.\n>> my $t = shift || time;\n>> + # timelocal() has a problem when it comes to DST ambiguity so\n>> + # times that are on a DST boundary cannot be properly converted\n>> + # using it. we will possibly adjust its result depending on whehter\n>> + # pre and post conversions agree on DST\n>> my $gm = timelocal(gmtime($t));\n>> +\n>> + # we need to know whether we were originally in DST or not\n>> + my $orig_dst = (localtime($t))[8];\n>> + # and also whether timelocal thinks we're in DST\n>> + my $conv_dst = (localtime($gm))[8];\n>> +\n>> + # re-adjust $gm based on the DST value for the two times we're\n>> + # handling.\n>> + if ($orig_dst != $conv_dst) {\n>> + if ($orig_dst == 1) {\n>> + $gm -= 3600;\n>> + } else {\n>> + $gm += 3600;\n>> + }\n>> + }\n>> +\n>> my $sign = qw( + + - )[ $t <=> $gm ];\n>> return sprintf(\"%s%02d%02d\", $sign, (gmtime(abs($t - $gm)))[2,1]);\n>> }\n\n\nSorry for the slow response, I didn't have a good chance to look at\nthis until now. You are correct; your solution appears simpler and\nalso avoids the oddball 1/2 hour DST shift. I can re-roll the series\nwith your code (and credit for it) or you can apply you change on top\nof my series...whichever is easiest for you.\n\nThanks\n-Ben\n--\n---------------------------------------------------------------------------------------------------------------------------\nTake the risk of thinking for yourself. Much more happiness,\ntruth, beauty and wisdom will come to you that way.\n\n-Christopher Hitchens\n---------------------------------------------------------------------------------------------------------------------------\n"},{"id":"207334","messageId":"7vzk03k1mt.fsf@alter.siamese.dyndns.org","threadId":"32641","inReplyTo":"CAP30j14Og7YLaZj0dbpAhUHFfuy0Y=bEn_3EqGzxR5PRA7vQXA@mail.gmail.com","subject":"Re: [PATCH 2/3] Allow Git::get_tz_offset to properly handle DST boundary times","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-20T21:03:54Z","receivedAt":"2013-01-20T21:03:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Walton writes:\n\n> also avoids the oddball 1/2 hour DST shift. I can re-roll the series\n> with your code (and credit for it) or you can apply you change on top\n> of my series...whichever is easiest for you.\n\nPlease reroll, as I do not have patience either to set up a test\ncase and verify the end result is correct, or to come up with a test\ncase for it. For this particular case, I think the identification\nof the issue weighs more than the implementation for fix it, so\nplease retain the authorship for the fix; mentioning \"taking the\nimplementation idea from Junio\" in the log message is the right\namount of credit due to me.\n\nThanks.\n"}]}