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, 22:19 UTC
Message-ID: <20250224221914.GA193356@coredump.intra.peff.net>
In-Reply-To: <xmqqr03n7218.fsf@gitster.g>

On Mon, Feb 24, 2025 at 08:14:43AM -0800, Junio C Hamano wrote:

Show 22 quoted lines

> Jeff King <peff@peff.net> 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