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

[Pierre-Yves David]

On 04/03/2016 03:56 AM, Florent Gallaire wrote:
> # HG changeset patch
> # User Florent Gallaire <fgallaire at 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,

-- 
Pierre-Yves David