On Fri, Jan 25, 2019 at 08:24:35PM +0100, Martin Ågren wrote: > On Wed, 23 Jan 2019 at 06:57, Jeff King <peff@xxxxxxxx> 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
