Re: [PATCH v3 22/22] builtin/diff: free symmetric diff members
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 13, 2024, 16:25 UTC
- Message-ID
- <xmqq1q2s8k56.fsf@gitster.g>
- In-Reply-To
- <31e38ba4e150c9bc9e3aa1073869881ccba9035e.1723540931.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 7 quoted lines
> 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.
Show 6 quoted lines
> +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.
Show 6 quoted lines
> @@ -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?
Show 8 quoted lines
> @@ -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.