On Mon, Nov 07, 2022 at 04:57:21PM +0100, SZEDER Gábor wrote:

On Mon, Nov 07, 2022 at 04:29:39PM +0100, Ævar Arnfjörð Bjarmason wrote:

quoted

On Mon, Nov 07 2022, SZEDER Gábor wrote:

quoted

On Wed, Nov 02, 2022 at 08:20:21PM -0400, Taylor Blau wrote:

quoted

quoted

+void diff_free_queue(struct diff_queue_struct *q)
+{
+	for (int i = 0; i < q->nr; i++)
+		diff_free_filepair(q->queue[i]);
+	free(q->queue);
+}

Though I wonder, should diff_free_queue() be a noop when q is NULL? The
caller in process_ranges_ordinary_commit() doesn't care, of course,
since q is always non-NULL there.

But if we're making it part of the diff API, we should probably err on
the side of flexibility.

On one hand, strbuf_reset(), string_list_clear(), or strvec_clear()
would all segfault on a NULL strbuf, string_list, or strvec pointer.

But the reason we do that is because those APIs will always ensure that
the struct is never in an inconsistent state, as opposed to the
destructor you're adding here.

Taylor's suggestion quoted above is not about the internal state of
the diff queue, but about a NULL pointer passed to diff_free_queue().

I think your perspective that strbuf_reset(), string_list_clear(), etc.
all segfault on a NULL argument is extremely reasonable. Let's start
merging this down.

Thanks,
Taylor