git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Jeff King <peff@peff.net>,
	Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>,
	Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: Re: [PATCH 1/1] vreportf(): avoid buffered write in favor of unbuffered one
Date: Tue, 29 Oct 2019 12:18:36 +0900	[thread overview]
Message-ID: <xmqqeeyw6xyr.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <455026ce3ef2b2d7cfecfc4b4bf5b588eebddcfe.1572274859.git.gitgitgadget@gmail.com> (Johannes Schindelin via GitGitGadget's message of "Mon, 28 Oct 2019 15:00:59 +0000")

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

  reply	other threads:[~2019-10-29  3:18 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-28 15:00 [PATCH 0/1] Fix t5516 flakiness in Visual Studio builds Johannes Schindelin via GitGitGadget
2019-10-28 15:00 ` [PATCH 1/1] vreportf(): avoid buffered write in favor of unbuffered one Johannes Schindelin via GitGitGadget
2019-10-29  3:18   ` Junio C Hamano [this message]
2019-10-29 12:30     ` Johannes Schindelin
2019-10-29 13:49       ` Jeff King
2019-10-29 14:13         ` Johannes Schindelin
2019-10-29 14:32           ` Jeff King
2019-10-29 20:09             ` Johannes Schindelin
2019-10-30  1:43               ` Junio C Hamano
2019-10-29 16:44         ` Junio C Hamano
2019-10-29 10:38   ` SZEDER Gábor
2019-10-29 12:38     ` Johannes Schindelin
2019-10-29 13:52       ` Jeff King
2019-10-29 14:18         ` Johannes Schindelin
2019-10-29 13:37 ` [PATCH v2 0/1] Fix t5516 flakiness in Visual Studio builds Johannes Schindelin via GitGitGadget
2019-10-29 13:37   ` [PATCH v2 1/1] vreportf(): avoid relying on stdio buffering Johannes Schindelin via GitGitGadget
2019-10-29 14:21     ` Alexandr Miloslavskiy
2019-10-29 19:57       ` Johannes Schindelin
2019-10-29 20:09         ` Jeff King
2019-10-29 20:24           ` Alexandr Miloslavskiy
2019-10-29 20:11         ` Alexandr Miloslavskiy
2019-10-29 20:01   ` [PATCH v3 0/1] Fix t5516 flakiness in Visual Studio builds Johannes Schindelin via GitGitGadget
2019-10-29 20:01     ` [PATCH v3 1/1] vreportf(): avoid relying on stdio buffering Johannes Schindelin via GitGitGadget
2019-10-29 20:32       ` Jeff King
2019-10-30  8:54         ` Johannes Schindelin
2019-10-31  6:24           ` Jeff King
2019-10-31 10:26             ` Johannes Schindelin
2019-10-31 15:48               ` Jeff King
2019-11-01 18:41                 ` Johannes Schindelin
2019-10-30  2:01       ` Junio C Hamano
2019-10-30  9:13         ` Johannes Schindelin
2019-10-30 10:44     ` [PATCH v4 0/1] Fix t5516 flakiness in Visual Studio builds Johannes Schindelin via GitGitGadget
2019-10-30 10:44       ` [PATCH v4 1/1] vreportf(): avoid relying on stdio buffering Johannes Schindelin via GitGitGadget
2019-11-02  4:05         ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqeeyw6xyr.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=alexandr.miloslavskiy@syntevo.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=johannes.schindelin@gmx.de \
    --cc=peff@peff.net \
    --subject='Re: [PATCH 1/1] vreportf(): avoid buffered write in favor of unbuffered one' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).