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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 3, 2021, 04:42 UTC
Message-ID: <xmqqbl5agtid.fsf@gitster.g>
In-Reply-To: <YTFS2oQz9QEzBe0Q@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 8 quoted lines

>> 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.

Very true. Ah, so you mean the way this function is supposed to be used is to _call_ it, like so:

	if (!is_our_data_structure_healthy())
		BUG(...);

It makes it easier to reason about what the function is doing, I guess.

Show 6 quoted lines

> 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