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

From: René Scharfe <l.s.r@web.de>
Date: Apr 5, 2024, 13:10 UTC
Message-ID: <6f727926-a901-4d9f-8a7f-d966f222f15d@web.de>
In-Reply-To: <20240403204836.GC1949464@coredump.intra.peff.net>

Am 03.04.24 um 22:48 schrieb Jeff King:

Show 22 quoted lines

> 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.

Show 14 quoted lines

>> 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.

Show 5 quoted lines

>> 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é