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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Nov 30, 2021, 14:05 UTC
Message-ID: <CAPig+cRQqm8Ce29PnkndT47NNxM3UhJv12RZGZZJD-AyGVC7Zw@mail.gmail.com>
In-Reply-To: <YaXQ/HinYZH1wL7E@coredump.intra.peff.net>

On Tue, Nov 30, 2021 at 2:21 AM Jeff King <peff@peff.net> wrote:

Show 14 quoted lines

> On Mon, Nov 29, 2021 at 11:39:46PM -0500, Eric Sunshine wrote:
> > 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".

I considered this as well and agree that it would be a nicer localized fix, but...

(1) I don't think the practice is documented anywhere, so people -- including me when I wrote builtin/worktree.c -- might not know about it. Indeed, we don't seem to be entirely consistent about doing it this way. Randomly picking submodule-helper.c, for instance, I see status-like messages going to stdout:

    printf(_("Entering '%s'\n"), displaypath);
    printf(_("Synchronizing submodule url for '%s'\n"), ...);

    if (...)
        format = _("Cleared directory '%s'\n");
    else
        format = _("Could not remove submodule work tree '%s'\n");
    printf(format, displaypath);

(2) With git-worktree being four or five years old, for backward-compatibility concerns, I worry that "that ship has sailed", where 'that' is the freedom to relocate those status-like messages from stdout to stderr. I don't want to break tooling which exists around git-worktree.

I'd be happy to be wrong on the second point -- indeed, git-worktree is still marked "experimental" in the man-page, but that may not mean anything this late in the game -- and submit a patch which places git-worktree's status-like messages on stderr instead of stdout. Thoughts?

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

I'll let the Windows experts chime in on this (Dscho?). For all I know, that might introduce a bad performance regression on that platform under whatever terminal-like or pseudo-tty-like emulation they are using.