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.
> @@ -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?
> @@ -619,6 +626,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
> refresh_index_quietly();
> release_revisions(&rev);
> object_array_clear(&ent);
> + symdiff_release(&sdiff);
> UNLEAK(blob);
> return result;
> }
Other than that, this looks cleanly done. Thanks.
