Re: [PATCH v3 22/22] builtin/diff: free symmetric diff members

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> 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.