Re: [PATCH 2/2] merge-ort: fix slightly overzealous assertion for rename-to-self

From: Elijah Newren <newren@gmail.com>
Date: Mar 13, 2025, 17:15 UTC
Message-ID: <CABPp-BF01a8bEZ2mfp7H_=HKsv6mSE4+ee8bi9D9xKWbH=0b5g@mail.gmail.com>
In-Reply-To: <xmqqbju4q39j.fsf@gitster.g>

On Thu, Mar 13, 2025 at 9:55 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 24 quoted lines

>
> Elijah Newren <newren@gmail.com> writes:
>
> > On Wed, Mar 12, 2025 at 4:18 PM Junio C Hamano <gitster@pobox.com> wrote:
> >>
> >> Elijah Newren <newren@gmail.com> writes:
> >>
> >> > But it might be worth mentioning that having side-effects in
> >> > assertions is a huge no-no, and I understand that when folks have to
> >> > debug one of those (I had to in a separate project years ago which was
> >> > kind of nasty), that they sometimes jump to the conclusion that
> >> > assertions are bad.
> >>
> >> Yes, assert() invites such mistakes.  Why not avoid them when there
> >> are better alternatives like "if (condition) BUG()"?
> >
> > I mean, I just gave my reasons above which you snipped out.  But if
> > you feel it is important for folks to move away from assert(), would
> > you like to see someone create a better alternative to assert, such as
> > BUG_ON(condition), so that they have reason to want to switch?
>
> You said "BUG_ON()" is better than "if (condition) BUG()", but I do
> not see a reason why.  It also shares the same problem with assert()
> if we make it honor NDEBUG.

"if (condition) BUG()" is invalid; it needs more arguments. "if (condition) BUG(something)" requires a separate "something", which requires awkward additional wording and/or is needlessly duplicative.

If you don't want to see assert in the codebase because of NDEBUG, then obviously we'd leave NDEBUG out of BUG_ON().

Such a BUG_ON() would be an improvement because it maintains the ergonomics of assert() while avoiding the potential mistake of accidentally including something with side-effects. "If (condition) BUG(something)" doesn't preserve the ergonomics and is thus worse, IMO. (Personally, I still like having cheap checks that are okay to compile out, but if we want a project directive against those, then I think a BUG_ON() that doesn't depend on NDEBUG would be the way to move in that direction.)