{"thread":{"id":"32871","subject":"[PATCH 3/3] Avoid non-portable strftime format specifiers in git-cvsimport","startedAt":"2013-02-09T21:46:55Z","lastAt":"2013-02-09T22:58:08Z","messageCount":6,"participants":["Ben Walton","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"209095","messageId":"1360446418-12280-1-git-send-email-bdwalton@gmail.com","threadId":"32871","inReplyTo":null,"subject":"[PATCH 0/3] Fix a portability issue with git-cvsimport","fromName":"Ben Walton","fromEmail":"bdwalton@gmail.com","sentAt":"2013-02-09T21:46:55Z","receivedAt":"2013-02-09T21:46:55Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"This is my (long overdue) re-roll of the series that fixes a\nportability issue with git-cvsimport's use of strftime.  It also fixes\na but in the original implementation of get_tz (now get_tz_offset).\n\nI ended up taking taking only part of the implementation suggested by\nJunio.\n\nThe only usage of get_tz_offset is by git-cvsimport and Git::SVN::Log\ncurrently.  There are tests that validate it works currently so I\ndidn't add anything additional.  If the git-cvsimport tests are\nremoved, there are no tests remaining that exercise the code full as\nthe SVN tests use UTC times.\n\nBen Walton (3):\n  Move Git::SVN::get_tz to Git::get_tz_offset\n  Fix get_tz_offset to properly handle DST boundary cases\n  Avoid non-portable strftime format specifiers in git-cvsimport\n\n git-cvsimport.perl  |    5 ++++-\n perl/Git.pm         |   23 +++++++++++++++++++++++\n perl/Git/SVN.pm     |   12 ++----------\n perl/Git/SVN/Log.pm |    8 ++++++--\n 4 files changed, 35 insertions(+), 13 deletions(-)\n\n-- \n1.7.10.4\n"},{"id":"209093","messageId":"1360446418-12280-2-git-send-email-bdwalton@gmail.com","threadId":"32871","inReplyTo":"1360446418-12280-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-02-09T21:46:56Z","receivedAt":"2013-02-09T21:46:56Z","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 <bdwalton@gmail.com>\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<git --html-path>). 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<PROMPT> and return answer from user.\ndiff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm\nindex 490e330..0ebc68a 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":"209094","messageId":"1360446418-12280-3-git-send-email-bdwalton@gmail.com","threadId":"32871","inReplyTo":"1360446418-12280-1-git-send-email-bdwalton@gmail.com","subject":"[PATCH 2/3] Fix get_tz_offset to properly handle DST boundary cases","fromName":"Ben Walton","fromEmail":"bdwalton@gmail.com","sentAt":"2013-02-09T21:46:57Z","receivedAt":"2013-02-09T21:46:57Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"When passed a local time that was on the boundary of a DST change,\nget_tz_offset returned a GMT offset that was incorrect (off by one\nhour).  This is because the time was converted to GMT and then back to\na time stamp via timelocal() which cannot disambiguate boundary cases\nas noted in its documentation.\n\nModify this algorithm, using an approach suggested by Junio C Hamano\nthat obtains the GMT time stamp by using timegm(localtime()) instead\nof timelocal(gmtime()).  This avoids the ambigious conversion and\nallows a correct time to be returned on every occassion.\n\nSigned-off-by: Ben Walton <bdwalton@gmail.com>\n---\n perl/Git.pm |    6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 5649bcc..a56d1e7 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -103,7 +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+use Time::Local qw(timegm);\n }\n \n \n@@ -528,8 +528,8 @@ 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-\tmy $gm = timelocal(gmtime($t));\n-\tmy $sign = qw( + + - )[ $t <=> $gm ];\n+\tmy $gm = timegm(localtime($t));\n+\tmy $sign = qw( + + - )[ $gm <=> $t ];\n \treturn sprintf(\"%s%02d%02d\", $sign, (gmtime(abs($t - $gm)))[2,1]);\n }\n \n-- \n1.7.10.4\n"},{"id":"209092","messageId":"1360446418-12280-4-git-send-email-bdwalton@gmail.com","threadId":"32871","inReplyTo":"1360446418-12280-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-02-09T21:46:58Z","receivedAt":"2013-02-09T21:46:58Z","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 <bdwalton@gmail.com>\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":"209097","messageId":"7vpq09uo2k.fsf@alter.siamese.dyndns.org","threadId":"32871","inReplyTo":"1360446418-12280-4-git-send-email-bdwalton@gmail.com","subject":"Re: [PATCH 3/3] Avoid non-portable strftime format specifiers in git-cvsimport","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-09T22:20:19Z","receivedAt":"2013-02-09T22:20:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Walton <bdwalton@gmail.com> writes:\n\n> Neither %s or %z are portable strftime format specifiers.\n\nWell, at least %z is in POSIX; \"Some implementations of strftime(3)\nlack support for %z format\" is fine, tough.\n\nThanks.\n"},{"id":"209098","messageId":"7vip61umbj.fsf@alter.siamese.dyndns.org","threadId":"32871","inReplyTo":"1360446418-12280-3-git-send-email-bdwalton@gmail.com","subject":"Re: [PATCH 2/3] Fix get_tz_offset to properly handle DST boundary cases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-09T22:58:08Z","receivedAt":"2013-02-09T22:58:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Walton <bdwalton@gmail.com> writes:\n\n> When passed a local time that was on the boundary of a DST change,\n> get_tz_offset returned a GMT offset that was incorrect (off by one\n> hour).  This is because the time was converted to GMT and then back to\n> a time stamp via timelocal() which cannot disambiguate boundary cases\n> as noted in its documentation.\n>\n> Modify this algorithm, using an approach suggested by Junio C Hamano\n> that obtains the GMT time stamp by using timegm(localtime()) instead\n> of timelocal(gmtime()).  This avoids the ambigious conversion and\n> allows a correct time to be returned on every occassion.\n\nI'll reword the log message a bit to explain why the updated logic\nis right and also refer to the message that has the suggestion.  o\n\nThe implemmentation is a bit dense to my taste, but looks correct (I\nhad to think about the comparison to come up with the value of the\n$sign, though).\n\nThanks.\n\n> Signed-off-by: Ben Walton <bdwalton@gmail.com>\n> ---\n>  perl/Git.pm |    6 +++---\n>  1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/perl/Git.pm b/perl/Git.pm\n> index 5649bcc..a56d1e7 100644\n> --- a/perl/Git.pm\n> +++ b/perl/Git.pm\n> @@ -103,7 +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> +use Time::Local qw(timegm);\n>  }\n>  \n>  \n> @@ -528,8 +528,8 @@ 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> -\tmy $gm = timelocal(gmtime($t));\n> -\tmy $sign = qw( + + - )[ $t <=> $gm ];\n> +\tmy $gm = timegm(localtime($t));\n> +\tmy $sign = qw( + + - )[ $gm <=> $t ];\n>  \treturn sprintf(\"%s%02d%02d\", $sign, (gmtime(abs($t - $gm)))[2,1]);\n>  }\n"}]}