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

From: Martin Ågren <martin.agren@gmail.com>
Date: Feb 25, 2019, 19:21 UTC
Message-ID: <CAN0heSq=0=B-FoPWcuRxp0oJ6capo3Pu4txZZOP2uDeNn5N1Og@mail.gmail.com>
In-Reply-To: <20190125195107.GA6520@sigill.intra.peff.net>

On Fri, 25 Jan 2019 at 20:51, Jeff King <peff@peff.net> wrote:

Show 12 quoted lines

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

Show 9 quoted lines

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

It's time to resurrect this thread. I've reworked this patch to avoid the compound literal when re-initing a struct, and I've been going back and forth on this point about having to initialize to `..._INIT` or risk crashing. And I keep coming back to thinking that it's not *that* different from how `STRBUF_INIT` works.

There's the obvious difference that there aren't as many functions to call, so there's certainly a difference in scale. And you'd think that you'll always start with `read_...()`, but another potential first function to call is `clear_...()` (see builtin/init-db.c), in which case you better have used `..._INIT` first.

I'm tempted to address this point by documenting as good as I can in the .h-file that one has to use this initializer macro. I'll obviously convert all users, so copy-paste programming should work fine...

How does that sound to you?

Martin