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

From: Junio C Hamano <gitster@pobox.com>
Date: May 1, 2025, 16:07 UTC
Message-ID: <xmqq4iy4z55h.fsf@gitster.g>
In-Reply-To: <20250501131751.GA1725607@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 9 quoted lines

> 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.

;-)