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
