Re: [PATCH v2 11/16] rerere: let `rerere_path()` write paths into a caller-provided buffer

From: Jeff King <peff@peff.net>
Date: Feb 24, 2025, 23:10 UTC
Message-ID: <20250224231024.GA729825@coredump.intra.peff.net>
In-Reply-To: <xmqqh64j3qkn.fsf@gitster.g>

On Mon, Feb 24, 2025 at 02:50:32PM -0800, Junio C Hamano wrote:

Show 18 quoted lines

> 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