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