On Mon, Feb 24, 2025 at 02:50:32PM -0800, Junio C Hamano wrote: > I do not think timestamp_t is appropriate for rerere records, > actually. The reason why we have timestamp_t is for things like the > author dates that can be arbitrarily and deliberately set to any > historical times, e.g. long before the committer was born. > > Unlike that, the timestamps we are dealing with with rerere records > are the times on the filesystem when these rerere records were > created and/or used so whatever stat() gives us for st_mtime > (i.e. time_t) is a lot more appropriate. > > I'd probably leave a #leftoverbits here; we should vet our use of > timestamp_t to see if we are not overusing the type. Roughly, the > timestamps we may record in the commit and the tag objects should be > timestamp_t, but the time we get from the filesystem and only > compared with another timestamp the same way should use appropriate > system-defined type, which is likely to be time_t, as not everybody > may have struct timespec, and file expiration should not need > nanoseconds precision. Yes, I was going to suggest using time_t (or even timespec) consistently here, but...I think it is a little trickier than that. We will compare these mtimes not just with themselves, but with a cutoff time that we get from the user, via repo_config_get_expiry_in_days(). And that's parsing lots of formats into a timestamp_t. So either we need a parallel universe of functions that operate on time_t (to use for things we expect to only interact with filesystem times), or at some point we have to interact between time_t and timestamp_t. I hope you'll agree that that the "parallel universe" direction is too gross to contemplate. ;) So the only question is how and when to convert from time_t to timestamp_t. I think our general strategy for when has been "as soon as possible" which makes sense to me (most of the code only has to deal with our one type). So I think doing it right after the stat() here makes sense. The "how" should probably not be "do an implicit cast". We have checks for overflow, etc, in gm_time_t(), though I'm not sure what the error handling should be for a case like this (that function just calls die(), which might not be ideal). All of which is to say it might be a bit more than a #leftoverbits. -Peff
