On Tue, Aug 27, 2019 at 2:07 AM Johannes Schindelin <Johannes.Schindelin@xxxxxx> wrote: > > Unfortunately, this is _still_ incorrect. I know, and that is why we worked out a v6 RC with Rene than then was pushed to github[0] and validated to work thanks to your integration as detailed in [1], I never got to send an update to the list though because Rene wanted my squashing into his patch to be independent as detailed there and because I assumed that when Junio didn't pull my reroll, it was probably because your tree had a fix already anyway. the recent discovery that xmalloc wasn't thread safe though, complicates things further and that is why I was expecting to reroll this on top of both ab/pcre-jit-fixes and jk/drop-release-pack-memory (later one already in next) as detailed in [2] > I pointed out multiple times that custom allocators can be activated at > run-time rather than compile-time, therefore making the choice at > compile-time is wrong. Besides, there is nothing specific to nedmalloc > about this. So the patch is double-wrong on that account. Just to clarify, I think my patch accounts for that (haven't tested that assumption, but will do now that I have a windows box, probably even with mi-alloc) but yes, the only reason why there were references to NEDMALLOC was to isolate the code and make sure the fix was tackling the problem, it was not my intention to do so at the end, specially once we agreed that xmalloc should be used anyway. > The patch has a yet even more immediate problem: t7816.48 is failing in > the CI build for _weeks_ now: it requires that global context to be > initialized, but no code path hits the initialization, resulting in a > very, very ugly: > > BUG: grep.c:516: pcre2_global_context uninitialized > > See > https://dev.azure.com/gitgitgadget/git/_build/results?buildId=15151&view=ms.vss-test-web.build-test-results-tab&runId=41282&resultId=101710&paneView=debug > for details. IMHO this is probably testing V3 (from pu) and which was hopefully fixed in the last github force push for my branch > All of this could be easily avoided. As I had pointed out already, the > obvious fragility is not worth the optimization, and we should just > initialize the global context always, as does this patch > (https://github.com/git-for-windows/git/commit/5e5b959169e6efee73e0b50e464166822b7d2d07). ironically only found out about that patch after I got a windows box (running Windows Home though) and had finished testing my own squashed fix[3] that has succeeded being validated in GitHub apologize for the delays, and will be fine using your squash, mine, the V6 RC (my preference) or dropping this series from pu if that would help clear the ugliness of pu for windows hopefully this won't be repeated now that I am aware of github's integration and have my own (albeit very slow) windows environment as well. Carlo [0] https://github.com/git/git/commit/0ca5d0550c17a68d83b8922b71aeff891958ed0e [1] https://public-inbox.org/git/CAPUEspiFuvgMQ3W1se1B=8aTTrQsJSZTyQTzG1TpiyN8HTOpkA@xxxxxxxxxxxxxx/ [2] https://public-inbox.org/git/CAPUEspg9F7RutCUCoRAAXmRePjiunq3-zG7cN3uz_t5DVMxP=g@xxxxxxxxxxxxxx/ [3] https://github.com/git/git/pull/627
