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

[Taylor Blau <me@ttaylorr.com>]

On Mon, Nov 07, 2022 at 05:13:11PM +0100, SZEDER Gábor wrote:
> 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.

Thanks for pointing it out.

Thanks,
Taylor