On Wed, Apr 30, 2025 at 04:10:37PM -0700, Junio C Hamano wrote: > Junio C Hamano <gitster@xxxxxxxxx> writes: > > > Patrick Steinhardt <ps@xxxxxx> 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). > 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
