patch, 2 partsAdded use of xmalloc() on diff-delta.c

Signed-off-by: Bruno Ribas <ribas@c3sl.ufpr.br>
---
 diff-delta.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

Signed-off-by: Bruno Ribas <ribas@c3sl.ufpr.br>
---
 builtin-pack-objects.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

These two functions, create_delta_index() and create_delta(), are already nicely libified. They allow the caller to deal with oom condition. The caller may die(), or it may decide to continue its operation with reduced functionality without using delta data. A good example of this is found a few lines after the lines the second patch touches. When create_delta() cannot find memory to work with, the entire function returns 0, saying "sorry, cannot deltify these two", which would cause the object stored without deltification.

These patches take that nice property away, making libification more difficult, which is the downside. Is there an upside?

If anything, I suspect that the part that calls die() you touched in the second patch could return NULL.

On Wed, 4 Apr 2007, Junio C Hamano wrote:

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.

Other libgit uses might end up doing something like

		..
		if (sigsetjump(buffer, 1)) {
			show_oom_message();
		..

	void *xmalloc(size_t size)
	{
		void *ret = malloc(size ? size : 1);
		if (!ret)
			siglongjmp(buffer);
		return ret;
	}

or, if they use C++ exception handling, they'd just make their own xmalloc() raise an exception, and have the callers catch it.

The point being that this is what you'd need to do *anyway*, and trying to make all the library routines return NULL or some other error case is just worse programming practice than just having a xmalloc() that dies by default but that can be overridden.

		Linus

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

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.