| Message ID | patch-v2-5.8-250e50667c2-20210920T225701Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | progress: assert "global_progress" + test fixes / cleanup | expand |
On Tue, Sep 21, 2021 at 01:09:26AM +0200, Ævar Arnfjörð Bjarmason wrote: > > 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 <avarab@gmail.com> > --- > 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. Reviewed-by: Emily Shaffer <emilyshaffer@google.com> > } > progress_update = 0; > } > -- > 2.33.0.1098.gf02a64c1a2d >
On Thu, Oct 07 2021, Emily Shaffer wrote: > On Tue, Sep 21, 2021 at 01:09:26AM +0200, Ævar Arnfjörð Bjarmason wrote: >> >> 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 <avarab@gmail.com> >> --- >> 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 <emilyshaffer@google.com> > >> } >> progress_update = 0; >> } >> -- >> 2.33.0.1098.gf02a64c1a2d >>
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); } progress_update = 0; }
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 <avarab@gmail.com> --- progress.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)