Re: [RFC PATCH] vreportf: ensure sensible ordering of normal and error output

From: Jeff King <peff@peff.net>
Date: Nov 30, 2021, 07:21 UTC
Message-ID: <YaXQ/HinYZH1wL7E@coredump.intra.peff.net>
In-Reply-To: <20211130043946.19987-1-sunshine@sunshineco.com>

On Mon, Nov 29, 2021 at 11:39:46PM -0500, Eric Sunshine wrote:

Show 24 quoted lines

> The order in which the stdout and stderr streams are flushed is not
> guaranteed to be the same across platforms or `libc` implementations.
> This lack of determinism can lead to anomalous and potentially confusing
> output if normal (stdout) output is flushed after error (stderr) output.
> For instance, the following output which clearly indicates a failure due
> to a fatal error:
> 
>     % git worktree add ../foo bar
>     Preparing worktree (checking out 'bar')
>     fatal: 'bar' is already checked out at '.../wherever'
> 
> has been reported[1] on Microsoft Windows to appear as:
> 
>     % git worktree add ../foo bar
>     fatal: 'bar' is already checked out at '.../wherever'
>     Preparing worktree (checking out 'bar')
> 
> which may confuse the reader into thinking that the command somehow
> recovered and ran to completion despite the error.
> 
> Rather than attempting to address this issue on a case by case basis,
> address it by making vreportf() -- which is the heart of error-reporting
> functions die(), error(), warn(), etc. -- flush stdout before emitting
> the error message to stderr.

I left some thoughts on whether this flush is safe elsewhere in the thread. But for this particular case, two things occur to me:

  - shouldn't status messages like this go to stderr anyway? I know some
    people follow the "unless it is an error, it should not to go
    stderr" philosophy. But I think in general our approach in Git is
    more "if it is the main output of the program, it goes to stdout; if
    it is chatter or progress for the user, it goes to stderr".

  - the reason it works consistently on glibc is that stdout to a
    terminal is line buffered by default, so the "preparing" line is
    flushed immediately. If that isn't the case on Windows, should we
    consider calling setlinebuf() preemptively when isatty(1)?

    That only covers most cases, of course (both to a terminal, or
    separated stdout/stderr). Even on Linux, if you do:

      git worktree add ../foo bar >out 2>&1

    you'll get the stderr result before stdout. That implies again to me
    that this kind of message really ought to be part of the stderr
    stream.

-Peff