Re: [PATCH] Call timegm and timelocal with 4-digit year

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 24, 2018, 22:28 UTC
Message-ID: <87tvu6dntx.fsf@evledraar.gmail.com>
In-Reply-To: <20180223172045.32090-1-bwiedemann@suse.de>

On Fri, Feb 23 2018, Bernhard M. Wiedemann jotted:

Show 35 quoted lines

> amazingly timegm(gmtime(0)) is only 0 before 2020
> because perl's timegm deviates from GNU timegm(3) in how it handles years.
>
> man Time::Local says
>
>  Whenever possible, use an absolute four digit year instead.
>
> with a detailed explanation about ambiguity of 2-digit years above that.
>
> Even though this ambiguity is error-prone with >50% of users getting it
> wrong, it has been like this for 20+ years, so we just use 4-digit years
> everywhere to be on the safe side.
>
> We add some extra logic to cvsimport because it allows 2-digit year
> input and interpreting an 18 as 1918 can be avoided easily and safely.
>
> Signed-off-by: Bernhard M. Wiedemann <bwiedemann@suse.de>
> ---
>  contrib/examples/git-svnimport.perl | 2 +-
>  git-cvsimport.perl                  | 4 +++-
>  perl/Git.pm                         | 4 +++-
>  perl/Git/SVN.pm                     | 2 +-
>  4 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/contrib/examples/git-svnimport.perl b/contrib/examples/git-svnimport.perl
> index c414f0d9c..75a43e23b 100755
> --- a/contrib/examples/git-svnimport.perl
> +++ b/contrib/examples/git-svnimport.perl
> @@ -238,7 +238,7 @@ sub pdate($) {
>  	my($d) = @_;
>  	$d =~ m#(\d\d\d\d)-(\d\d)-(\d\d)T(\d\d):(\d\d):(\d\d)#
>  		or die "Unparseable date: $d\n";
> -	my $y=$1; $y-=1900 if $y>1900;
> +	my $y=$1; $y+=1900 if $y<1000;
>  	return timegm($6||0,$5,$4,$3,$2-1,$y);

I wonder if this whole thing was just cargo-culted to begin with. We need to match (\d\d\d\d) here, so did SVN's format ever have years like "0098" (just "98" wouldn't match), so I suspect the whole munging could be dropped, but this change seems harmless. Just something that jumped out at me reviewing this.

Show 14 quoted lines

> diff --git a/git-cvsimport.perl b/git-cvsimport.perl
> index 2d8df8317..b31613cb8 100755
> --- a/git-cvsimport.perl
> +++ b/git-cvsimport.perl
> @@ -601,7 +601,9 @@ sub pdate($) {
>  	my ($d) = @_;
>  	m#(\d{2,4})/(\d\d)/(\d\d)\s(\d\d):(\d\d)(?::(\d\d))?#
>  		or die "Unparseable date: $d\n";
> -	my $y=$1; $y-=1900 if $y>1900;
> +	my $y=$1;
> +	$y+=100 if $y<70;
> +	$y+=1900 if $y<1000;
>  	return timegm($6||0,$5,$4,$3,$2-1,$y);
>  }

My Time::Local 1.2300 on perl 5.024001 currently interprets "69" here as 1969, but after this it'll be 2069.

Now I doubt anyone's going to be importing CVS history of projects a little over 20 years before CVS was created in 1990 (although I suppose old imports...), but just wanted to note it since it seems odd for code that's auto-interpreting double digit years for the purposes of importing existing data to end up in an edge case where it returns dates more than 50 years in the future.

Show 13 quoted lines

> diff --git a/perl/Git.pm b/perl/Git.pm
> index ffa09ace9..df62518c7 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -534,7 +534,9 @@ If TIME is not supplied, the current local time is used.
>  sub get_tz_offset {
>  	# some systems don't handle or mishandle %z, so be creative.
>  	my $t = shift || time;
> -	my $gm = timegm(localtime($t));
> +	my @t = localtime($t);
> +	$t[5] += 1900;
> +	my $gm = timegm(@t);
>

Nice. Just using the 4-digit date is always more correct and won't ever be buggy.

Show 13 quoted lines

>  	my $sign = qw( + + - )[ $gm <=> $t ];
>  	return sprintf("%s%02d%02d", $sign, (gmtime(abs($t - $gm)))[2,1]);
>  }
> diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
> index bc4eed3d7..991a5885e 100644
> --- a/perl/Git/SVN.pm
> +++ b/perl/Git/SVN.pm
> @@ -1405,7 +1405,7 @@ sub parse_svn_date {
>  		$ENV{TZ} = 'UTC';
>
>  		my $epoch_in_UTC =
> -		    Time::Local::timelocal($S, $M, $H, $d, $m - 1, $Y - 1900);
> +		    Time::Local::timelocal($S, $M, $H, $d, $m - 1, $Y);

Ditto. Nicely caught.

>
>  		# Determine our local timezone (including DST) at the
>  		# time of $epoch_in_UTC.  $Git::SVN::Log::TZ stored the

Anyway, this all looks good to me as-is. That CVS edge case is obscure and not worth focusing on, and the SVN one could be fixed up in another commit if anyone cared.

I just spent a bit more time than I should have wondering what this timegm() edge case was about and whether it might impact other (unrelated to git) code I had.