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:
> >
> > On Mon, Nov 07 2022, SZEDER Gábor wrote:
> >
> > > On Wed, Nov 02, 2022 at 08:20:21PM -0400, Taylor Blau wrote:
> > >> > +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
