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:

quoted

On Wed, Nov 02, 2022 at 11:01:42PM +0100, SZEDER Gábor wrote:

quoted

 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