Re: commit-graph overflow generation chicken and egg

From: Jeff King <peff@peff.net>
Date: Jul 6, 2022, 09:11 UTC
Message-ID: <YsVRq82ewvvIyxDT@coredump.intra.peff.net>
In-Reply-To: <a154e109-3f4c-c500-3365-d47879abf30d@github.com>

On Mon, Jul 04, 2022 at 02:50:57PM -0600, Derrick Stolee wrote:

Show 7 quoted lines

> > + GIT_COMMITTER_DATE='1970-01-01T00:00:00 +0100'
> 
> Because of the "+0100" in the time zone, this date
> becomes a negative value. The commit-graph does not
> store dates with more than 34 bits (and Git does
> not handle negative timestamps very well? Peff can
> clarify here).

I don't think we handle them at all. Certainly we'll refuse to parse any, but here we get tricked by the timezone, and end up with the underflow. Git writes 2^64 - 3600 into the commit object with this one.

The code in gm_time_t() tries to catch this (as well as overflow), but this one gets handled by parse_date_basic(), which doesn't. Adding this causes it barf:

diff --git a/date.c b/date.c
index 68a260c214..07a60fee35 100644
--- a/date.c
+++ b/date.c
@@ -893,8 +893,11 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)
 		}
 	}
 
-	if (!tm_gmt)
+	if (!tm_gmt) {
+		if (*timestamp < *offset * 60)
+			return -1;
 		*timestamp -= *offset * 60;
+	}
 	return 0; /* success */
 }
 

We should probably check for overflow there, as well, though I think in
practice it would get caught earlier. The timestamp variable comes from
tm_to_time_t(), which already has some limits.

Of course the right fix is for us to handle negative timestamps. I have
some patches working towards that, but...

> The commit-graph could certainly warn better here to
> say we do not have enough date bits to store this
> timestamp (the same would happen with a date beyond
> 2138 or something like that).

Right. By the time it gets to the commit-graph, it's a gigantic positive
time. But either way, yes, it should realize it's not representable and
omit the commit from the graph file.

That's something I haven't looked at for my negative timestamp patches
at all. I think it's OK for the commit-graph to simply punt on them, as
long as it doesn't die or produce wrong answers.

-Peff