Re: [PATCH] diff.c: flush stdout before printing rename warnings

From: Duy Nguyen <pclouds@gmail.com>
Date: Jan 17, 2018, 01:04 UTC
Message-ID: <CACsJy8AzU_WUFvhBKZ76korGJoMy9-6XXx7kZUKyz2GoHgxXGg@mail.gmail.com>
In-Reply-To: <xmqqd129vd17.fsf@gitster.mtv.corp.google.com>

On Wed, Jan 17, 2018 at 2:06 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 27 quoted lines

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> The diff output is buffered in a FILE object and could still be
>> partially buffered when we print these warnings (directly to fd 2).
>> The output is messed up like this
>>
>>  worktree.c                                   |   138 +-
>>  worktree.h        warning: inexact rename detection was skipped due to too many files.
>>                            |    12 +-
>>  wrapper.c                                    |    83 +-
>>
>> It gets worse if the warning is printed after color codes for the graph
>> part are already printed. You'll get a warning in green or red.
>>
>> Flush stdout first, so we can get something like this instead:
>>
>>  xdiff/xutils.c                               |    42 +-
>>  xdiff/xutils.h                               |     4 +-
>>  1033 files changed, 150824 insertions(+), 69395 deletions(-)
>> warning: inexact rename detection was skipped due to too many files.
>
> The patch sort-of makes sense, and I am not sure if any of the
> issues that show rooms for potential improvements I'll mention are
> worth doing.
>
>  - This matters only when the standard output and the starndard error
>    are going to the same place.

Which is usually true when pager is involved because we redirect stderr to the pager too 61b80509e3 (sending errors to stdout under $PAGER - 2008-02-16)

>    It also would be conceptually nicer to
>    flush stderr as well even though it is by default not fully
>    buffered.

There's more than that. I briefly considered if the same thing could happen elsewhere, this is not the only place we use buffered stdio. But since it's just a minor annoyance, I stuck to minimal fixes and the "fix as we see" approach instead of scanning the whole code base and preventing similar cases.

-- 
Duy