From: René Scharfe <l.s.r@web.de>
Date: Wed, 26 Apr 2017 22:09:17 GMT
Subject: Re: [PATCH v5 0/8] Introduce timestamp_t for timestamps
Message-ID: <b6302244-7425-605e-8103-9aa845c03c3e@web.de>
In-Reply-To: <alpine.DEB.2.20.1704260005310.3480@virtualbox>

Hi Dscho,

Am 26.04.2017 um 00:22 schrieb Johannes Schindelin:
> On Tue, 25 Apr 2017, René Scharfe wrote:
>> Am 24.04.2017 um 15:57 schrieb Johannes Schindelin:
>> Can we leave time_t alone and just do the part where you replace
>> unsigned long with timestamp_t defined as uint64_t?  That should already
>> help on Windows, correct?  When/if timestamp_t is later changed to a
>> signed type then we could easily convert the time_t cases to timestamp_t
>> as well, or the other way around.
> 
> This patch series leaves time_t alone already, so your wish has been
> fulfilled preemptively.

Sounds good!  It does contain conversions from time_t to timestamp_t in
archive-zip.c, though.  I'll comment in reply to the relevant patch.

>>> It is arguably a bug to paper over too-large author/committer dates and
>>> to replace them with Jan 1 1970 without even telling the user that we do
>>> that, but this is the behavior that t4212 verifies, so I reinstated that
>>> behavior. The change in behavior was missed because of the missing
>>> unsigned_add_overflows() test.
>>
>> I can't think of many ways to get future time stamps (broken clock,
>> broken CMOS battery, bit rot, time travel), so I wouldn't expect a
>> change towards better error reporting to affect a lot of users.  (Not
>> necessarily as part of this series, of course.)
> 
> If you want to suggest that we should stop verifying overflows when a
> complex reasoning can prove that the overflow is not happening in a
> billion years, I disagree. Not only is it unnecessarily time-consuming to
> ask readers to perform the complex reasoning, and not only is there enough
> room for bugs to hide in plain sight (because of the complexity), it also
> makes the same code harder to reuse in other software where a different
> timestamp data type was chosen (or inherited from previous Git versions).
> 
> I'd much rather have easy-to-reason code that does not cause head
> scratching (like the "why do we ignore a too large timestamp?" triggering
> `if (date_overflows(date)) date = 0;`) than pretending to be smart and
> clever and make everybody else feel stupid by forcing them through hoops
> of thinking bubbles until they also reached the conclusion that this
> actually won't happen. Unless there is a bug in the code.

No, I meant that the presence of tests does not necessarily cast an
undesirable behavior into stone, especially if it's hard to trigger
in real life.  And that improvements thereof can be done later.  In
other words: It's OK to maintain the same behavior in this series,
but cheer up, we may be able to fix the issue eventually.

René