Hi Patrick & Junio, On Fri, 2 May 2025, Patrick Steinhardt wrote: > 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. I see a need for this patch, still, and it is not a mere "opinion". The fact of the matter is that the Visual C-built CI (first the Azure Pipeline, then GitHub Actions) have built the artifacts in release mode since forever. And the Meson addition simply made a mistake by _not specifying_ release mode (and hence defaulting to debug mode). This makes a difference because in `compat/mingw.c`, there is this (https://github.com/git/git/blob/v2.49.0/compat/mingw.c#L3247-L3255): #ifdef _MSC_VER #ifdef _DEBUG _CrtSetReportMode(_CRT_ASSERT, _CRTDBG_MODE_DEBUG); #endif [...] #endif This means that each and every `assert()`, whether that be in Git's source code, or in any dependency of Git (which the Git project does not control!), is handled with this modal dialog. It was an oversight in the win+Meson patches not to use release mode, and the patch I proposed fixes this bug. In the alternative, you could also just drop the entire win+Meson stuff, of which I would be actually quite in favor: No Visual Studio user will be happy with Meson, therefore it would be the kind thing to drop all pretense, officially, that Git cares about Visual Studio users. The easier and quicker solution, though, (which would maintain a modicum of Visual Studio support) would be to un-drop the fix proposed in https://lore.kernel.org/git/pull.1908.git.1745593515875.gitgitgadget@xxxxxxxxx/ Ciao, Johannes
