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.
