Re: [PATCH 2/2] builtin/mv: convert assert(3p) into `BUG()`

From: Patrick Steinhardt <ps@pks.im>
Date: May 2, 2025, 08:06 UTC
Message-ID: <aBR86Ct8mMUN_tzk@pks.im>
In-Reply-To: <xmqqecx9z1n6.fsf@gitster.g>

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

Show 37 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Patrick Steinhardt <ps@pks.im> writes:
> >
> >> 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.
> >
> >> 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).

Show 12 quoted lines

> 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