Re: [PATCH 3/3] xdiff: introduce XDL_ALLOC_GROW()

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Jul 06 2022, Phillip Wood wrote:

> Hi Ævar
>
> On 30/06/2022 14:25, Ævar Arnfjörð Bjarmason wrote:
>> On Thu, Jun 30 2022, Phillip Wood wrote:
>> 
>>> On 30/06/2022 13:03, Phillip Wood wrote:
>>>> On 30/06/2022 11:54, Ævar Arnfjörð Bjarmason wrote:
>>>>>
>>>>> On Wed, Jun 29 2022, Phillip Wood via GitGitGadget wrote:
>>>>>
>>>>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>>>>
>>>>>> Add a helper to grow an array. This is analogous to ALLOC_GROW() in
>>>>>> the rest of the codebase but returns −1 on allocation failure to
>>>>>> accommodate other users of libxdiff such as libgit2.
>>>>>
>>>>> Urm, does it? I just skimmed this, so maybe I missed something, but I
>>>>> don't see where you changed the definition of xdl_malloc(),
>>>>> xdl_realloc() etc.
>>>
>>> Oh I think I might have misunderstood your question. For git.git it
>>> will still die() but for other users that arrange for xdl_realloc() to
>>> return NULL on failure it will return -1. The same applies to the
>>> comments in the previous two patches about XDL_[CM]ALLOC_ARRAY()
>>> returning NULL on allocation failure.
>> Yes, I meant that the "but returns −1 on allocation failure to
>> accommodate other users of libxdiff such as libgit2" is really more of
>> a:
>> 	...but *allows for* dropping in another xmalloc(), xrealloc()
>> 	etc. implementation that doesn't die on failure.
>> So I think the rest of my upthread question still stands, i.e.:
>> 	"So if that's the plan why would we need an XDL_ALLOC_ARRAY(),
>> 	can't you just check that it [I mean ALLOC_ARRAY()] returns
>> 	non-NULL?"
>> I.e. if the plan is to replace the underlying x*() functions with
>> non-fatal variants can't you use ALLOC_ARRAY() instead? I haven't tried
>> that, but I don't see a reason we couldn't do that in principle...
>
> As the cover letter says, the aim of this series is not to replace
> xmalloc() etc with non-fatal variants, it is just to make the xdiff
> code more readable.

I don't think it's more readable to carry code in-tree that's
unreachable except when combined with code out-of-tree. I.e. this series
leaves us with the equivalent of:

	ptr = xmalloc(...);
        if (!ptr)
		/* unreachable in git.git ... */

I don't think it's more readable to have code that rather trivial
analysis will show goes against the "__attribute__((noreturn))" we're
placing on our die() function.

Which is what I'm pointing out with "running with scissors". I.e. I'm
fully on-board with the end goal, but that can be accomplished in a way
that doesn't confuse humans & analyzers alike.

> (One can already use a non-fatal allocation
> function for xdl_malloc()) [...]

That just seems inviting a segfault or undefined/untested behavior
(whether in the sense of "undefined by C" or "untested by git.git's
codebase logic"). Everything around xmalloc() now assumes "never returns
NULL", and you want to:

 * Make it return NULL when combined with out-of-tree-code
 * Maintain the code in git.git, where it never returns NULL, but in a
   way where we won't have bugs when combined with a new macro that
   behaves differently, in a way we never even test ourselves.

Isn't that correct, or am I missing something?

> I don't think that using ALLOC_ARRAY() in
> xdiff is helpful for other users as they would have to define their
> own array allocation macros, rather than just providing their own
> allocation functions. I would like to reduce the friction others have
> upstreaming xdiff patches to us, not increase it.

Yes, I'm totally on-board with reducing the friction in others using
xdiff, and would like to see more of that sort of out-of-tree use in
general (although for things outside of xdiff GPL v.s. LGPL concerns
come into play).

I'd even like for us to explicitly make that much easier. I.e. if you
want to use xdiff now you search for it, and find the at this point
unmaintained upstream, and if you find that git has a "newer" version
you'll have some trouble extracting it already.

After this series you'll need to start writing & maintaining your own
non-trivial alloc wrapper logic if you're doing that. If you get it
subtly wrong you'll have a buggy xdiff, and most likely users will just
copy/paste the git.git version from our git-compat-util.h & cache.h,
which is rather silly.


Which is why I'm saying we could/should do this in a much easier way,
i.e.:

 * Factor out the now only-fatal-on-NULL ALLOC_GROW() such that we can
   have a non-fatal version (ALLOC_GROW) and a non-fatal one.

   I don't know what we'd call it. we usually have X*() meaning "fatal",
   but these are fatal by default, maybe G* for gently? So
   GALLOC_GROW().  Urgh, anyway, continuing with that ugly name...

 * Have xdiff/ use that GALLOC_GROW() & malloc(), not ALLOC_GROW() &
   xmalloc(), as we really want to have the appropriate code flow
   analysis etc. spot for us that we should handle NULL returns,
   otherwise combining this code with libgit2 will be buggy/broken.

This makes it much easier for libgit2 to use this, as it won't need to
do anything special at all. Since our GALLOC_GROW() will eventualy use
malloc() instead of xmalloc() you don't need to define anything that
re-implements the GALLOC_GROW() or whatever other non-fatal versions of
our only-fatal helpers we have.

This assumes that we'd move these macros out of git-compat-util.h and a
new git-exernal-compat.h, or that instead of *just* copying the xdiff/
directory your import script would need to run some small bit of cc -E
and or perl/sed to one-off extract the smell bits of
git-exernal-compat.h or cache.h that we need.