Re: [PATCH 3/3] diff.c: use diff_free_queue()

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Wed, Nov 02, 2022 at 08:24:09PM -0400, Taylor Blau wrote:
> On Wed, Nov 02, 2022 at 11:01:42PM +0100, SZEDER Gábor wrote:
> >  int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only)
> >  {
> >  	struct diff_queue_struct *q = &diff_queued_diff;
> > -	int i;
> >  	int result = diff_get_patch_id(options, oid, diff_header_only);
> >
> > -	for (i = 0; i < q->nr; i++)
> > -		diff_free_filepair(q->queue[i]);
> > -
> > -	free(q->queue);
> > +	diff_free_queue(q);
> >  	DIFF_QUEUE_CLEAR(q);
> 
> So, this all looks fine to me. But I did a quick grep around for
> DIFF_QUEUE_CLEAR(), and this macro is used in quite a few places.
> Mostly, as far as I can tell, to "empty" out the diff-queue by setting
> its 'queue' pointer to NULL, and its 'nr' back to 0.
> 
> Should we be freeing the memory held by the queue there more
> aggressively? I.e., should we make sure that there is a
> diff_free_queue() call above each expansion of the DIFF_QUEUE_CLEAR()
> macro?

Definitely not.  DIFF_QUEUE_CLEAR is often used to initialize a just
created 'struct diff_queue_struct' instance; by adding a
diff_free_queue() above those it would operate on uninitialized
memory.