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

From: Matthew DeVore <matvore@comcast.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: Matthew DeVore <matvore@google.com>,
	git@vger.kernel.org, jonathantanmy@google.com, jrn@google.com,
	dstolee@microsoft.com, jeffhost@microsoft.com,
	jrnieder@gmail.com, pclouds@gmail.com, emilyshaffer@google.com
Subject: Re: [PATCH v2 6/9] list-objects-filter-options: make filter_spec a strbuf
Date: Tue, 11 Jun 2019 17:37:16 -0700	[thread overview]
Message-ID: <20190612003716.GD58112@comcast.net> (raw)
In-Reply-To: <xmqqmuinkd30.fsf@gitster-ct.c.googlers.com>

On Tue, Jun 11, 2019 at 02:48:51PM -0700, Junio C Hamano wrote:
> Matthew DeVore <matvore@comcast.net> writes:
> 
> >> It is brittle because callers are bound to forget doing "if
> >> (!x->buf.buf) lazy_init(&x->buf)" at some point, and blindly use an
> >> uninitialized x->buf.  Making sure x->buf is always initialized
> >
> > A corallary proposition would be to make this particular strbuf a "struct
> > strbuf *" rather than an inline strbuf. It should then be rather clear to users
> > that it may be null.
> 
> Would make it less likely for uses of an uninitialized strbuf to be
> left undetected as errors?  I guess so, and if that is the case it
> would definitely be an improvement.
> 
> But initializing the strbuf at the point where the enclosing
> structure is initialized (or calloc()'ed) is also a vaiable option,
> and between the two, I think that would be even more robust.
> 
> There may be reasons why it is cumbersome to arrange it that way,
> though (e.g. if the code does not introduce a "new_stuff()"
> allocator that also initializes, and instead uses xcalloc() from
> many places, initializing the enclosing structure properly might
> take a preliminary clean-up step before the main part of the patch
> series can begin).

These are all the locations where a struct which ultimately contains a
list_objects_filter_options is instantiated:

GLOBAL VARIABLES:

builtin/clone.c:68:static struct list_objects_filter_options filter_options;
builtin/fetch.c:66:static struct list_objects_filter_options filter_options;
builtin/pack-objects.c:112:static struct list_objects_filter_options filter_options;
builtin/rev-list.c:65:static struct list_objects_filter_options filter_options;

LOCAL VARIABLES:

builtin/fetch-pack.c:54:        struct fetch_pack_args args;
transport.c:327:        struct fetch_pack_args args;

HEAP ALLOCATIONS:

transport-helper.c:1123:	struct helper_data *data = xcalloc(1, sizeof(*data));
transport.c:964:                struct git_transport_data *data = xcalloc(1, sizeof(*data));

git_transport_options is also not directly instantiated as a local or static
variable, but it would need to have a git_transport_options_init function
defined.

I didn't count exactly the number of _INIT macros and _init functions that
would need to be defined. It seems like a lot of work. It is hard to believe
that our ability to exhaustively pinpoint all these instantiations, and to
catch ALL future instantiations, is all that reliable. I think our ability to
find the places we need to lazily instantiate the strbuf-containing-struct
(struct filter_spec in the interdiff) is more reliable.