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

[Jim Meyering <jim@meyering.net>]

Alex Riesen wrote:

> 2009/6/15 Jim Meyering <jim@meyering.net>:
>> Alex Riesen wrote:
>>> 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)?
>>
>> So you want to tell the user that we failed
>> to strdup the "merge.default" value?
>> Or the "driver" value?
>
> "merge: recursive: error loading configuration (last seen:
> merge.default): Out of memory\n"
>
>> Of more general interest, when xstrdup fails, it might be useful to
>> include in the diagnostic how long the would-be-dup'd string was.  I.e.,
>> rather than saying
>>
>>    die("Out of memory, strdup failed");
>> say
>>    die("Out of memory, failed to strdup a %lu-byte string",
>>        (unsigned long int) strlen(str));
>
> Yes. Still lacks higher level information, though.
>
>>> 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
>>
>> Exactly.  This is why I think it's not worthwhile to invest in
>> a more precise diagnostic, here.
>
> I disagree. It is already hard to find starting point for debugging if
> the failed code is just a layer: the config of ll-merge is called not only
> from the merge drivers, but also indirectly from the programs which
> call the merge itself. Now, go figure where has it failed...

If you're convinced of the value of such a change, go for it.
Though it sounds like you're saying you'd prefer a stack trace.