On Fri, 25 Jan 2019 at 20:51, Jeff King <peff@xxxxxxxx> 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? > > > 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
