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

[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

[PATCH 2/3] Allow Git::get_tz_offset to properly handle DST boundary times

[Ben Walton]

The Git::get_tz_offset is meant to provide a workalike replacement for
the GNU strftime %z format specifier.  The algorithm used failed to
properly handle DST boundary cases.

For example, the unix time 1162105199 in CST6CDT saw set_tz_offset
improperly return -0600 instead of -0500.

TZ=CST6CDT date -d @1162105199 +"%c %z"
Sun 29 Oct 2006 01:59:59 AM CDT -0500

$ zdump -v /usr/share/zoneinfo/CST6CDT | grep 2006
/usr/share/zoneinfo/CST6CDT  Sun Apr  2 07:59:59 2006 UTC = Sun Apr  2
01:59:59 2006 CST isdst=0 gmtoff=-21600
/usr/share/zoneinfo/CST6CDT  Sun Apr  2 08:00:00 2006 UTC = Sun Apr  2
03:00:00 2006 CDT isdst=1 gmtoff=-18000
/usr/share/zoneinfo/CST6CDT  Sun Oct 29 06:59:59 2006 UTC = Sun Oct 29
01:59:59 2006 CDT isdst=1 gmtoff=-18000
/usr/share/zoneinfo/CST6CDT  Sun Oct 29 07:00:00 2006 UTC = Sun Oct 29
01:00:00 2006 CST isdst=0 gmtoff=-21600

To determine how many hours/minutes away from GMT a particular time
was, we calculated the gmtime() of the requested time value and then
used Time::Local's timelocal() function to turn the GMT-based time
back into a scalar value representing seconds from the epoch.  Because
GMT has no daylight savings time, timelocal() cannot handle the
ambiguous times that occur at DST boundaries since there are two
possible correct results.

To work around the ambiguity at these boundaries, we must take into
account the pre and post conversion values for is_dst as provided by
both the original time value and the value that has been run through
timelocal().  If the is_dst field of the two times disagree then we
must modify the value returned from timelocal() by an hour in the
correct direction.

Signed-off-by: Ben Walton <bdwalton@gmail.com>
---
 perl/Git.pm |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/perl/Git.pm b/perl/Git.pm
index 5649bcc..788b9b4 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -528,7 +528,27 @@ 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;
+	# timelocal() has a problem when it comes to DST ambiguity so
+	# times that are on a DST boundary cannot be properly converted
+	# using it.  we will possibly adjust its result depending on whehter
+	# pre and post conversions agree on DST
 	my $gm = timelocal(gmtime($t));
+
+	# we need to know whether we were originally in DST or not
+	my $orig_dst = (localtime($t))[8];
+	# and also whether timelocal thinks we're in DST
+	my $conv_dst = (localtime($gm))[8];
+
+	# re-adjust $gm based on the DST value for the two times we're
+	# handling.
+	if ($orig_dst != $conv_dst) {
+		if ($orig_dst == 1) {
+			$gm -= 3600;
+		} else {
+			$gm += 3600;
+		}
+	}
+
 	my $sign = qw( + + - )[ $t <=> $gm ];
 	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 0/3] Fix a portability issue with git-cvsimport

[Junio C Hamano]

Ben Walton <bdwalton@gmail.com> writes:

> 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?)

There is my reroll of Eric's patch [*1*], that is in 'pu'. The topic
ends at 12b3541 (t9600: adjust for new cvsimport, 2013-01-13).

I think the folks on the traditional Git side prefer the approach
taken by it to keep the old one under cvsimport-2 while adding
Eric's as cvsimport-3 and have a separate version switcher wrapper
[*2*, *3*].  Also Chris Rorvick, a contributor to cvsps-3 & new
cvsimport combo, who already has patches to Eric's version, agrees
that it is a foundation we can build on together [*4*].

Eric hasn't spoken on the topic yet, but I think what the rest of us
agreed may be a reasonable starting point.

I think I can apply your patches on top of 12b3541 with "am -3" and
have it automatically update git-cvsimport-2.perl ;-)


[References]

*1* http://thread.gmane.org/gmane.comp.version-control.git/213170/focus=213460
*2* http://thread.gmane.org/gmane.comp.version-control.git/213170/focus=213432
*3* http://thread.gmane.org/gmane.comp.version-control.git/213170/focus=213466
*4* http://thread.gmane.org/gmane.comp.version-control.git/213537/focus=213595

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

[Chris Rorvick]

On Tue, Jan 15, 2013 at 5:10 PM, Ben Walton <bdwalton@gmail.com> wrote:
> 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.

Out of curiosity, which platforms are affected?  Assuming DST is a 1
hour shift (patch 2/3) is not necessarily portable either, though this
currently appears to only affect a small island off of the coast of
Australia.  :-)

Chris

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

[Ben Walton]

On Wed, Jan 16, 2013 at 1:53 AM, Chris Rorvick <chris@rorvick.com> wrote:
> On Tue, Jan 15, 2013 at 5:10 PM, Ben Walton <bdwalton@gmail.com> wrote:
>> 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.
>
> Out of curiosity, which platforms are affected?  Assuming DST is a 1
> hour shift (patch 2/3) is not necessarily portable either, though this
> currently appears to only affect a small island off of the coast of
> Australia.  :-)

My primary motivation on this change was for solaris.  %s isn't
supported in 10 (not sure about 11) and %z was only added in 10.  The
issue affects other older platforms as well.

Good point about the 1 hour assumption.  Is it worth hacking in
additional logic to handle Lord Howe Island?  I think it's likely a
case of "in for a penny, in for a pound" but that could lead to
madness when it comes to time zones.  Either way, the function behaves
better now than before.

(I wasn't aware of the half hour oddball wrt to DST, so I learned
something new here too!)

Thanks
-Ben
--
---------------------------------------------------------------------------------------------------------------------------
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:

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

Re: [PATCH 2/3] Allow Git::get_tz_offset to properly handle DST boundary times

[Junio C Hamano]

Ben Walton <bdwalton@gmail.com> writes:

> The Git::get_tz_offset is meant to provide a workalike replacement for
> the GNU strftime %z format specifier.  The algorithm used failed to
> properly handle DST boundary cases.
>
> For example, the unix time 1162105199 in CST6CDT saw set_tz_offset
> improperly return -0600 instead of -0500.
>
> TZ=CST6CDT date -d @1162105199 +"%c %z"
> Sun 29 Oct 2006 01:59:59 AM CDT -0500
>
> $ zdump -v /usr/share/zoneinfo/CST6CDT | grep 2006
> /usr/share/zoneinfo/CST6CDT  Sun Apr  2 07:59:59 2006 UTC = Sun Apr  2
> 01:59:59 2006 CST isdst=0 gmtoff=-21600
> /usr/share/zoneinfo/CST6CDT  Sun Apr  2 08:00:00 2006 UTC = Sun Apr  2
> 03:00:00 2006 CDT isdst=1 gmtoff=-18000
> /usr/share/zoneinfo/CST6CDT  Sun Oct 29 06:59:59 2006 UTC = Sun Oct 29
> 01:59:59 2006 CDT isdst=1 gmtoff=-18000
> /usr/share/zoneinfo/CST6CDT  Sun Oct 29 07:00:00 2006 UTC = Sun Oct 29
> 01:00:00 2006 CST isdst=0 gmtoff=-21600
>
> To determine how many hours/minutes away from GMT a particular time
> was, we calculated the gmtime() of the requested time value and then
> used Time::Local's timelocal() function to turn the GMT-based time
> back into a scalar value representing seconds from the epoch.  Because
> GMT has no daylight savings time, timelocal() cannot handle the
> ambiguous times that occur at DST boundaries since there are two
> possible correct results.
>
> To work around the ambiguity at these boundaries, we must take into
> account the pre and post conversion values for is_dst as provided by
> both the original time value and the value that has been run through
> timelocal().  If the is_dst field of the two times disagree then we
> must modify the value returned from timelocal() by an hour in the
> correct direction.

It seems to me that it is a very roundabout way.  It may be correct,
but it is unclear why the magic +/-3600 shift is the right solution
and I suspect even you wouldn't notice if I sent you back your patch
with a slight change to swap $gm += 3600 and $gm -= 3600 lines ;-).

For that timestamp in question, the human-readable representation of
gmtime($t) and localtime($t) look like these two strings:

	my $t = 1162105199;
	print gmtime($t), "\n";    # Sun Oct 29 06:59:59 2006
        print localtime($t), "\n"; # Sun Oct 29 01:59:59 2006

As a human, you can easily see that these two stringified timestamps
look 5 hours apart.  Think how you managed to do so.

If we convert these back to the seconds-since-epoch, as if these
broken-down times were both in a single timezone that does not have
any DST issues, you can get the offset (in seconds) by subtraction,
and that is essentially the same as the way in which your eyes saw
they are 5 hours apart, no?  In other words, why do you need to run
timelocal() at all?

	my $t = 1162105199;
        my $lct = timegm(localtime($t)); 
        # of course, timegm(gmtime($t)) == $t

	my $minutes = int(($lct - $t)/60);
        my $sign "+";
        if ($minutes < 0) {
		$sign = "-";
                $minutes = -$minutes;
	}
        my $hours = int($minutes/60);
        $minutes -= $hours * 60;
        sprintf("%s%02d%02d", $sign, $hours, $minutes);

Confused...

>
> Signed-off-by: Ben Walton <bdwalton@gmail.com>
> ---
>  perl/Git.pm |   20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/perl/Git.pm b/perl/Git.pm
> index 5649bcc..788b9b4 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -528,7 +528,27 @@ 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;
> +	# timelocal() has a problem when it comes to DST ambiguity so
> +	# times that are on a DST boundary cannot be properly converted
> +	# using it.  we will possibly adjust its result depending on whehter
> +	# pre and post conversions agree on DST
>  	my $gm = timelocal(gmtime($t));
> +
> +	# we need to know whether we were originally in DST or not
> +	my $orig_dst = (localtime($t))[8];
> +	# and also whether timelocal thinks we're in DST
> +	my $conv_dst = (localtime($gm))[8];
> +
> +	# re-adjust $gm based on the DST value for the two times we're
> +	# handling.
> +	if ($orig_dst != $conv_dst) {
> +		if ($orig_dst == 1) {
> +			$gm -= 3600;
> +		} else {
> +			$gm += 3600;
> +		}
> +	}
> +
>  	my $sign = qw( + + - )[ $t <=> $gm ];
>  	return sprintf("%s%02d%02d", $sign, (gmtime(abs($t - $gm)))[2,1]);
>  }

Re: [PATCH 2/3] Allow Git::get_tz_offset to properly handle DST boundary times

[Ben Walton]

On Thu, Jan 17, 2013 at 7:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ben Walton <bdwalton@gmail.com> writes:
>
>> The Git::get_tz_offset is meant to provide a workalike replacement for
>> the GNU strftime %z format specifier.  The algorithm used failed to
>> properly handle DST boundary cases.
>>
>> For example, the unix time 1162105199 in CST6CDT saw set_tz_offset
>> improperly return -0600 instead of -0500.
>>
>> TZ=CST6CDT date -d @1162105199 +"%c %z"
>> Sun 29 Oct 2006 01:59:59 AM CDT -0500
>>
>> $ zdump -v /usr/share/zoneinfo/CST6CDT | grep 2006
>> /usr/share/zoneinfo/CST6CDT  Sun Apr  2 07:59:59 2006 UTC = Sun Apr  2
>> 01:59:59 2006 CST isdst=0 gmtoff=-21600
>> /usr/share/zoneinfo/CST6CDT  Sun Apr  2 08:00:00 2006 UTC = Sun Apr  2
>> 03:00:00 2006 CDT isdst=1 gmtoff=-18000
>> /usr/share/zoneinfo/CST6CDT  Sun Oct 29 06:59:59 2006 UTC = Sun Oct 29
>> 01:59:59 2006 CDT isdst=1 gmtoff=-18000
>> /usr/share/zoneinfo/CST6CDT  Sun Oct 29 07:00:00 2006 UTC = Sun Oct 29
>> 01:00:00 2006 CST isdst=0 gmtoff=-21600
>>
>> To determine how many hours/minutes away from GMT a particular time
>> was, we calculated the gmtime() of the requested time value and then
>> used Time::Local's timelocal() function to turn the GMT-based time
>> back into a scalar value representing seconds from the epoch.  Because
>> GMT has no daylight savings time, timelocal() cannot handle the
>> ambiguous times that occur at DST boundaries since there are two
>> possible correct results.
>>
>> To work around the ambiguity at these boundaries, we must take into
>> account the pre and post conversion values for is_dst as provided by
>> both the original time value and the value that has been run through
>> timelocal().  If the is_dst field of the two times disagree then we
>> must modify the value returned from timelocal() by an hour in the
>> correct direction.
>
> It seems to me that it is a very roundabout way.  It may be correct,
> but it is unclear why the magic +/-3600 shift is the right solution
> and I suspect even you wouldn't notice if I sent you back your patch
> with a slight change to swap $gm += 3600 and $gm -= 3600 lines ;-).
>
> For that timestamp in question, the human-readable representation of
> gmtime($t) and localtime($t) look like these two strings:
>
>         my $t = 1162105199;
>         print gmtime($t), "\n";    # Sun Oct 29 06:59:59 2006
>         print localtime($t), "\n"; # Sun Oct 29 01:59:59 2006
>
> As a human, you can easily see that these two stringified timestamps
> look 5 hours apart.  Think how you managed to do so.
>
> If we convert these back to the seconds-since-epoch, as if these
> broken-down times were both in a single timezone that does not have
> any DST issues, you can get the offset (in seconds) by subtraction,
> and that is essentially the same as the way in which your eyes saw
> they are 5 hours apart, no?  In other words, why do you need to run
> timelocal() at all?
>
>         my $t = 1162105199;
>         my $lct = timegm(localtime($t));
>         # of course, timegm(gmtime($t)) == $t
>
>         my $minutes = int(($lct - $t)/60);
>         my $sign "+";
>         if ($minutes < 0) {
>                 $sign = "-";
>                 $minutes = -$minutes;
>         }
>         my $hours = int($minutes/60);
>         $minutes -= $hours * 60;
>         sprintf("%s%02d%02d", $sign, $hours, $minutes);
>
> Confused...
>
>>
>> Signed-off-by: Ben Walton <bdwalton@gmail.com>
>> ---
>>  perl/Git.pm |   20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/perl/Git.pm b/perl/Git.pm
>> index 5649bcc..788b9b4 100644
>> --- a/perl/Git.pm
>> +++ b/perl/Git.pm
>> @@ -528,7 +528,27 @@ 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;
>> +     # timelocal() has a problem when it comes to DST ambiguity so
>> +     # times that are on a DST boundary cannot be properly converted
>> +     # using it.  we will possibly adjust its result depending on whehter
>> +     # pre and post conversions agree on DST
>>       my $gm = timelocal(gmtime($t));
>> +
>> +     # we need to know whether we were originally in DST or not
>> +     my $orig_dst = (localtime($t))[8];
>> +     # and also whether timelocal thinks we're in DST
>> +     my $conv_dst = (localtime($gm))[8];
>> +
>> +     # re-adjust $gm based on the DST value for the two times we're
>> +     # handling.
>> +     if ($orig_dst != $conv_dst) {
>> +             if ($orig_dst == 1) {
>> +                     $gm -= 3600;
>> +             } else {
>> +                     $gm += 3600;
>> +             }
>> +     }
>> +
>>       my $sign = qw( + + - )[ $t <=> $gm ];
>>       return sprintf("%s%02d%02d", $sign, (gmtime(abs($t - $gm)))[2,1]);
>>  }


Sorry for the slow response, I didn't have a good chance to look at
this until now.  You are correct; your solution appears simpler and
also avoids the oddball 1/2 hour DST shift.  I can re-roll the series
with your code (and credit for it) or you can apply you change on top
of my series...whichever is easiest for you.

Thanks
-Ben
--
---------------------------------------------------------------------------------------------------------------------------
Take the risk of thinking for yourself.  Much more happiness,
truth, beauty and wisdom will come to you that way.

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

Re: [PATCH 2/3] Allow Git::get_tz_offset to properly handle DST boundary times

[Junio C Hamano]

Ben Walton <bdwalton@gmail.com> writes:

> also avoids the oddball 1/2 hour DST shift.  I can re-roll the series
> with your code (and credit for it) or you can apply you change on top
> of my series...whichever is easiest for you.

Please reroll, as I do not have patience either to set up a test
case and verify the end result is correct, or to come up with a test
case for it.  For this particular case, I think the identification
of the issue weighs more than the implementation for fix it, so
please retain the authorship for the fix; mentioning "taking the
implementation idea from Junio" in the log message is the right
amount of credit due to me.

Thanks.