Re: [PATCH 1/2] Added use of xmalloc() on diff-delta.c

From: Junio C Hamano <junkio@cox.net>
Date: Apr 5, 2007, 05:14 UTC
Message-ID: <7vy7l7wggr.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <Pine.LNX.4.64.0704041525220.6730@woody.linux-foundation.org>

Linus Torvalds <torvalds@linux-foundation.org> writes:

Show 14 quoted lines

> On Wed, 4 Apr 2007, Junio C Hamano wrote:
>> 
>> These patches take that nice property away, making libification
>> more difficult, which is the downside.  Is there an upside?
>
> Well, we could just make the libification rule very simple:
>
>  - the library does *not* include "xmalloc()", and you have to handle 
>    out-of-memory situations yourself inside the xmalloc() that *you* as a 
>    libification user provide!.
>
> Then, we just make our xmalloc() be non-inlined (which we should do 
> *anyway* - it's long since grown so big that it shouldn't be inlined in 
> the first place), and we make it part of a non-library git object file.

I agree that is probably a sane thing to do for existing callers of xmalloc() -- the callers are not prepared to handle (near-)oom case gracefully to begin with.

Your "libified git decides how xmalloc() copes with (near-)oom condition gracefully" is much better than "memory-allocating function in git declares that oom in xmalloc() is fatal", which is what we currently have.

I however think it is an independent issue, wrt the part Bruno's patch touches. In the case of this particular call chain between create_delta()/create_delta_index() and its caller, I think the code that is there allows nicer arrangement. The caller could instead easily attempt to cope with (near-)oom condition more gracefully. And that was my suggestion about returning 0 when delta_index cannot be built instead of dying.

Of course, this caller is in memory-hungry "pack-objects", and all of the above is mostly academic, as failure to allocate memory in create_delta_index() would most likely mean you would have trouble allocating memory for other more important data.