On Sun, Sep 22, 2019 at 07:57:36PM +0100, Phillip Wood wrote: > On 22/09/2019 19:01, SZEDER Gábor wrote: > >When 'git name-rev' is invoked with commit-ish parameters, it tries to > >save some work, and doesn't visit commits older than the committer > >date of the oldest given commit minus a one day worth of slop. Since > >our 'timestamp_t' is an unsigned type, this leads to a timestamp > >underflow when the committer date of the oldest given commit is within > >a day of the UNIX epoch. As a result the cutoff timestamp ends up > >far-far in the future, and 'git name-rev' doesn't visit any commits, > >and names each given commit as 'undefined'. > > > >Check whether substacting the slop from the oldest committer date > >would lead to an underflow, and use a 0 as cutoff in that case. This > >way it will handle commits shortly after the epoch even if we were to > >switch to a signed 'timestamp_t' (but then we'll have to worry about > >signed underflow for very old commits). > > > >Note that the type of the cutoff timestamp variable used to be signed > >before 5589e87fd8 (name-rev: change a "long" variable to timestamp_t, > >2017-05-20). The behavior was still the same even back then, but the > >underflow didn't happen when substracting the slop from the oldest > >committer date, but when comparing the signed cutoff timestamp with > >unsigned committer dates in name_rev(). IOW, this underflow bug is as > >old as 'git name-rev' itself. > > > >Signed-off-by: SZEDER Gábor <szeder.dev@xxxxxxxxx> > >--- > > > >This patch adds a test at the end of 't6120-describe.sh', so it will > >conflict with my non-recursive name-rev patch series, which adds a > >test there as well, but the conflict should be wasy to resolve. > > > > https://public-inbox.org/git/20190919214712.7348-7-szeder.dev@xxxxxxxxx/ > > > > builtin/name-rev.c | 15 ++++++++++++--- > > t/t6120-describe.sh | 15 +++++++++++++++ > > 2 files changed, 27 insertions(+), 3 deletions(-) > > > >diff --git a/builtin/name-rev.c b/builtin/name-rev.c > >index c785fe16ba..a4d8d312ab 100644 > >--- a/builtin/name-rev.c > >+++ b/builtin/name-rev.c > >@@ -9,7 +9,11 @@ > > #include "sha1-lookup.h" > > #include "commit-slab.h" > >-#define CUTOFF_DATE_SLOP 86400 /* one day */ > >+/* > >+ * 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. > >+ cutoff = cutoff - CUTOFF_DATE_SLOP; > >+ else > >+ cutoff = 0; > >+ }
