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

From: Matthew DeVore <matvore@comcast.net>
Date: Jun 12, 2019, 00:37 UTC
Message-ID: <20190612003716.GD58112@comcast.net>
In-Reply-To: <xmqqmuinkd30.fsf@gitster-ct.c.googlers.com>

On Tue, Jun 11, 2019 at 02:48:51PM -0700, Junio C Hamano wrote:

Show 24 quoted lines

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