On Wed, Apr 30, 2025 at 04:10:37PM -0700, Junio C Hamano wrote:

Junio C Hamano [off-list ref] writes:

quoted

Patrick Steinhardt [off-list ref] writes:

quoted

The use of asserts is discouraged in our codebase because they lead to
different behaviour depending on how Git is built. When being unsure
enough whether a condition always holds so that one adds the assert,
then the assert should probably trigger regardless of how Git is being
built.

Nicely put.  Yes, this is another reason why we frown on the use of
assert(), in addition to the reason why why Elijah's series that
ends with 5633aa3a (treewide: replace assert() with ASSERT() in
special cases, 2025-03-19) was written.

quoted

Drop the call to assert(3p) in git-mv(1) and instead use `BUG()`.

Being explicit about what we are unsure about is always good.  It
would hopefully entice those who want to get their hands dirty to
see if they can "prove" that BUG() would never happen, which would
be a great outcome ;-).

By the way, with this in place, and without Dscho's "assert() makes
Win+Meson test job get stuck, so let's make assert() a no-op" patch,
the CI seems to be fine.

    https://github.com/git/git/actions/runs/14765572702

Triggering assert() and BUG() are something we would always want to
notice.  They should never trigger in production and it is an event
to call for fixing the underlying cause that made the condition
trigger if it is shown to end-users.  Dscho's patch protects us from
addition of a new test that triggers an assert().  We won't see such
a test get stuck forever on Windows, but by turning such an assert()
into a no-op, we would waste electricity for running CI only to miss
the triggering assert(), which does not sound like a good use of our
resources.

It makes me wonder whether we should forbid `assert()` altogether and
use `BUG()` everywhere, similar to the recent discussion with Elijah. We
do have >600 callsites of `assert()` though, so we would have to
introduce a macro that doesn't require us to provide a reasoning for
now. E.g.

    #define BUG_UNLESS(condition) if (!(condition)) BUG(##condition)

or something like this.

And once we've done such a conversion we could add `assert()` to our
deny list of functions (wherever it was, I forgot).

So I am inclined to drop Dscho's "build in release mode" patch when
we merge this series down to 'next'.  Being able to notice a
breakage (which triggers a real assert(), whether it is due to
broken code, or due to a broken test that documents a broken code
path---which should be rewritten to use "if (condition) BUG()"),
even if it needs to be done by noticing a test that gets stuck,
would be much better than missing such a breakage at all, and that
is the primary reasoning behind my suggesting to do so.  I would not
be surprised if I am missing a good reason or two to make build
tested in CI ignore asserts, so let's hear from others.

Opinions?

As far as I understand there is no need for this patch anymore.

Patrick