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

[Jeff King <peff@peff.net>]

On Fri, Jan 25, 2019 at 08:24:35PM +0100, Martin Ågren wrote:

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

I'm less worried about out-of-tree users, and more concerned with just
having a calling convention that matches usual conventions (and is
harder to get wrong).

It's a pretty minor point, though, so I can live with it either way.

-Peff