Re: [PATCH] mem-pool: use st_add() in mem_pool_strvfmt()

["René Scharfe" <l.s.r@web.de>]

Am 03.04.24 um 22:48 schrieb Jeff King:
> On Wed, Apr 03, 2024 at 12:01:13PM +0200, René Scharfe wrote:
>
>>> diff --git a/usage.c b/usage.c
>>> index 09f0ed509b..5baab98fa3 100644
>>> --- a/usage.c
>>> +++ b/usage.c
>>> @@ -19,8 +19,10 @@ static void vreportf(const char *prefix, const char *err, va_list params)
>>>  	}
>>>  	memcpy(msg, prefix, prefix_len);
>>>  	p = msg + prefix_len;
>>> -	if (vsnprintf(p, pend - p, err, params) < 0)
>>> +	if (vsnprintf(p, pend - p, err, params) < 0) {
>>> +		if (snprintf(p, pend - p, "could not format error: %s", err) < 0)
>>>  		*p = '\0'; /* vsnprintf() failed, clip at prefix */
>>> +	}
>>>
>>>  	for (; p != pend - 1 && *p; p++) {
>>>  		if (iscntrl(*p) && *p != '\t' && *p != '\n')
>>>
>>> Though most of the rest of the function is not that useful (it is mostly
>>> removing unreadable chars, and hopefully we do not have any of those in
>>> our format strings!).

Hmm, this might be worth doing to avoid messing up the terminal if
'err' points into the weeds for some reason.

>> For warnings and usage messages this would keep the prefix and not
>> die.  This would look a bit strange, no?
>>
>> 	usage: could not format error: TERRIBLE MESSAGE!
>
> Sure, but I think any solution here is going to look strange. Keep in
> mind that we're trying to improve the case where we print _nothing_
> useful at all. If you do see this on a non-fatal message, the subsequent
> messages may be informative. E.g.:
>
>   error: could not format error: unable to open loose object %s
>   fatal: bad object HEAD~12
>
> is probably better than just exiting after the first.

OK, right, a format error doesn't have to be fatal and we can keep
running and possibly provide more details.

But mixing the format error with the actual payload message is not ideal.
At least we should give the format error its proper prefix, while still
reporting the prefix of the original message, e.g. like this:

   error: unable to format message: unable to open loose object %s
   fatal:

... or this:

   error: unable to format message: fatal: unable to open loose object %s

I tend to like the first one slightly better, even though the empty
message looks silly.  It doesn't mix the two and answers the questions
that I would have:  Why did the program stop?  Due to a fatal error.
Why is the fatal message silent?  The preceding error explains it.

While the second one only reveals the fatality somewhere in the middle
of the text, weakening the meaning of prefixes.

>> Yes, showing errno doesn't add that much value.  Except in this case it
>> shows that there's something going on that I don't understand.  Dare I
>> dig deeper?  Probably not..
>
> Well, let us know if you do. ;)

I still don't know why the error code varies between runs, but it
clearly does not come from vsnprintf(3) -- if I set errno to some
arbitrary value before the call, it is kept.  Which is enough to
convince me to ignore errno here.

René