On 23/09/2019 09:37, SZEDER Gábor wrote:
On Sun, Sep 22, 2019 at 11:01:26PM +0200, Johannes Sixt wrote:
Am 22.09.19 um 21:53 schrieb SZEDER Gábor:
On Sun, Sep 22, 2019 at 07:57:36PM +0100, Phillip Wood wrote:
On 22/09/2019 19:01, SZEDER Gábor wrote:
+/*
+ * One day. See the 'name a rev close to epoch' test in t6120 when
+ * changing this value
+ */
+#define CUTOFF_DATE_SLOP 86400
typedef struct rev_name {
const char *tip_name;
@@ -481,8 +485,13 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
add_object_array(object, *argv, &revs);
}
- if (cutoff)
- cutoff = cutoff - CUTOFF_DATE_SLOP;
+ if (cutoff) {
+ /* check for undeflow */
+ if (cutoff - CUTOFF_DATE_SLOP < cutoff)
Nice catch but wouldn't this be clearer as
if (cutoff > CUTOFF_DATE_SLOP) ?
It would only be clearer now, with an unsigned 'timestamp_t'. I
tried to future-proof for a signed 'timestamp_t' and a cutoff date
before the UNIX epoch.
Huh? For signed cutoff and positive CUTOFF_DATE_SLOP,
cutoff - CUTOFF_DATE_SLOP < cutoff is ALWAYS true. Signed interger
underflow is undefined behavior and signed integer arithmetic does not
wrap around!
IOW, the new condition makes only sense today, because cutoff is an
unsigned type, but breaks down should we switch to a signed type.
Yeah, that's what I meant with worrying about signed underflow in the
commit message. As long as the cutoff is at least a day later than
the minimum value of our future signed 'timestamp_t', the condition
does the right thing. And considering that oldest time a signed 64
bit timestamp can represent far exceeds the age of the universe, and
the oldest value of even a signed 32 bit timestamp is almost half the
age of the Earth, I wasn't too worried.
It's still going to trip up static analysers though isn't it? Also it
will confuse readers who have to reason that it does not overflow in
practice as we (probably?) never use very negative values
Best Wishes
Phillip
You need this on top:
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index a4d8d312ab..2d83c2b172 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -487,10 +487,10 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
if (cutoff) {
/* check for undeflow */
- if (cutoff - CUTOFF_DATE_SLOP < cutoff)
+ if (cutoff > TIME_MIN + CUTOFF_DATE_SLOP)
cutoff = cutoff - CUTOFF_DATE_SLOP;
else
- cutoff = 0;
+ cutoff = TIME_MIN;
}
for_each_ref(name_ref, &data);
diff --git a/git-compat-util.h b/git-compat-util.h
index c68c61d07c..1bdc21a069 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -344,6 +344,7 @@ typedef uintmax_t timestamp_t;
#define PRItime PRIuMAX
#define parse_timestamp strtoumax
#define TIME_MAX UINTMAX_MAX
+#define TIME_MIN 0
#ifndef PATH_SEP
#define PATH_SEP ':'
