Re: [PATCH 1/1] xdiff: provide indirection to git functions

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 15, 2022, 15:55 UTC
Message-ID: <220415.867d7qbaad.gmgdl@evledraar.gmail.com>
In-Reply-To: <20220217225804.GC7@edef91d97c94>

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/