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
