Re: [PATCH v2] sequencer: fix edit handling for cherry-pick and revert messages

From: Elijah Newren <newren@gmail.com>
Date: Apr 1, 2021, 16:31 UTC
Message-ID: <CABPp-BFLyYTboiGkP=sc7S+stRZvozAqnJPnza+M0q2nakvD2Q@mail.gmail.com>
In-Reply-To: <xmqqmtuj8ank.fsf@gitster.g>

On Wed, Mar 31, 2021 at 11:01 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 17 quoted lines

>
> Elijah Newren <newren@gmail.com> writes:
>
> > ... would you two prefer
> > that I add a new affirm() function that is ALSO compiled out in
> > production?
>
> The reason I do not think affirm would fly is because it shares the
> most problematic trait with assert(): it can be comipled out.
>
> There was at least one bug we found and fixed in a piece of code
> that came later in a codepath that had an assert() in it.  The buggy
> code (IIRC, it was added way after the assert() was written in an
> unrelated patch) was unknowingly depending on a side-effect of an
> innocuous function call (i.e. assert(func_tion(args) == expected))
> made in the assert condition, and the bug did not reproduce in the
> developer's environment.

Ah, now that's a much different argument than claiming assert is useless. I had to debug one of those once about a decade ago, in a different project, put in by some other developer, and yeah it's painful.

If that causes assert() to be frowned upon, then I can understand based on this reason. If so, perhaps a BUG_ON() function would be useful? The "if (...) BUG()" construct causes line coverage problems and I find it an onerous way to signal to future code readers "hey, just as a reminder, these things should be true at this point". If you need to signal something more than that or explain why the things being false is a problem or what might cause it, then the "if (...) BUG()" construct is useful and there are certainly many places for that, but it just doesn't fit all the cases.