Re: [PATCH v4 02/16] Report bugs consistently

Hi Junio & Peff,

On Mon, 25 Jul 2016, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

On Mon, Jul 25, 2016 at 02:44:25PM -0700, Junio C Hamano wrote:

quoted

quoted

diff --git a/imap-send.c b/imap-send.c
index db0fafe..67d67f8 100644
--- a/imap-send.c
+++ b/imap-send.c

@@ -506,12 +506,12 @@ static char *next_arg(char **s)
 
 static int nfsnprintf(char *buf, int blen, const char *fmt, ...)
 {
-	int ret;
+	int ret = -1;
 	va_list va;
 
 	va_start(va, fmt);
 	if (blen <= 0 || (unsigned)(ret = vsnprintf(buf, blen, fmt, va)) >= (unsigned)blen)
-		die("Fatal: buffer too small. Please report a bug.");
+		die("BUG: buffer too small (%d < %d)", ret, blen);
 	va_end(va);
 	return ret;
 }

If "you gave me this size but you need at least this much" is truly
worth reporting, then this is misleading (ret is shown as -1 but you
do not even know how much is necessary).  In any case, this should
be done as a separate step anyway.

Hrm, isn't "ret" going to be the necessary size? According to the
standard, it should tell us how many bytes were needed, not "-1" (this
is the "your vsnprintf is broken" case handled by the strbuf code).

Yes.  If blen <= 0, we do not even do vsnprintf() and that is why
Dscho added "int ret = -1" initialization; otherwise his new die()
would end up referencing uninitialized ret.

Exactly. While I was fixing this bug message, it occurred to me that it
makes little sense to ask a user to report a bug when it is unknown how
small the buffer was and what would have been the desired buffer size. So
I did a fly-by fix.

However, it is true that this is completely outside the purpose of this
patch series (in fact, most of this patch is completely outside the
purpose, and I am regretting that dearly, as the patch series' submission
already takes almost a month, and I only now get people to review the
critical parts of the changes).

So I simply backed out the more verbose message and *only* do the obvious
thing: replace the "Fatal:" prefix by a "BUG:" one.

quoted

I do think the numbers are reversed, though. It should be "blen < ret".

That, too ;-)

True. All the better that I reverted that part of the patch ;-)

Ciao,
Dscho