[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

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

[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.

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

[Ben Walton]

This patch series started as a quick fix for the use of %s and %z in
git-cvsimport but grew slightly when I realized that the get_tz
(get_tz_offset after this series) function used by Git::SVN didn't
properly handle DST boundary conditions.

I realize that Eric Raymond is working to deprecate the current
iteration of git-cvsimport so this series may be only partially
worthwhile.  (If the cvsps 2 vs 3 issue does require a fallback
git-cvsimport script then maybe the whole series is still valid?)  I'm
not attached to the current git-cvsimport so if the third patch isn't
useful then maybe the only the second patch is worthwhile (modified to
correct the function in its current location).

Currently, the DST boundary functionality is exercised by the
git-cvsimport tests.  If those go away as part of Eric's work then
another test that monitors this condition should be added.  I can do
that as part of this series if it seems the right way to go.

Ben Walton (3):
  Move Git::SVN::get_tz to Git::get_tz_offset
  Allow Git::get_tz_offset to properly handle DST boundary times
  Avoid non-portable strftime format specifiers in git-cvsimport

 git-cvsimport.perl  |    5 ++++-
 perl/Git.pm         |   43 +++++++++++++++++++++++++++++++++++++++++++
 perl/Git/SVN.pm     |   12 ++----------
 perl/Git/SVN/Log.pm |    8 ++++++--
 4 files changed, 55 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 59215fa..8c84560 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

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

[Junio C Hamano]

Ben Walton <bdwalton@gmail.com> writes:

> +sub get_tz_offset {
> +	# some systmes don't handle or mishandle %z, so be creative.

Hmph.  I wonder if we can use %z if it is handled correctly and fall
back to this code only on platforms that are broken?

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

[Ben Walton]

On Wed, Jan 16, 2013 at 3:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ben Walton <bdwalton@gmail.com> writes:
>
>> +sub get_tz_offset {
>> +     # some systmes don't handle or mishandle %z, so be creative.
>
> Hmph.  I wonder if we can use %z if it is handled correctly and fall
> back to this code only on platforms that are broken?

That would be perfectly acceptable to me.  The reason I set it up to
always run through this function here is that when I originally added
this function for git-svn, I'd made it conditional and Eric Wong
preferred that the function be used exclusively[1].  I opted to take
the same approach here to keep things congrous.

If it were to be conditional, I think I'd add a variable to the build
system and have the code leverage that at runtime instead of the
try/except approach I attempted in 2009.

Thanks
-Ben

[1] http://lists-archives.com/git/683572-git-svn-fix-for-systems-without-strftime-z.html
--
---------------------------------------------------------------------------------------------------------------------------
Take the risk of thinking for yourself.  Much more happiness,
truth, beauty and wisdom will come to you that way.

-Christopher Hitchens
---------------------------------------------------------------------------------------------------------------------------

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

[Junio C Hamano]

Ben Walton <bdwalton@gmail.com> writes:

> On Wed, Jan 16, 2013 at 3:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Ben Walton <bdwalton@gmail.com> writes:
>>
>>> +sub get_tz_offset {
>>> +     # some systmes don't handle or mishandle %z, so be creative.
>>
>> Hmph.  I wonder if we can use %z if it is handled correctly and fall
>> back to this code only on platforms that are broken?
>
> That would be perfectly acceptable to me.  The reason I set it up to
> always run through this function here is that when I originally added
> this function for git-svn, I'd made it conditional and Eric Wong
> preferred that the function be used exclusively[1].  I opted to take
> the same approach here to keep things congrous.
>
> If it were to be conditional, I think I'd add a variable to the build
> system and have the code leverage that at runtime instead of the
> try/except approach I attempted in 2009.

If the code was originally unconditional for a reason (and I think
being bug-to-bug compatible across platforms is actually a good
thing in a tool like importers), I would not object to it.  Thanks
for the back-story.