Re: [PATCH 1/1] vreportf(): avoid buffered write in favor of unbuffered one

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Oct 29, 2019, 20:09 UTC
Message-ID: <nycvar.QRO.7.76.6.1910292101300.46@tvgsbejvaqbjf.bet>
In-Reply-To: <20191029143246.GA3683@sigill.intra.peff.net>

Hi Peff,

On Tue, 29 Oct 2019, Jeff King wrote:

Show 12 quoted lines

> On Tue, Oct 29, 2019 at 03:13:39PM +0100, Johannes Schindelin wrote:
> >
> > > I'd disagree here. Any caller sending an arbitrarily-large prefix
> > > is holding it wrong, and we'd probably want to know as soon as
> > > possible (and a BUG() is our best bet there).
> >
> > How about truncating already inside the prefix, then? It would miss
> > the entire error message... but at least it would print
> > _something_...
>
> Yeah, that might be OK. Hopefully missing the whole rest of the error
> message would cause some tests to fail.

I am not really worried about that. So far, we only have prefixes like `"fatal: "` or `"error: "`. The longest prefix is in `BUG_fl()` itself, when we have access to the file name and the line number of the triggering line, in which case it is `"BUG: <file>:<line>: "`. So as long as nobody builds a custom version of Git that includes a file whose path is insanely long, they should be fine. I am certain that Junio will never accept such a file into git.git.

Oh, and even if anybody would introduce insanely long paths into their fork of git.git, they would _still_ be safe because `BUG_fl()` limits the prefix to 256 bytes, including `NUL`.

> You could also abort() after having written if we want to be more
> BUG()-like.

As I said, this is really a part of the patch I am not at all concerned about. The prefixes are well under control, and I only addressed that potential future breakage (that I believe falls squarely into YAGNI territory) only because two reviewers seemed to be concerned.

I really believe that the care I put into the patch to safeguard against overly long prefixes is seriously overkill.

Ciao, Dscho