Re: [PATCH v2 5/8] progress.c: stop eagerly fflush(stderr) when not a terminal

On Thu, Oct 07 2021, Emily Shaffer wrote:

On Tue, Sep 21, 2021 at 01:09:26AM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

It's the clear intention of the combination of 137a0d0ef56 (Flush
progress message buffer in display()., 2007-11-19) and
85cb8906f0e (progress: no progress in background, 2015-04-13) to call
fflush(stderr) when we have a stderr in the foreground, but we ended
up always calling fflush(stderr) seemingly by omission. Let's not.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 progress.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/progress.c b/progress.c
index 7fcc513717a..1fade5808de 100644
--- a/progress.c
+++ b/progress.c

@@ -91,7 +91,8 @@ static void display(struct progress *progress, uint64_t n, const char *done)
 	}
 
 	if (show_update) {
-		if (is_foreground_fd(fileno(stderr)) || done) {
+		int stderr_is_foreground_fd = is_foreground_fd(fileno(stderr));
+		if (stderr_is_foreground_fd || done) {
 			const char *eol = done ? done : "\r";
 			size_t clear_len = counters_sb->len < last_count_len ?
 					last_count_len - counters_sb->len + 1 :

@@ -115,7 +116,8 @@ static void display(struct progress *progress, uint64_t n, const char *done)
 				fprintf(stderr, "%s: %s%*s", progress->title,
 					counters_sb->buf, (int) clear_len, eol);
 			}
-			fflush(stderr);
+			if (stderr_is_foreground_fd)
+				fflush(stderr);

Looks like a straightforward refactor, although I wonder what's the
difference between is_foreground_fd(fileno(stderr)) and isatty() in
practice.

Good question. Whether you have a TTY is different from if it's in the
foreground. In this case we don't want progress bars to display their
full output if they're not in the foreground, just the summary line.

I.e.:
    
    perl -MPOSIX=tcgetpgrp,isatty,getpgrp -wE '
            say "TTY: ", isatty(1) ? "yes" : "no";
            open my $tty, "/dev/tty";
            my $tpgrp = tcgetpgrp(fileno($tty));
            my $pgrp = getpgrp();
            say "Foreground?: ",  $tpgrp == $pgrp ? "yes" : "no"
    '
    
Then:
    
    $ <that>
    TTY: yes
    Foreground?: yes
    $ <that> &
    TTY: yes
    Foreground?: no
    $ <that> >f && cat f
    TTY: no
    Foreground?: yes
    $ (<that> >f &); sleep 1; cat f;
    TTY: no
    Foreground?: no

But having written that I can see that this commit of mine is buggy,
because when I wrote it I conflated the two. I.e. we don't want to defer
eager flushing in that "&" case. I.e. to have our line-buffered summary
line be held up by I/O buffered flushing.

Reviewed-by: Emily Shaffer <redacted>

quoted

 		}
 		progress_update = 0;
 	}
-- 
2.33.0.1098.gf02a64c1a2d