Re: [PATCH v2 6/9] list-objects-filter-options: make filter_spec a strbuf

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 10, 2019, 20:13 UTC
Message-ID: <xmqqimtdmc59.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190601003603.90794-7-matvore@google.com>

Matthew DeVore <matvore@google.com> 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).