Hi Peff,

On Tue, 29 Oct 2019, Jeff King wrote:

On Tue, Oct 29, 2019 at 01:30:51PM +0100, Johannes Schindelin wrote:

quoted

quoted

Also, this is only to help output from us that goes via vreportf() and
other codepaths in us that use stdio to write to the standard error
stream can still get mixed on Windows (I think the answer is yes,
because we wouldn't need fflush() in this patch if we are covering all
writes to the standard error stream)?

Yes, `write()` can get interrupted, so there is still a slight chance of
interleaving.

However, with `fprintf()`, apparently the MSVC runtime essentially
writes and flushes one character at a time, which will make it _much_
more likely that two competing processes write interleaved messages to
`stderr`.

Wow, they have truly taken "unbuffered" to a whole new level.

I don't mind seeing this for all platforms, though. I can't think of any
downside, and having one less moving part to contend with in our
error-reporting code seems like a good thing.

quoted

quoted

quoted

-	vsnprintf(msg, sizeof(msg), err, params);
+	size_t off = strlcpy(msg, prefix, sizeof(msg));

Like snprintf(3) the strlcpy() and strlcat() functions return the
total length of the string they tried to create.  For strlcpy() that
means the length of src.

True (I misread `compat/strlcpy.c` and forgot to consult the
documentation). This length can be longer than `msg`, of course.

I'd recommend xsnprintf() here. If we have a prefix longer than our
vreportf() buffer, I think a BUG() is the right outcome.

But BUG_fl() calls vreportf(). I am worried about an infinite
recursion...

quoted

I `git grep`ed and saw that only very short `prefix`es are hard-coded.
So that is a hypothetical concern.

However, Alex also indicated his discomfort with this, so I will change
the code to account for a `prefix` that is too long (the entire error
message will be clipped away in that case, which is unfortunate, but to
be expected).

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

Ciao,
Dscho