* [PATCH 0/3] Fix a portability issue with git-cvsimport
@ 2013-02-09 21:46 Ben Walton
2013-02-09 21:46 ` [PATCH 1/3] Move Git::SVN::get_tz to Git::get_tz_offset Ben Walton
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Ben Walton @ 2013-02-09 21:46 UTC (permalink / raw)
To: gitster; +Cc: git, Ben Walton
This is my (long overdue) re-roll of the series that fixes a
portability issue with git-cvsimport's use of strftime. It also fixes
a but in the original implementation of get_tz (now get_tz_offset).
I ended up taking taking only part of the implementation suggested by
Junio.
The only usage of get_tz_offset is by git-cvsimport and Git::SVN::Log
currently. There are tests that validate it works currently so I
didn't add anything additional. If the git-cvsimport tests are
removed, there are no tests remaining that exercise the code full as
the SVN tests use UTC times.
Ben Walton (3):
Move Git::SVN::get_tz to Git::get_tz_offset
Fix get_tz_offset to properly handle DST boundary cases
Avoid non-portable strftime format specifiers in git-cvsimport
git-cvsimport.perl | 5 ++++-
perl/Git.pm | 23 +++++++++++++++++++++++
perl/Git/SVN.pm | 12 ++----------
perl/Git/SVN/Log.pm | 8 ++++++--
4 files changed, 35 insertions(+), 13 deletions(-)
--
1.7.10.4
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/3] Move Git::SVN::get_tz to Git::get_tz_offset 2013-02-09 21:46 [PATCH 0/3] Fix a portability issue with git-cvsimport Ben Walton @ 2013-02-09 21:46 ` Ben Walton 2013-02-09 21:46 ` [PATCH 2/3] Fix get_tz_offset to properly handle DST boundary cases Ben Walton 2013-02-09 21:46 ` [PATCH 3/3] Avoid non-portable strftime format specifiers in git-cvsimport Ben Walton 2 siblings, 0 replies; 6+ messages in thread From: Ben Walton @ 2013-02-09 21:46 UTC (permalink / raw) To: gitster; +Cc: git, Ben Walton This function has utility outside of the SVN module for any routine that needs the equivalent of GNU strftime's %z formatting option. Move it to the top-level Git.pm so that non-SVN modules don't need to import the SVN module to use it. The rename makes the purpose of the function clearer. Signed-off-by: Ben Walton <bdwalton@gmail.com> --- perl/Git.pm | 23 +++++++++++++++++++++++ perl/Git/SVN.pm | 12 ++---------- perl/Git/SVN/Log.pm | 8 ++++++-- 3 files changed, 31 insertions(+), 12 deletions(-) diff --git a/perl/Git.pm b/perl/Git.pm index 931047c..5649bcc 100644 --- a/perl/Git.pm +++ b/perl/Git.pm @@ -59,6 +59,7 @@ require Exporter; command_bidi_pipe command_close_bidi_pipe version exec_path html_path hash_object git_cmd_try remote_refs prompt + get_tz_offset temp_acquire temp_release temp_reset temp_path); @@ -102,6 +103,7 @@ use Error qw(:try); use Cwd qw(abs_path cwd); use IPC::Open2 qw(open2); use Fcntl qw(SEEK_SET SEEK_CUR); +use Time::Local qw(timelocal); } @@ -511,6 +513,27 @@ C<git --html-path>). Useful mostly only internally. sub html_path { command_oneline('--html-path') } + +=item get_tz_offset ( TIME ) + +Return the time zone offset from GMT in the form +/-HHMM where HH is +the number of hours from GMT and MM is the number of minutes. This is +the equivalent of what strftime("%z", ...) would provide on a GNU +platform. + +If TIME is not supplied, the current local time is used. + +=cut + +sub get_tz_offset { + # some systmes don't handle or mishandle %z, so be creative. + my $t = shift || time; + my $gm = timelocal(gmtime($t)); + my $sign = qw( + + - )[ $t <=> $gm ]; + return sprintf("%s%02d%02d", $sign, (gmtime(abs($t - $gm)))[2,1]); +} + + =item prompt ( PROMPT , ISPASSWORD ) Query user C<PROMPT> and return answer from user. diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm index 490e330..0ebc68a 100644 --- a/perl/Git/SVN.pm +++ b/perl/Git/SVN.pm @@ -11,7 +11,6 @@ use Carp qw/croak/; use File::Path qw/mkpath/; use File::Copy qw/copy/; use IPC::Open3; -use Time::Local; use Memoize; # core since 5.8.0, Jul 2002 use Memoize::Storable; use POSIX qw(:signal_h); @@ -22,6 +21,7 @@ use Git qw( command_noisy command_output_pipe command_close_pipe + get_tz_offset ); use Git::SVN::Utils qw( fatal @@ -1311,14 +1311,6 @@ sub get_untracked { \@out; } -sub get_tz { - # some systmes don't handle or mishandle %z, so be creative. - my $t = shift || time; - my $gm = timelocal(gmtime($t)); - my $sign = qw( + + - )[ $t <=> $gm ]; - return sprintf("%s%02d%02d", $sign, (gmtime(abs($t - $gm)))[2,1]); -} - # parse_svn_date(DATE) # -------------------- # Given a date (in UTC) from Subversion, return a string in the format @@ -1351,7 +1343,7 @@ sub parse_svn_date { delete $ENV{TZ}; } - my $our_TZ = get_tz(); + my $our_TZ = get_tz_offset(); # This converts $epoch_in_UTC into our local timezone. my ($sec, $min, $hour, $mday, $mon, $year, diff --git a/perl/Git/SVN/Log.pm b/perl/Git/SVN/Log.pm index 3cc1c6f..3f8350a 100644 --- a/perl/Git/SVN/Log.pm +++ b/perl/Git/SVN/Log.pm @@ -2,7 +2,11 @@ package Git::SVN::Log; use strict; use warnings; use Git::SVN::Utils qw(fatal); -use Git qw(command command_oneline command_output_pipe command_close_pipe); +use Git qw(command + command_oneline + command_output_pipe + command_close_pipe + get_tz_offset); use POSIX qw/strftime/; use constant commit_log_separator => ('-' x 72) . "\n"; use vars qw/$TZ $limit $color $pager $non_recursive $verbose $oneline @@ -119,7 +123,7 @@ sub run_pager { sub format_svn_date { my $t = shift || time; require Git::SVN; - my $gmoff = Git::SVN::get_tz($t); + my $gmoff = get_tz_offset($t); return strftime("%Y-%m-%d %H:%M:%S $gmoff (%a, %d %b %Y)", localtime($t)); } -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] Fix get_tz_offset to properly handle DST boundary cases 2013-02-09 21:46 [PATCH 0/3] Fix a portability issue with git-cvsimport Ben Walton 2013-02-09 21:46 ` [PATCH 1/3] Move Git::SVN::get_tz to Git::get_tz_offset Ben Walton @ 2013-02-09 21:46 ` Ben Walton 2013-02-09 22:58 ` Junio C Hamano 2013-02-09 21:46 ` [PATCH 3/3] Avoid non-portable strftime format specifiers in git-cvsimport Ben Walton 2 siblings, 1 reply; 6+ messages in thread From: Ben Walton @ 2013-02-09 21:46 UTC (permalink / raw) To: gitster; +Cc: git, Ben Walton When passed a local time that was on the boundary of a DST change, get_tz_offset returned a GMT offset that was incorrect (off by one hour). This is because the time was converted to GMT and then back to a time stamp via timelocal() which cannot disambiguate boundary cases as noted in its documentation. Modify this algorithm, using an approach suggested by Junio C Hamano that obtains the GMT time stamp by using timegm(localtime()) instead of timelocal(gmtime()). This avoids the ambigious conversion and allows a correct time to be returned on every occassion. Signed-off-by: Ben Walton <bdwalton@gmail.com> --- perl/Git.pm | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/perl/Git.pm b/perl/Git.pm index 5649bcc..a56d1e7 100644 --- a/perl/Git.pm +++ b/perl/Git.pm @@ -103,7 +103,7 @@ use Error qw(:try); use Cwd qw(abs_path cwd); use IPC::Open2 qw(open2); use Fcntl qw(SEEK_SET SEEK_CUR); -use Time::Local qw(timelocal); +use Time::Local qw(timegm); } @@ -528,8 +528,8 @@ 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; - my $gm = timelocal(gmtime($t)); - my $sign = qw( + + - )[ $t <=> $gm ]; + my $gm = timegm(localtime($t)); + my $sign = qw( + + - )[ $gm <=> $t ]; return sprintf("%s%02d%02d", $sign, (gmtime(abs($t - $gm)))[2,1]); } -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] Fix get_tz_offset to properly handle DST boundary cases 2013-02-09 21:46 ` [PATCH 2/3] Fix get_tz_offset to properly handle DST boundary cases Ben Walton @ 2013-02-09 22:58 ` Junio C Hamano 0 siblings, 0 replies; 6+ messages in thread From: Junio C Hamano @ 2013-02-09 22:58 UTC (permalink / raw) To: Ben Walton; +Cc: git Ben Walton <bdwalton@gmail.com> writes: > When passed a local time that was on the boundary of a DST change, > get_tz_offset returned a GMT offset that was incorrect (off by one > hour). This is because the time was converted to GMT and then back to > a time stamp via timelocal() which cannot disambiguate boundary cases > as noted in its documentation. > > Modify this algorithm, using an approach suggested by Junio C Hamano > that obtains the GMT time stamp by using timegm(localtime()) instead > of timelocal(gmtime()). This avoids the ambigious conversion and > allows a correct time to be returned on every occassion. I'll reword the log message a bit to explain why the updated logic is right and also refer to the message that has the suggestion. o The implemmentation is a bit dense to my taste, but looks correct (I had to think about the comparison to come up with the value of the $sign, though). Thanks. > Signed-off-by: Ben Walton <bdwalton@gmail.com> > --- > perl/Git.pm | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/perl/Git.pm b/perl/Git.pm > index 5649bcc..a56d1e7 100644 > --- a/perl/Git.pm > +++ b/perl/Git.pm > @@ -103,7 +103,7 @@ use Error qw(:try); > use Cwd qw(abs_path cwd); > use IPC::Open2 qw(open2); > use Fcntl qw(SEEK_SET SEEK_CUR); > -use Time::Local qw(timelocal); > +use Time::Local qw(timegm); > } > > > @@ -528,8 +528,8 @@ 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; > - my $gm = timelocal(gmtime($t)); > - my $sign = qw( + + - )[ $t <=> $gm ]; > + my $gm = timegm(localtime($t)); > + my $sign = qw( + + - )[ $gm <=> $t ]; > return sprintf("%s%02d%02d", $sign, (gmtime(abs($t - $gm)))[2,1]); > } ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 3/3] Avoid non-portable strftime format specifiers in git-cvsimport 2013-02-09 21:46 [PATCH 0/3] Fix a portability issue with git-cvsimport Ben Walton 2013-02-09 21:46 ` [PATCH 1/3] Move Git::SVN::get_tz to Git::get_tz_offset Ben Walton 2013-02-09 21:46 ` [PATCH 2/3] Fix get_tz_offset to properly handle DST boundary cases Ben Walton @ 2013-02-09 21:46 ` Ben Walton 2013-02-09 22:20 ` Junio C Hamano 2 siblings, 1 reply; 6+ messages in thread From: Ben Walton @ 2013-02-09 21:46 UTC (permalink / raw) To: gitster; +Cc: git, Ben Walton Neither %s or %z are portable strftime format specifiers. There is no need for %s in git-cvsimport as the supplied time is already in seconds since the epoch. For %z, use the function get_tz_offset provided by Git.pm instead. Signed-off-by: Ben Walton <bdwalton@gmail.com> --- git-cvsimport.perl | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/git-cvsimport.perl b/git-cvsimport.perl index 0a31ebd..344f120 100755 --- a/git-cvsimport.perl +++ b/git-cvsimport.perl @@ -26,6 +26,7 @@ use IO::Socket; use IO::Pipe; use POSIX qw(strftime tzset dup2 ENOENT); use IPC::Open2; +use Git qw(get_tz_offset); $SIG{'PIPE'}="IGNORE"; set_timezone('UTC'); @@ -864,7 +865,9 @@ sub commit { } set_timezone($author_tz); - my $commit_date = strftime("%s %z", localtime($date)); + # $date is in the seconds since epoch format + my $tz_offset = get_tz_offset($date); + my $commit_date = "$date $tz_offset"; set_timezone('UTC'); $ENV{GIT_AUTHOR_NAME} = $author_name; $ENV{GIT_AUTHOR_EMAIL} = $author_email; -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] Avoid non-portable strftime format specifiers in git-cvsimport 2013-02-09 21:46 ` [PATCH 3/3] Avoid non-portable strftime format specifiers in git-cvsimport Ben Walton @ 2013-02-09 22:20 ` Junio C Hamano 0 siblings, 0 replies; 6+ messages in thread From: Junio C Hamano @ 2013-02-09 22:20 UTC (permalink / raw) To: Ben Walton; +Cc: git Ben Walton <bdwalton@gmail.com> writes: > Neither %s or %z are portable strftime format specifiers. Well, at least %z is in POSIX; "Some implementations of strftime(3) lack support for %z format" is fine, tough. Thanks. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-02-09 22:58 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-02-09 21:46 [PATCH 0/3] Fix a portability issue with git-cvsimport Ben Walton 2013-02-09 21:46 ` [PATCH 1/3] Move Git::SVN::get_tz to Git::get_tz_offset Ben Walton 2013-02-09 21:46 ` [PATCH 2/3] Fix get_tz_offset to properly handle DST boundary cases Ben Walton 2013-02-09 22:58 ` Junio C Hamano 2013-02-09 21:46 ` [PATCH 3/3] Avoid non-portable strftime format specifiers in git-cvsimport Ben Walton 2013-02-09 22:20 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).