[PATCH 0/3] Fix a portability issue with git-cvsimport

[PATCH 0/3] Fix a portability issue with git-cvsimport

[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

[PATCH 1/3] Move Git::SVN::get_tz to Git::get_tz_offset

[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

[PATCH 2/3] Fix get_tz_offset to properly handle DST boundary cases

[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

[PATCH 3/3] Avoid non-portable strftime format specifiers in git-cvsimport

[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

Re: [PATCH 3/3] Avoid non-portable strftime format specifiers in git-cvsimport

[Junio C Hamano]

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.

Re: [PATCH 2/3] Fix get_tz_offset to properly handle DST boundary cases

[Junio C Hamano]

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]);
>  }