Re: [PATCH v3 2/2] setup: fix memory leaks with `struct repository_format`

["Martin Ågren" <martin.agren@gmail.com>]

On Wed, 23 Jan 2019 at 06:57, Jeff King <peff@peff.net> wrote:
>
> On Tue, Jan 22, 2019 at 10:45:48PM +0100, Martin Ågren wrote:
>
> > Call `clear_...()` at the start of `read_...()` instead of just zeroing
> > the struct, since we sometimes enter the function multiple times. This
> > means that it is important to initialize the struct before calling
> > `read_...()`, so document that.
>
> This part is a little counter-intuitive to me. Is anybody ever going to
> pass in anything except a struct initialized to REPOSITORY_FORMAT_INIT?

I do update all users in git.git, but yeah, out-of-tree users and
in-flight topics would segfault.

> If so, might it be kinder for read_...() to not assume anything about
> the incoming struct, and initialize it from scratch? I.e., not to use
> clear() but just do the initialization step?

I have some vague memory from going down that route and giving up. Now
that I'm looking at it again, I think we can at least try to do
something. We can make sure that "external" users that call into setup.c
are fine (they'll leak, but won't crash). Out-of-tree users inside
setup.c will still be able to trip on this. I don't have much spare time
over the next few days, but I'll get to this.

Or we could accept that we may leak when we end up calling `read()`
multiple times (I could catch all leaks now, but new ones might sneak in
after that) and come back to this after X months, when we can perhaps
afford to be a bit more aggressive.

I guess we could just rename the struct to have the compiler catch
out-of-tree users...

> A caller which calls read_() multiple times would presumably have an
> intervening clear (either their own, or the one done on an error return
> from the read function).
>
> Other than that minor nit, I like the overall shape of this.

Thank you.

Martin