On Tue, Aug 13, 2024 at 09:25:41AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@xxxxxx> writes:
>
> > We populate a `struct symdiff` in case the user has requested a
> > symmetric diff. Part of this is to populate a `skip` bitmap that
> > indicates which commits shall be ignored in the diff. But while this
> > bitmap is dynamically allocated, we never free it.
> >
> > Fix this by introducing and calling a new `symdiff_release()` function
> > that does this for us.
>
> OK.
>
> > +static void symdiff_release(struct symdiff *sdiff)
> > +{
> > + if (!sdiff)
> > + return;
> > + bitmap_free(sdiff->skip);
> > +}
>
> Hmph, wouldn't it be a BUG if any caller feeds a NULL pointer to it,
> though? When symdiff was prepared but not used, sdiff->skip will be
> NULL but sdiff is never NULL even in such a case.
Good point. It does make sense for `_free()` functions to handle NULL
pointers, but doesn't quite for `_release()` ones.
> > @@ -398,7 +405,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
> > struct object_array_entry *blob[2];
> > int nongit = 0, no_index = 0;
> > int result;
> > - struct symdiff sdiff;
> > + struct symdiff sdiff = {0};
>
> And symdiff_prepare() at least clears its .skip member to NULL, so
> this pre-initialization is probably not needed. If we are preparing
> ourselves for future changes of the flow in this function (e.g.
> goto's that jump to the clean-up label from which symdiff_release()
> is always called, even when we did not call symdiff_prepare() on
> this thing), this is probably not sufficient to convey that
> intention (instead I'd use an explicit ".skip = NULL" to say "we
> might not even call _prepare() but this one is prepared to be passed
> to _release() even in such a case").
>
> Given that there is no such goto exists, and that _prepare() always
> sets up the .skip member appropriately, I wonder if we are much
> better off leaving sdiff uninitialized at the declaration site here.
> If we add such a goto that bypasses _prepare() in the future, the
> compiler will notice that we are passing an uninitialized sdiff to
> _release(), no?
You'd hope it does, but it certainly depends on your compiler flags.
Various hardening flags for example implicitly initialize variables, and
I have a feeling that this also causes them to not emit any warnings
anymore. At least I only spot such warnings in CI.
In any case, yes, we can drop the initialization here.
Patrick
