Patchwork [v3] Allow commit date before Unix epoch, clean fix for (issue2513)

login
register
mail settings
Submitter Florent Gallaire
Date April 2, 2016, 6:28 p.m.
Message ID <CAB9F1ceFugAQ7TjU=HjsKKE33Pz8MmRfQnRnJZ=bMN9-jy_+SQ@mail.gmail.com>
Download mbox | patch
Permalink /patch/14265/
State Superseded
Headers show

Comments

Florent Gallaire - April 2, 2016, 6:28 p.m.
# HG changeset patch
# User Florent Gallaire <fgallaire@gmail.com>
# Date 1459621582 -7200
#      Sat Apr 02 20:26:22 2016 +0200
# Node ID a415db6cb545dc312019cd919dbbe7e2b7cbe0f9
# Parent  ff0d3b6b287f89594bd8d0308fe2810d2a18ea01
date: reallow negative timestamp, fix for Windows buggy gmtime() (issue2513)

DVCS are very useful to store various texts (as legislation) written before
Unix epoch. Fri, 13 Dec 1901 is a nice gain over Thu, 01 Jan 1970.
Revert dd24f3e7ca9e and e1002cf9fe54.
Adrian Buehlmann - April 2, 2016, 9:03 p.m.
On 2016-04-02 20:28, Florent Gallaire wrote:
> # HG changeset patch
> # User Florent Gallaire <fgallaire@gmail.com>
> # Date 1459621582 -7200
> #      Sat Apr 02 20:26:22 2016 +0200
> # Node ID a415db6cb545dc312019cd919dbbe7e2b7cbe0f9
> # Parent  ff0d3b6b287f89594bd8d0308fe2810d2a18ea01
> date: reallow negative timestamp, fix for Windows buggy gmtime() (issue2513)
> 
> DVCS are very useful to store various texts (as legislation) written before
> Unix epoch. Fri, 13 Dec 1901 is a nice gain over Thu, 01 Jan 1970.
> Revert dd24f3e7ca9e and e1002cf9fe54.
> 
> diff -r ff0d3b6b287f -r a415db6cb545 mercurial/util.py
> --- a/mercurial/util.py    Tue Mar 29 12:29:00 2016 -0500
> +++ b/mercurial/util.py    Sat Apr 02 20:26:22 2016 +0200
> @@ -1578,9 +1578,6 @@
>      number of seconds away from UTC. if timezone is false, do not
>      append time zone to string."""
>      t, tz = date or makedate()
> -    if t < 0:
> -        t = 0   # time.gmtime(lt) fails on Windows for lt < -43200
> -        tz = 0
>      if "%1" in format or "%2" in format or "%z" in format:
>          sign = (tz > 0) and "-" or "+"
>          minutes = abs(tz) // 60
> @@ -1589,11 +1586,15 @@
>          format = format.replace("%1", "%c%02d" % (sign, q))
>          format = format.replace("%2", "%02d" % r)
>      try:
> -        t = time.gmtime(float(t) - tz)
> -    except ValueError:
> +        # Never use time.gmtime() and datetime.datetime.fromtimestamp()
> +        # because they use the gmtime() system call which is buggy on Windows
> +        # for negative values.
> +        t = datetime.datetime(1970, 1, 1) + datetime.timedelta(seconds=t - tz)
> +    except OverflowError:

Note: If I understand docs.python.org correctly, OverflowError can only
happen if the result of the sum would have a year smaller than 1
(datetime.MINYEAR) or larger than 9999 (datetime.MAXYEAR).

>          # time was out of range
> -        t = time.gmtime(sys.maxint)
> -    s = time.strftime(format, t)
> +        t = (datetime.datetime(1970, 1, 1) +
> +             datetime.timedelta(seconds=cmp(t, 0) * 0x7fffffff))
> +    s = t.strftime(format)

Nitpick: Since you don't use time.gmtime any more, clamping to the magic
year-2038-value on OverflowError is... a little bit strange.

Perhaps, it would make more sense to clamp to the limits of
datetime.datetime instead? Or explicitly check that t is within the
bounds of

datetime.datetime(1970, 1, 1) + datetime.timedelta(0x7fffffff)
datetime.datetime(1970, 1, 1) + datetime.timedelta(-0x7fffffff)

?

(see also c208dcd0f709, which introduced the original clamping for the
time.gmtime call)

>      return s
> 
>  def shortdate(date=None):
> @@ -1714,8 +1715,6 @@
>      # to UTC+14
>      if abs(when) > 0x7fffffff:
>          raise Abort(_('date exceeds 32 bits: %d') % when)
> -    if when < 0:
> -        raise Abort(_('negative date value: %d') % when)
>      if offset < -50400 or offset > 43200:
>          raise Abort(_('impossible time zone offset: %d') % offset)
>      return when, offset
> diff -r ff0d3b6b287f -r a415db6cb545 tests/test-commit.t
> --- a/tests/test-commit.t    Tue Mar 29 12:29:00 2016 -0500
> +++ b/tests/test-commit.t    Sat Apr 02 20:26:22 2016 +0200
> @@ -27,8 +27,8 @@
>    $ hg commit -d '111111111111 0' -m commit-7
>    abort: date exceeds 32 bits: 111111111111
>    [255]
> -  $ hg commit -d '-7654321 3600' -m commit-7
> -  abort: negative date value: -7654321
> +  $ hg commit -d '-111111111111 0' -m commit-7
> +  abort: date exceeds 32 bits: -111111111111
>    [255]
> 
>  commit added file that has been deleted
Florent Gallaire - April 2, 2016, 10:14 p.m.
> Nitpick: Since you don't use time.gmtime any more, clamping to the magic
> year-2038-value on OverflowError is... a little bit strange.

I agree it's not perfect, but it's a lot better than
t = time.gmtime(sys.maxint)
which is completely broken on 64 bits system (exception on the except clause)

1) I just fix issue 2513 the right way
2) it's simple
3) for backward compatibility, hg has never accepted more than signed
32 bits for dates

If you want to go further after my patch.... IMHO, it's the good first step.

Florent

> Perhaps, it would make more sense to clamp to the limits of
> datetime.datetime instead? Or explicitly check that t is within the
> bounds of
>
> datetime.datetime(1970, 1, 1) + datetime.timedelta(0x7fffffff)
> datetime.datetime(1970, 1, 1) + datetime.timedelta(-0x7fffffff)
>
> ?
>
> (see also c208dcd0f709, which introduced the original clamping for the
> time.gmtime call)
>
>>      return s
>>
>>  def shortdate(date=None):
>> @@ -1714,8 +1715,6 @@
>>      # to UTC+14
>>      if abs(when) > 0x7fffffff:
>>          raise Abort(_('date exceeds 32 bits: %d') % when)
>> -    if when < 0:
>> -        raise Abort(_('negative date value: %d') % when)
>>      if offset < -50400 or offset > 43200:
>>          raise Abort(_('impossible time zone offset: %d') % offset)
>>      return when, offset
>> diff -r ff0d3b6b287f -r a415db6cb545 tests/test-commit.t
>> --- a/tests/test-commit.t    Tue Mar 29 12:29:00 2016 -0500
>> +++ b/tests/test-commit.t    Sat Apr 02 20:26:22 2016 +0200
>> @@ -27,8 +27,8 @@
>>    $ hg commit -d '111111111111 0' -m commit-7
>>    abort: date exceeds 32 bits: 111111111111
>>    [255]
>> -  $ hg commit -d '-7654321 3600' -m commit-7
>> -  abort: negative date value: -7654321
>> +  $ hg commit -d '-111111111111 0' -m commit-7
>> +  abort: date exceeds 32 bits: -111111111111
>>    [255]
>>
>>  commit added file that has been deleted
Adrian Buehlmann - April 3, 2016, 9:11 a.m.
On 2016-04-03 00:14, Florent Gallaire wrote:
>> Nitpick: Since you don't use time.gmtime any more, clamping to the magic
>> year-2038-value on OverflowError is... a little bit strange.
> 
> I agree it's not perfect, but it's a lot better than
> t = time.gmtime(sys.maxint)
> which is completely broken on 64 bits system (exception on the except clause)
> 
> 1) I just fix issue 2513 the right way
> 2) it's simple
> 3) for backward compatibility, hg has never accepted more than signed
> 32 bits for dates
> 
> If you want to go further after my patch.... IMHO, it's the good first step.

Well. It's better to do your patch right - instead of cleaning up
afterwards.

If you want to clamp to 0x7fffffff, why don't you simplify matters to:

    d = t - tz
    if abs(d) > 0x7fffffff:
        d = cmp(d, 0) * 0x7fffffff
    t = datetime.datetime(1970, 1, 1) + datetime.timedelta(seconds=d)
    s = t.strftime(format)
    return s

The catching of OverflowError looks a bit pointless to me. It looks like
you are trying to preserve the try-block introduced in c208dcd0f709 -
which I think is no longer needed.
Florent Gallaire - April 3, 2016, 11:06 a.m.
2016-04-03 11:11 GMT+02:00 Adrian Buehlmann <adrian@cadifra.com>:
> Well. It's better to do your patch right - instead of cleaning up
> afterwards.
>
> If you want to clamp to 0x7fffffff, why don't you simplify matters to:
>
>     d = t - tz
>     if abs(d) > 0x7fffffff:
>         d = cmp(d, 0) * 0x7fffffff
>     t = datetime.datetime(1970, 1, 1) + datetime.timedelta(seconds=d)
>     s = t.strftime(format)
>     return s

I agree with you, this is the right way. New patch submitted.

> The catching of OverflowError looks a bit pointless to me. It looks like
> you are trying to preserve the try-block introduced in c208dcd0f709 -
> which I think is no longer needed.

In fact the try-block was broken since the beginning.

Thanks for your attention.

Florent

Patch

diff -r ff0d3b6b287f -r a415db6cb545 mercurial/util.py
--- a/mercurial/util.py    Tue Mar 29 12:29:00 2016 -0500
+++ b/mercurial/util.py    Sat Apr 02 20:26:22 2016 +0200
@@ -1578,9 +1578,6 @@ 
     number of seconds away from UTC. if timezone is false, do not
     append time zone to string."""
     t, tz = date or makedate()
-    if t < 0:
-        t = 0   # time.gmtime(lt) fails on Windows for lt < -43200
-        tz = 0
     if "%1" in format or "%2" in format or "%z" in format:
         sign = (tz > 0) and "-" or "+"
         minutes = abs(tz) // 60
@@ -1589,11 +1586,15 @@ 
         format = format.replace("%1", "%c%02d" % (sign, q))
         format = format.replace("%2", "%02d" % r)
     try:
-        t = time.gmtime(float(t) - tz)
-    except ValueError:
+        # Never use time.gmtime() and datetime.datetime.fromtimestamp()
+        # because they use the gmtime() system call which is buggy on Windows
+        # for negative values.
+        t = datetime.datetime(1970, 1, 1) + datetime.timedelta(seconds=t - tz)
+    except OverflowError:
         # time was out of range
-        t = time.gmtime(sys.maxint)
-    s = time.strftime(format, t)
+        t = (datetime.datetime(1970, 1, 1) +
+             datetime.timedelta(seconds=cmp(t, 0) * 0x7fffffff))
+    s = t.strftime(format)
     return s

 def shortdate(date=None):
@@ -1714,8 +1715,6 @@ 
     # to UTC+14
     if abs(when) > 0x7fffffff:
         raise Abort(_('date exceeds 32 bits: %d') % when)
-    if when < 0:
-        raise Abort(_('negative date value: %d') % when)
     if offset < -50400 or offset > 43200:
         raise Abort(_('impossible time zone offset: %d') % offset)
     return when, offset
diff -r ff0d3b6b287f -r a415db6cb545 tests/test-commit.t
--- a/tests/test-commit.t    Tue Mar 29 12:29:00 2016 -0500
+++ b/tests/test-commit.t    Sat Apr 02 20:26:22 2016 +0200
@@ -27,8 +27,8 @@ 
   $ hg commit -d '111111111111 0' -m commit-7
   abort: date exceeds 32 bits: 111111111111
   [255]
-  $ hg commit -d '-7654321 3600' -m commit-7
-  abort: negative date value: -7654321
+  $ hg commit -d '-111111111111 0' -m commit-7
+  abort: date exceeds 32 bits: -111111111111
   [255]

 commit added file that has been deleted