On Mon, Feb 24, 2025 at 08:14:43AM -0800, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> writes:
>
> > On Fri, Feb 07, 2025 at 12:03:36PM +0100, Patrick Steinhardt wrote:
> >
> >> static timestamp_t rerere_last_used_at(struct rerere_id *id)
> >> {
> >> + struct strbuf buf = STRBUF_INIT;
> >> struct stat st;
> >> + int ret;
> >> +
> >> + ret = stat(rerere_path(&buf, id, "postimage"), &st) ? (time_t) 0 : st.st_mtime;
> >>
> >> - return stat(rerere_path(id, "postimage"), &st) ? (time_t) 0 : st.st_mtime;
> >> + strbuf_release(&buf);
> >> + return ret;
> >> }
> >
> > This "int ret" should surely be a timestamp_t, no?
> >
> > -Peff
>
> Indeeeeeed. Thanks for a careful reading.
Less reading, and more merge resolution. ;) (I had a similar series to
Patrick's that was half-done from a few months ago).
I see you added a fixup! to the topic. Note that rerere_created_at()
needs the same treatment, too.
One side note: using timestamp_t here should get us the same behavior
that the original had before this patch. But I'm not sure the original
was entirely correct. st_mtime is a time_t, so we are assuming the
implicit cast is OK. Our timestamp_t tries to be at least as long as
time_t, so I think we are OK for the future. For very old timestamps, it
is probably wrong (since time_t is usually signed, and timestamp_t is
not yet).
It's mostly academic, though, unless your filesystem has rerere files
before 1970. So I think we can probably just ignore it (and I do still
hope eventually to support negative values with timestamp_t).
-Peff
