Subject: Re: [BUG] rebase: can write reflog with uninit. `action` string

From: Junio C Hamano <[email-masked]>
Date: Thu, 01 May 2025 09:07:06 -0700

Jeff King <[email-masked]> writes:

> Probably the smallest solution is for ctx->reflog_message to copy the
> result and always own the memory (and then remember to free it, both at
> cleanup and if it is ever overwritten).
>
> But I think the way reflog_message() returns the "buf" member of a
> static strbuf is kind of an anti-pattern, exactly because you can get
> this kind of subtle re-use. It probably should just return a non-const
> pointer, handing over memory ownership to the caller. That would require
> adjusting its other callers, too.

Yeah, yesterday I was looking at the same report and was thinking
that the memory ownership model here is somewhat screwed up.  As
a few more allocations/deallocations should be dwarfed by the real
processing cost of replaying each commit, I think this "smallest"
solution is also the solution in the right direction.

> So the "smallest" version is perhaps something like this, totally
> untested except for confirming that t3430 no longer complains:

;-)

>  	va_start(ap, fmt);
> -	strbuf_reset(&buf);
> +	strbuf_release(&buf); /* guarantees realloaction */

I initially thought that this comment may have to be updated in the
production version, but because we have to freshly allocate for each
new message for ownership change, this comment still is correct.
The only difference between the "here is how to expose" and "this is
part of the smallest solution" is why we want to guarantee it.

> I'm hoping your or Phillip can decide on the best fix from here.

;-)