Re: [PATCH 1/3] log-tree: take ownership of pointer

From: "Kristoffer Haugsbakk" <code@khaugsbakk.name>
To: "Jeff King" <peff@peff.net>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 1/3] log-tree: take ownership of pointer
Date: Wed, 13 Mar 2024 18:49:52 +0100	[thread overview]
Message-ID: <929692bc-c5f5-4dca-a96c-5b95603c3d26@app.fastmail.com> (raw)
In-Reply-To: <20240313065454.GB125150@coredump.intra.peff.net>

On Wed, Mar 13, 2024, at 07:54, Jeff King wrote:
> On Tue, Mar 12, 2024 at 06:43:55PM +0100, Kristoffer Haugsbakk wrote:
>
>> > Hmm, OK. This patch by itself introduces a memory leak. It would be nice
>> > if we could couple it with the matching free() so that we can see that
>> > the issue is fixed. It sounds like your patch 2 is going to introduce
>> > such a free, but I'm not sure it's complete.
>>
>> Is it okay if it is done in patch 2?
>
> I don't think it's the end of the world to do it in patch 2, as long as
> we end up in a good spot. But IMHO it's really hard for reviewers to
> understand what is going on, because it's intermingled with so many
> other changes. It would be much easier to read if we had a preparatory
> patch that switched the memory ownership of the field, and then built on
> top of that.

Sounds good. I’ll do that.

> But I recognize that sometimes that's hard to do, because the state is
> so tangled that the functional change is what untangles it. I'm not sure
> if that's the case here or not; you'd probably have a better idea as
> somebody who looked carefully at it recently.

Seems doable in this case.

By the way. I pretty much just elbowed in the changes I needed (like in
`revision.h`) in order to add this per-patch/cover letter headers
variable. Let me know if there are better ways to do it.

>> > It frees the old extra_headers before reassigning it, but nobody
>> > cleans it up after handling the final commit.
>>
>> I didn’t get any leak errors from the CI. `extra_headers` in `show_log`
>> is populated by calling `log_write_email_headers`. Then later it is
>> assigned to
>>
>>     ctx.after_subject = extra_headers;
>>
>> Then `ctx.after_subject is freed later
>>
>>     free((char *)ctx.after_subject);
>>
>> Am I missing something?
>
> Ah, I see. I was confused by looking for a free of an extra_headers
> field. We have rev_info.extra_headers, and that is _not_ owned by
> rev_info. We used to assign that to a variable in
> log_write_email_headers(), but now we actually make a copy of it. And so
> the copy is freed in that function when we replace it with a version
> containing extra mime headers here:
>
> [snip]
>
> But the actual ownership is passed out via the extra_headers_p variable,
> and that is what is assigned to ctx.after_subject (which now takes
> ownership).
>
> I think in the snippet I quoted above that extra_headers could never be
> NULL now, right? We'll always return at least an empty string. But
> moreover, we are formatting it into a strbuf, only to potentially copy
> it it another strbuf. Couldn't we just do it all in one strbuf?
>
> Something like this:
>
> [snip]
>
>
> The resulting code is shorter and (IMHO) easier to understand. It
> avoids an extra allocation and copy when using mime. It also avoids the
> allocation of an empty string when opt->extra_headers and
> opt->pe_headers are both NULL. It does make an extra copy when
> extra_headers is non-NULL but pe_headers is NULL (and you're not using
> MIME), as we could just use opt->extra_headers as-is, then. But since
> the caller needs to take ownership, we can't avoid that copy.
>
> I think you could even do this cleanup before adding pe_headers,
> especially if it was coupled with cleaning up the memory ownership
> issues.
>
> -Peff

I haven’t tried yet but this seems like a good plan. It was getting a
getting a bit too back and forth with my changes. So I’ll try to use
your patch and see if I can get a clean preparatory patch/commit before
the main change.

Cheers

-- 
Kristoffer Haugsbakk