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

[Junio C Hamano <gitster@pobox.com>]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> 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?

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

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.

> Co-authored-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  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.

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

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

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