Hi Junio,

On Tue, 29 Oct 2019, Junio C Hamano wrote:

"Johannes Schindelin via GitGitGadget" [off-list ref]
writes:

quoted

Also please note that we `fflush(stderr)` here to help when running in a
Git Bash on Windows: in this case, `stderr` is not actually truly
unbuffered, and needs the extra help.

Yuck.  So on all systems, vreportf() now totally bypasses stdio?

Yep ;-)

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

By the way, I'd retitle the patch to highlight that we are still doing
buffered write, if I were doing this topic.  We are just avoiding some
implementations of stdio that do not give us buffering and doing the
buffering ourselves.

    Subject: vreportf(): don't rely on stdio buffering

or something like that.

Good idea.

quoted

Co-authored-by: Alexandr Miloslavskiy [off-list ref]
Signed-off-by: Johannes Schindelin <redacted>
---
 usage.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/usage.c b/usage.c
index 2fdb20086b..4328894dce 100644
--- a/usage.c
+++ b/usage.c

@@ -10,13 +10,19 @@ void vreportf(const char *prefix, const char *err, va_list params)
 {
 	char msg[4096];
 	char *p;
-
-	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.

So "off" would be strlen(prefix), which could be longer than
sizeof(msg)?  Then what happens to this vsnprintf() we see below?

A problem.

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

quoted

+	int ret = vsnprintf(msg + off, sizeof(msg) - off, err, params);
 	for (p = msg; *p; p++) {
 		if (iscntrl(*p) && *p != '\t' && *p != '\n')
 			*p = '?';
 	}
-	fprintf(stderr, "%s%s\n", prefix, msg);

Strictly speaking this is a breaking change in that control
sequences in prefix are now clobbered.  Does any caller call this
function with prefix like "^M\033[K<some string>" to overwrite the
last output line with the new message?  If not, then probably we do
not have to worry about it (and reusing msg[] does feel attractive).

Such a sequence would not exactly be a prefix, but okay, I changed the
code to replace only characters in the non-prefix part. For good
measure, I also detect `NUL`s in that part and shorten `ret` in that
case (think `die("This was an\0unintentional NUL")`).

Thanks for the review!
Dscho

quoted

+	if (ret > 0) {
+		if (off + ret > sizeof(msg) - 1)
+			ret = sizeof(msg) - 1 - off;
+		msg[off + ret] = '\n'; /* we no longer need a NUL */
+		fflush(stderr);
+		xwrite(2, msg, off + ret + 1);
+	}
 }

 static NORETURN void usage_builtin(const char *err, va_list params)