git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] diff.c: flush stdout before printing rename warnings
@ 2018-01-16  9:23 Nguyễn Thái Ngọc Duy
  2018-01-16 19:06 ` Junio C Hamano
  0 siblings, 1 reply; 3+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2018-01-16  9:23 UTC (permalink / raw)
  To: git; +Cc: Nguyễn Thái Ngọc Duy

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.

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)
-- 
2.15.1.600.g899a5f85c6


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] diff.c: flush stdout before printing rename warnings
  2018-01-16  9:23 [PATCH] diff.c: flush stdout before printing rename warnings Nguyễn Thái Ngọc Duy
@ 2018-01-16 19:06 ` Junio C Hamano
  2018-01-17  1:04   ` Duy Nguyen
  0 siblings, 1 reply; 3+ messages in thread
From: Junio C Hamano @ 2018-01-16 19:06 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: git

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.  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.



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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] diff.c: flush stdout before printing rename warnings
  2018-01-16 19:06 ` Junio C Hamano
@ 2018-01-17  1:04   ` Duy Nguyen
  0 siblings, 0 replies; 3+ messages in thread
From: Duy Nguyen @ 2018-01-17  1:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List

On Wed, Jan 17, 2018 at 2:06 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2018-01-17  1:04 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-16  9:23 [PATCH] diff.c: flush stdout before printing rename warnings Nguyễn Thái Ngọc Duy
2018-01-16 19:06 ` Junio C Hamano
2018-01-17  1:04   ` Duy Nguyen

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git