Re: [PATCH 3/3] fixup! reftable: add a heap-based priority queue for reftable records

From: Jeff King <peff@peff.net>
Date: Sep 2, 2021, 22:40 UTC
Message-ID: <YTFS2oQz9QEzBe0Q@coredump.intra.peff.net>
In-Reply-To: <xmqqeea6kaed.fsf@gitster.g>

On Thu, Sep 02, 2021 at 01:08:58PM -0700, Junio C Hamano wrote:

Show 11 quoted lines

> > This will trigger -Wunused-parameter warnings, since the function body
> > is now empty when NDEBUG is undefined. Probably switching the assert()
> > to die() would be better, since the whole point of the function is just
> > to exit on error.
> >
> > If there's a problem using die() from the reftable code, it could also
> > return an error and the caller in the test helper could propagate it.
> 
> I agree that the patch as posted does not help but if this is
> originally an assertion, then it should never trigger in real life,
> so BUG() would be more appropriate than an error return, no?

My thinking was that it doesn't make much sense as an assertion in the first place. It is not a side effect of "let's make sure things are as we expect while we're doing some other operation". The whole point of the function is: is this data structure properly in order.

But I guess you could argue that calling the function is itself a form of assertion. I don't really care that much either way, so whatever Han-Wen prefers is fine with me (but I do think it is worth addressing the warning Carlo found _somehow_).

-Peff