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

> 
> 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

>> 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.