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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 16, 2018, 19:06 UTC
Message-ID: <xmqqd129vd17.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20180116092349.11330-1-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 18 quoted lines

> 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.  It also would be conceptually nicer to
   flush stderr as well even though it is by default not fully
   buffered.

 - Also this function can take two bools and gives a warning that
   potentially cause the issue three out of four combinations, so one
   out of four cases we would be unnecessarily flushing.

Show 17 quoted lines

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  diff.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/diff.c b/diff.c
> index fb22b19f09..5545c25640 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -5454,6 +5454,7 @@ N_("you may want to set your %s variable to at least "
>  
>  void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc)
>  {
> +	fflush(stdout);
>  	if (degraded_cc)
>  		warning(_(degrade_cc_to_c_warning));
>  	else if (needed)