Re: [PATCH 1/1] xdiff: provide indirection to git functions
On Thu, Feb 17 2022, Edward Thomson wrote:
Show 5 quoted lines
> 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@evledraar.gmail.com/, and saw that I'd left this hanging...
Show 6 quoted lines
>> 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).
Show 13 quoted lines
> 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/
Edward Thomson
