On Thu, Sep 02, 2021 at 01:08:58PM -0700, Junio C Hamano wrote: > > 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
