From: Junio C Hamano <gitster@pobox•com>
To: Jeff King <peff@peff•net>
Cc: Kristoffer Haugsbakk <code@khaugsbakk•name>,
Phillip Wood <phillip.wood@dunelm•org.uk>,
git@vger•kernel.org
Subject: Re: [BUG] rebase: can write reflog with uninit. `action` string
Date: Thu, 01 May 2025 09:07:06 -0700 [thread overview]
Message-ID: <xmqq4iy4z55h.fsf@gitster.g> (raw)
In-Reply-To: <20250501131751.GA1725607@coredump.intra.peff.net> (Jeff King's message of "Thu, 1 May 2025 09:17:51 -0400")
Jeff King <peff@peff•net> 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.
;-)
next prev parent reply other threads:[~2025-05-01 16:07 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-04-28 19:40 [BUG] rebase: can write reflog with uninit. `action` string Kristoffer Haugsbakk
2025-04-29 9:22 ` Phillip Wood
2025-04-29 19:40 ` Kristoffer Haugsbakk
2025-04-29 21:51 ` Jeff King
2025-04-30 15:17 ` Kristoffer Haugsbakk
2025-05-01 13:17 ` Jeff King
2025-05-01 14:36 ` phillip.wood123
2025-05-01 15:36 ` Jeff King
2025-05-01 16:07 ` Junio C Hamano [this message]
2025-05-01 16:38 ` Jeff King
2025-05-01 17:53 ` Junio C Hamano
2025-05-01 14:10 ` phillip.wood123
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq4iy4z55h.fsf@gitster.g \
--to=gitster@pobox$(echo .)com \
--cc=code@khaugsbakk$(echo .)name \
--cc=git@vger$(echo .)kernel.org \
--cc=peff@peff$(echo .)net \
--cc=phillip.wood@dunelm$(echo .)org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox