On Thu, Feb 17 2022, Edward Thomson wrote: > On Thu, Feb 17, 2022 at 10:29:23AM +0100, Ævar Arnfjörð Bjarmason wrote: >> >> [I'm assuming that dropping the list from CC was a mistake, re-CC-ing] > > It was; many apologies, I don't use mutt very often any more. Thanks! No worries. Also, late reply but I remembered & referenced this thread in https://lore.kernel.org/git/220415.86bkx2bb0p.gmgdl@xxxxxxxxxxxxxxxxxxx/, and saw that I'd left this hanging... >> As for the "new person to our codebase..." I don't think you're wrong >> there, but that's an asthetic preference, not something that's required >> for the stated aims of this series of dropping in compatibility shims. > > Sure, but avoiding a prefix is also not a technical decision but an > aesthetic and ergonomic one. Yes, I see that, but this code is maintained in git.git, not libgit2.git, and having to remember to use custom malloc()/free() per-namespace is very much negative asthetics & ergonomics in that context. So if the linker solution works... > Is using a prefix here great? No, it's not great, it's shit. But it's > shit that's easy to reason about. I really don't see that, as noted in the linked newer reply above we have bugs due to this sort of pattern where someone uses mycustom_malloc(), forgets that, and then calls free() instead of mycustom_free(). Which is a bug and potential segfault that's entirely preventable by not using such wrappers at the per-file level (some one-off "this is where we provide a custom malloc" file might of course have such complexity). > If somebody sees a call to `xdl_free` in some code, they say "wtf is > this `xdl_free` nonsense?" And they grep around and figure it out and > understand the way that this project handles heap allocations. It's > very transparent. > > If somebody sees a call to `free` in their code, they say "great, > `free`". But it merely *appears* very transparent; in fact, there's > some magic behind the scenes that turns a `free` into a `git__free` > without you knowing it. You've not learned the way that this project > handles heap allocations, but you also don't know that there's anything > that you needed to learn. These are the sorts of things that you think > you understand but only discover when you _need_ to discover it because > something's gone very wrong. Because the reader assumed that when they saw malloc/free that it was The Canonical Libc version, as opposed to whatever custom malloc the library linked to? > In my experience, calling a function what it _isn't_ is the sort of thing > that a developer discovers the hard way, and that often leads to them > not trusting the codebase because it doesn't do what it says it does. But they aren't anything until you link to something that provides them. Anyway, I think I see your point, you'd like names to always reflect their different-ness, no linker shenanigans. Anyway, since per [1] it seemed Junio was also more partial to sticking with malloc/free *and* we're talking about a thing that gets one-off-imported into libgit2 (not as a submodule, presumably) I don't think there's any reason to really argue about this. I.e. instead of importing the sources as-is why not just search-replace malloc to mymalloc and free to myfree? Which can be either a dumb "sed" script, or even better the same (and guaranteed to understand C syntax) thing with coccinelle/spatch. Which wouldn't require libgit2 to have a dependency on that, just whatever dev runs that one-off import occasionally. The semantic patch is just: @@ expression E; @@ - free(E); + myfree(E); @@ expression E; @@ - malloc(E); + mymalloc(E); etc. Wouldn't that also give you exactly what you want? Or was the plan to have libgit2 have some option to build this *directly* from git.git sources? 1. https://lore.kernel.org/git/xmqqfsohbdre.fsf@gitster.g/
