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.
> 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 ':'
