From: Junio C Hamano Date: Mon, 10 Jun 2019 20:13:54 GMT Subject: Re: [PATCH v2 6/9] list-objects-filter-options: make filter_spec a strbuf Message-ID: In-Reply-To: <20190601003603.90794-7-matvore@google.com> Matthew DeVore writes: > - filter_options->filter_spec = strdup(core_partial_clone_filter_default); > + if (!filter_options->filter_spec.buf) > + strbuf_init(&filter_options->filter_spec, 0); This part made me go "Huh?" a bit. Do we document that .buf==NULL means an uninitialized strbuf that is safe to run strbuf_init() on? I do not mind that as a general convention, and it may even be a useful one (i.e. it allows you to calloc() a structure with an embedded strbuf in it and the "if .buf==NULL, call strbuf_init() lazily" can become an established pattern), but at the same time it feels a bit brittle. Such a convention forces everybody who might want to use such an embedded strbuf to first check .buf==NULL and lazily initialize it---and at some point when the embedded strbuf to be used by enough codepaths, it would make the code more robust by giving up on the lazy initialization (iow, when *filter_options is initialized, run strbuf_init() on its .filter_spec field).