Re: [PATCH v4 02/16] Report bugs consistently
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Jul 26, 2016, 12:24 UTC
- Message-ID
- <alpine.DEB.2.20.1607261421310.14111@virtualbox>
- In-Reply-To
- <xmqqlh0p2vmn.fsf@gitster.mtv.corp.google.com>
Hi Junio & Peff,
On Mon, 25 Jul 2016, Junio C Hamano wrote:
Show 36 quoted lines
> Jeff King <peff@peff.net> writes:
>
> > On Mon, Jul 25, 2016 at 02:44:25PM -0700, Junio C Hamano wrote:
> >
> >> > 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.
> > 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