On Fri, Jun 03 2022, Junio C Hamano wrote: > Junio C Hamano <gitster@xxxxxxxxx> writes: > >>> However, in doing so we've been fooling ourselves when it comes to >>> what trace2 events we log. The events tested for in >>> 0a9dde4a04c (usage: trace2 BUG() invocations, 2021-02-05) are not the >>> real ones, but those that we emit only from the "test-tool". >> >> I can fully agree with the above reasoning, i.e. let's test what we >> do use in production, instead of something nobody uses for real, if >> we were adding a test for BUG() in vacuum, but why did we have to >> "fake" it in the first place? >> ... >> Are we sure that the reason no longer applies? How do we know? We >> would want to explain that to future developers in the proposed log >> message, I would think. > > We can flip it the other way around. > > I do not think I ever saw anybody asked anybody on this list who got > a BUG() message to use the coredump to do something useful. Don't > modern distros ship with "ulimit -c 0" these days? I think that part of it was just a side-effect of SIGABRT. > It might be possible that a better direction is to introduce > GIT_ABORT_ON_BUG environment or core.abortOnBUG configuration that > chooses between abort() and exit(99), or something like that, and > then we switch to use the latter by default over time? I think the reason for abort() was what's covered in raise(3), i.e. it's a one-stop-shop to getting "stop it" behavior both under threading and non-threading, which as e.g. exit(3) discusses wouldn't be thread-safe with it. But perhaps that was all premature worrying, we're mostly running non-threaded, and to the extent that we ever BUG() running into exit(3) threading issues is probably the least of our worries. So perhaps we should drop abort() entirely, I don't know. These proposed patches tried not to do that, but just to log things when we did so.
