Re: commit-graph overflow generation chicken and egg

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

quoted

+ 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