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

login
register
mail settings
Submitter Florent Gallaire
Date April 3, 2016, 10:56 a.m.
Message ID <CAB9F1cfTGf-MHfX=OkjZ0resGz33HQGFFW0PByXxnR5NOYSNhg@mail.gmail.com>
Download mbox | patch
Permalink /patch/14283/
State Accepted
Headers show

Comments

Florent Gallaire - April 3, 2016, 10:56 a.m.
# HG changeset patch
# User Florent Gallaire <fgallaire@gmail.com>
# Date 1459680844 -7200
#      Sun Apr 03 12:54:04 2016 +0200
# Node ID 0342a67d70739d2e22c45bf29783949092441342
# 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, fix c208dcd0f709.
Adrian Buehlmann - April 3, 2016, 11:47 a.m.
On 2016-04-03 12:56, Florent Gallaire wrote:
> # HG changeset patch
> # User Florent Gallaire <fgallaire@gmail.com>
> # Date 1459680844 -7200
> #      Sun Apr 03 12:54:04 2016 +0200
> # Node ID 0342a67d70739d2e22c45bf29783949092441342
> # 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, fix c208dcd0f709.
> 
> diff -r ff0d3b6b287f -r 0342a67d7073 mercurial/util.py
> --- a/mercurial/util.py    Tue Mar 29 12:29:00 2016 -0500
> +++ b/mercurial/util.py    Sun Apr 03 12:54:04 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
> @@ -1588,12 +1585,14 @@
>          format = format.replace("%z", "%1%2")
>          format = format.replace("%1", "%c%02d" % (sign, q))
>          format = format.replace("%2", "%02d" % r)
> -    try:
> -        t = time.gmtime(float(t) - tz)
> -    except ValueError:
> -        # time was out of range
> -        t = time.gmtime(sys.maxint)
> -    s = time.strftime(format, t)
> +    d = t - tz
> +    if abs(d) > 0x7fffffff:
> +        d = cmp(d, 0) * 0x7fffffff
> +    # 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=d)
> +    s = t.strftime(format)
>      return s
> 
>  def shortdate(date=None):
> @@ -1714,8 +1713,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 0342a67d7073 tests/test-commit.t
> --- a/tests/test-commit.t    Tue Mar 29 12:29:00 2016 -0500
> +++ b/tests/test-commit.t    Sun Apr 03 12:54:04 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

Looks good to me. I think this is less of a problem than I first feared.

The expression

   datetime.datetime(1970, 1, 1) + datetime.timedelta(seconds=d)

is a pretty elegant way of avoiding the known problems with negative
timestamps on Windows.
Pierre-Yves David - April 3, 2016, 11:45 p.m.
On 04/03/2016 03:56 AM, Florent Gallaire wrote:
> # HG changeset patch
> # User Florent Gallaire <fgallaire@gmail.com>
> # Date 1459680844 -7200
> #      Sun Apr 03 12:54:04 2016 +0200
> # Node ID 0342a67d70739d2e22c45bf29783949092441342
> # 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, fix c208dcd0f709.
>
> diff -r ff0d3b6b287f -r 0342a67d7073 mercurial/util.py
> --- a/mercurial/util.py    Tue Mar 29 12:29:00 2016 -0500
> +++ b/mercurial/util.py    Sun Apr 03 12:54:04 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
> @@ -1588,12 +1585,14 @@
>           format = format.replace("%z", "%1%2")
>           format = format.replace("%1", "%c%02d" % (sign, q))
>           format = format.replace("%2", "%02d" % r)
> -    try:
> -        t = time.gmtime(float(t) - tz)
> -    except ValueError:
> -        # time was out of range
> -        t = time.gmtime(sys.maxint)
> -    s = time.strftime(format, t)
> +    d = t - tz
> +    if abs(d) > 0x7fffffff:
> +        d = cmp(d, 0) * 0x7fffffff

Can we have a small inline comment that explain where this 0x7fffffff 
value comes from?

> +    # 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=d)
> +    s = t.strftime(format)
>       return s
>
>   def shortdate(date=None):
> @@ -1714,8 +1713,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 0342a67d7073 tests/test-commit.t
> --- a/tests/test-commit.t    Tue Mar 29 12:29:00 2016 -0500
> +++ b/tests/test-commit.t    Sun Apr 03 12:54:04 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]

As I said earlier, we want stronger testing of this (or pointer that 
this is already tested properly testing in another patch),

You are adding (okay, re-introducing) a space of possible date, we 
should add automated testing for values in this space including possible 
limits.

If we don't do this, I'll have a low confidence that this patch is not 
breaking windows again. And equally important, someone may as well 
silently break that feature in the future, reintroducing regression.

Cheers,
Florent Gallaire - April 4, 2016, 1:13 a.m.
2016-04-04 1:45 GMT+02:00 Pierre-Yves David <pierre-yves.david@ens-lyon.org>:
> Can we have a small inline comment that explain where this 0x7fffffff value
> comes from?

This is already done in the parsedate() function in the same util.py file.

> As I said earlier, we want stronger testing of this (or pointer that this is
> already tested properly testing in another patch),
>
> You are adding (okay, re-introducing) a space of possible date, we should
> add automated testing for values in this space including possible limits.

Add all tests you want.

> If we don't do this, I'll have a low confidence that this patch is not
> breaking windows again. And equally important, someone may as well silently
> break that feature in the future, reintroducing regression.

You can be confident, this patch doesn't break Windows (more than it
already is of course).

Cheers,

Florent
Adrian Buehlmann - April 4, 2016, 7:07 a.m.
On 2016-04-04 03:13, Florent Gallaire wrote:
> 2016-04-04 1:45 GMT+02:00 Pierre-Yves David <pierre-yves.david@ens-lyon.org>:
>> Can we have a small inline comment that explain where this 0x7fffffff value
>> comes from?
> 
> This is already done in the parsedate() function in the same util.py file.
> 
>> As I said earlier, we want stronger testing of this (or pointer that this is
>> already tested properly testing in another patch),
>>
>> You are adding (okay, re-introducing) a space of possible date, we should
>> add automated testing for values in this space including possible limits.
> 
> Add all tests you want.

Reviewers usually don't do the work for contributers (they are busy
enough already). It should be pretty easy for you to add a couple of
testcases to existing tests. This is often done at the end of an
existing testsfile or wherever you see fit.

It's ok if you run the testsuite on Linux only, so it should be pretty
easy. See https://www.mercurial-scm.org/wiki/WritingTests

Perhaps, you can also add a few doctests to the datestr function. See
for example the pre-existing doctests for checkwinfilename() on line
1069 in util.py and
https://www.mercurial-scm.org/wiki/WritingTests#Writing_a_Python_doctest

>> If we don't do this, I'll have a low confidence that this patch is not
>> breaking windows again. And equally important, someone may as well silently
>> break that feature in the future, reintroducing regression.
> 
> You can be confident, this patch doesn't break Windows (more than it
> already is of course).

Having it tested in the testsuite is better.
Florent Gallaire - April 4, 2016, 10:59 a.m.
2016-04-04 9:07 GMT+02:00 Adrian Buehlmann <adrian@cadifra.com>:
> Reviewers usually don't do the work for contributers (they are busy
> enough already).

I really think it's unfair since:
1) You know what ? I'm busy enough already too
2) I'm not paid thousands of dollars a month by Facebook or other to
work on Mercurial
3) I have fixed THREE buggy commits that were good enough to be in
Mercurial for six years.

> Having it tested in the testsuite is better.

I agree with you, but it has never been done before and it was not a
problem until now.

Florent
Adrian Buehlmann - April 4, 2016, 11:45 a.m.
On 2016-04-04 12:59, Florent Gallaire wrote:
> 2016-04-04 9:07 GMT+02:00 Adrian Buehlmann <adrian@cadifra.com>:
>> Reviewers usually don't do the work for contributers (they are busy
>> enough already).
> 
> I really think it's unfair since:
> 1) You know what ? I'm busy enough already too
> 2) I'm not paid thousands of dollars a month by Facebook or other to
> work on Mercurial
> 3) I have fixed THREE buggy commits that were good enough to be in
> Mercurial for six years.
> 
>> Having it tested in the testsuite is better.
> 
> I agree with you, but it has never been done before and it was not a
> problem until now.

Ok. Perhaps I can hack up some test changes and resend them together
with your patch as a series.

My mercurial dev environment (and knowledge) is a bit outdated though...
(I've been trying to keep away from mercurial hacking for a while now).

Just leave things as they are if you don't have time.

Patch

diff -r ff0d3b6b287f -r 0342a67d7073 mercurial/util.py
--- a/mercurial/util.py    Tue Mar 29 12:29:00 2016 -0500
+++ b/mercurial/util.py    Sun Apr 03 12:54:04 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
@@ -1588,12 +1585,14 @@ 
         format = format.replace("%z", "%1%2")
         format = format.replace("%1", "%c%02d" % (sign, q))
         format = format.replace("%2", "%02d" % r)
-    try:
-        t = time.gmtime(float(t) - tz)
-    except ValueError:
-        # time was out of range
-        t = time.gmtime(sys.maxint)
-    s = time.strftime(format, t)
+    d = t - tz
+    if abs(d) > 0x7fffffff:
+        d = cmp(d, 0) * 0x7fffffff
+    # 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=d)
+    s = t.strftime(format)
     return s

 def shortdate(date=None):
@@ -1714,8 +1713,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 0342a67d7073 tests/test-commit.t
--- a/tests/test-commit.t    Tue Mar 29 12:29:00 2016 -0500
+++ b/tests/test-commit.t    Sun Apr 03 12:54:04 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