Re: [PATCH] use xstrdup, not strdup in ll-merge.c

[Alex Riesen <raa.lkml@gmail.com>]

2009/6/15 Jim Meyering <jim@meyering.net>:
> Alex Riesen wrote:
>> 2009/6/14 Jim Meyering <jim@meyering.net>:
>>> @@ -231,7 +231,7 @@ static int read_merge_config(const char *var, const char *value, void *cb)
>>>
>>>        if (!strcmp(var, "merge.default")) {
>>>                if (value)
>>> -                       default_ll_merge = strdup(value);
>>> +                       default_ll_merge = xstrdup(value);
>>
>> read_merge_config has a failure mode (where it returns -1), why not use it?
>
> I didn't even consider it, because it would be inconsistent with
> the other heap-allocation functions used there (xcalloc, xmemdupz).
>
> However, now that I do, it looks like that would mean adding four times
> the same code (including conditionals and code to generate a diagnostic via
> a call to error -- or a goto). Why bother, when all of that is already
> encapsulated in xmalloc?

So that a useful error message can be given in the _caller_ (it knows
more about context)?

Otherwise the error message ("Out of memory, strdup failed") does not
have anything about the place nor situation in it. As the situations
when a modern system really runs out of memory are very rare,
mostly such reports just point at some inconsistency elsewhere
(like bloody stupid memory management in system support libraries
on an OS-not-to-be-named-again or the usual corruption of heap
control structures).

Besides, xstrdup does more then just allocation: it tries to free global
list of cached pack chunks. This does not play very well with the efforts
to make a library out of the modern Git code.

> Maybe because you want to be able to continue after an allocation failure?

No.

> If a small strdup allocation fails, odds are good that the code won't
> be able to do anything useful, so when not in library code, cleanest is
> simply to exit.

Doubt it (because you better describe _why_ you "simply" interrupted
users workflow, so the said user can do something about it).

> In addition, if you insist on using strdup, you'll probably want to
> be consistent and use calloc and memdupz, too.  Adding all of the code
> required to recover from those failures and to avoid leaks would be messy.

I don't insist on that. I should have said in the first message about
more elaborate explanation of the error to user. Sorry.