On Wed, May 25, 2022 at 09:30:55AM -0400, Derrick Stolee wrote: > On 5/25/2022 5:11 AM, Ævar Arnfjörð Bjarmason wrote: > > I must say that I really don't like this part of the format. Is it > > really necessary to optimize the storage space here in a way that leaves > > open questions about future time_t compatibility, and having to > > introduce the first use of unsigned 32 bit timestamps to git's codebase? > > The commit-graph file format uses unsigned 34-bit timestamps (packed > with 30-bit topological levels in the CDAT chunk), so this "not-64-bit > signed timestamps" thing is something we've done before. > > > Yes, this is its own self-contained format, so we don't *need* time_t > > here, but it's also really handy if we can eventually consistently use > > 64 time_t everywhere and not worry about any compatibility issues, or > > unsigned v.s. signed, or to create our own little ext4-like signed 32 > > bit timestamp format. > > We can also use a new file format version when it is necessary. We > have a lot of time to add that detail without overly complicating the > format right now. > > > If we really are trying to micro-optimize storage space here I'm willing > > to bet that this is still a bad/premature optimization. There's much > > better ways to store this sort of data in a compact way if that's the > > concern. E.g. you'd store a 64 bit "base" timestamp in the header for > > the first entry, and have smaller (signed) "delta" timestamps storing > > offsets from that "base" timestamp. > > This is a good idea for a v2 format when that is necessary. I agree here. I'm not opposed to such a change (or even being the one to work on it!), but I would encourage us to pursue that change outside of this series, since it can easily be done on top. Of course, if we ever did decide to implement 64-bit mtimes, we would have to maintain support for reading both the 32-bit and 64-bit values. But I think the code is well-equipped to do that, and it could be done on top without significant additional complexity. Thanks, Taylor
