Re: [PATCH 2/5] progress: return early when in the background
On 19-03-25 12:39, SZEDER Gábor wrote: | On Mon, Mar 25, 2019 at 12:08:47PM +0100, Ævar Arnfjörð Bjarmason wrote: | > | > On Mon, Mar 25 2019, SZEDER Gábor wrote: | > | > > When a git process runs in the background, it doesn't display | > > progress, only the final "done" line [1]. The condition to check that | > > are a bit too deep in the display() function, and thus it calculates | > > the progress percentage even when no progress will be displayed | > > anyway. | > > | > > Restructure the display() function to return early when we are in the | > > background, which prevents the unnecessary progress percentae | > > calculation, and make the function look a bit better by losing one | > > level of indentation. | > > | > > [1] 85cb8906f0 (progress: no progress in background, 2015-04-13) | > | > CC-ing the author of that patch. | > | > > Signed-off-by: SZEDER Gábor | > > --- | > > progress.c | 26 ++ | > > 1 file changed, 14 insertions(+), 12 deletions(-) | > > | > > diff --git a/progress.c b/progress.c | > > index 02a20e7d58..b57c0dae16 100644 | > > --- a/progress.c | > > +++ b/progress.c | > > @@ -86,28 +86,30 @@ static void display(struct progress *progress, uint64_t n, const char *done) | > > return; | > > | > > progress->last_value = n; | > > + | > > + if (!is_foreground_fd(fileno(stderr)) && !done) { | > > + progress_update = 0; | > > + return; | > > + } | > > + | > > tp = (progress->throughput) ? progress->throughput->display.buf : ""; | > > eol = done ? done : " \r"; | > > if (progress->total) { | > > unsigned percent = n * 100 / progress->total; | > > if (percent != progress->last_percent || progress_update) { | > > progress->last_percent = percent; | > > - if (is_foreground_fd(fileno(stderr)) || done) { | > > - fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s", | > > - progress->title, percent, | > > - (uintmax_t)n, (uintmax_t)progress->total, | > > - tp, eol); | > > - fflush(stderr); | > > - } | > > + fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s", | > > + progress->title, percent, | > > + (uintmax_t)n, (uintmax_t)progress->total, | > > + tp, eol); | > > + fflush(stderr); | > > progress_update = 0; | > > return; | > > } | > > } else if (progress_update) { | > > - if (is_foreground_fd(fileno(stderr)) || done) { | > > - fprintf(stderr, "%s: %"PRIuMAX"%s%s", | > > - progress->title, (uintmax_t)n, tp, eol); | > > - fflush(stderr); | > > - } | > > + fprintf(stderr, "%s: %"PRIuMAX"%s%s", | > > + progress->title, (uintmax_t)n, tp, eol); | > > + fflush(stderr); | > > progress_update = 0; | > > return; | > > } | > | > This patch looks good, just notes for potential follow-up: | > | > * Is the "is_foreground_fd(fileno(stderr))" case worth moving into | >start_progress_delay() & setting a variable? It's a few C lib calls & | >potential syscall (getpid(...)). | | It depends on whether you consider the following case worth caring | about: | | $ git long-cmd | | Ctrl-Z! | $ bg | | $ fg | | | Or: | | $ git long-cmd & | | $ fg | | | By moving the is_foreground_fd() check to start_progress_delay() and | caching its result, in the first case we would print progress even | after the process is sent to the background, while in the second we | wouldn't print progress even after the initially backgrounded process | is brought to the foreground. | | I think the current behavior makes sense (though I'm not quite sure | about printing the final "done" line, as I think I would be annoyed if | it were printed from the background process while I was typing a | longer command... but I don't run git commands in the background in | the first place) You've described the current behaviour as I intended it in the original patch. I.e,: - Display progress if foreground. - Suppress output if background. - Check the foreground/background state each update in case it changed. I based that on other tools that also dynamically change their output/progress behaviour whether in the fo
Re: [PATCH] compat/mingw: stubs for getpgid() and tcgetpgrp()
On Wed, Apr 15, 2015 at 08:29:30PM +0200, Johannes Sixt wrote: | Windows does not have process groups. It is, therefore, the simplest | to pretend that each process is in its own process group. | | [...] | | diff --git a/compat/mingw.h b/compat/mingw.h | index 7b523cf..a552026 100644 | @@ -118,6 +116,12 @@ static inline int sigaddset(sigset_t *set, int signum) | #define SIG_UNBLOCK 0 | static inline int sigprocmask(int how, const sigset_t *set, sigset_t *oldset) | { return 0; } | +static inline pid_t getppid(void) | +{ return 1; } | +static inline pid_t getpgid(pid_t pid) | +{ return pid == 0 ? getpid() : pid; } | +static inline pid_t tcgetpgrp(int fd) | +{ return getpid(); } This appears to be similar to the approach that tcsh uses too; return the current process ID for the process group ID. See https://github.com/tcsh-org/tcsh/blob/master/win32/ntport.h for tcsh's implementation of getpgrp() (a variation of getpgid()) and tcgetpgrp(). regards, Luke. pgpyN0nrBi1n3.pgp Description: PGP signature
[PATCH] progress: no progress in background
Disable the display of the progress if stderr is not the current foreground process. Still display the final result when done. Signed-off-by: Luke Mewburn Acked-by: Nicolas Pitre --- progress.c | 22 -- 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/progress.c b/progress.c index 412e6b1..43d9228 100644 --- a/progress.c +++ b/progress.c @@ -72,6 +72,11 @@ static void clear_progress_signal(void) progress_update = 0; } +static int is_foreground_fd(int fd) +{ + return getpgid(0) == tcgetpgrp(fd); +} + static int display(struct progress *progress, unsigned n, const char *done) { const char *eol, *tp; @@ -98,16 +103,21 @@ static int display(struct progress *progress, unsigned n, const char *done) unsigned percent = n * 100 / progress->total; if (percent != progress->last_percent || progress_update) { progress->last_percent = percent; - fprintf(stderr, "%s: %3u%% (%u/%u)%s%s", - progress->title, percent, n, - progress->total, tp, eol); - fflush(stderr); + if (is_foreground_fd(fileno(stderr)) || done) { + fprintf(stderr, "%s: %3u%% (%u/%u)%s%s", + progress->title, percent, n, + progress->total, tp, eol); + fflush(stderr); + } progress_update = 0; return 1; } } else if (progress_update) { - fprintf(stderr, "%s: %u%s%s", progress->title, n, tp, eol); - fflush(stderr); + if (is_foreground_fd(fileno(stderr)) || done) { + fprintf(stderr, "%s: %u%s%s", + progress->title, n, tp, eol); + fflush(stderr); + } progress_update = 0; return 1; } -- 2.3.5.1.g63ef1a0 pgpGj_5bLCqHt.pgp Description: PGP signature
Re: [PATCH] reduce progress updates in background
On Mon, Apr 13, 2015 at 11:01:04AM -0400, Nicolas Pitre wrote: | > That's what happens; the suppression only occurs if the process is | > currently background. If I start a long-running operation (such as "git | > fsck"), the progress is displayed. I then suspend & background, and the | > progress is suppressed. If I resume the process in the foreground, the | > progress starts to display again at the appropriate point. | | I agree. I was just comenting on your suggestion about caching the | in_progress_fd() result which would prevent that. Ahh. My suggestion about is_foreground_fd() result caching within struct progress was only about caching the getpgid(0) portion of the test (as that's not expected to change for the life of the process), and not the tcgetpgrp(fd) portion. I.e, add 'int curpgid' to struct progress, set that to getpgid(0) in start_progress_display(), and compare tcgetpgrp(fd) against progress->curpgid. In any case, I think it's a micro optimisation not worth worrying about at this point, given is_foreground_fd() is only called each time the output would change, per your feedback. regards, Luke. pgpGbPBkemS0K.pgp Description: PGP signature
[PATCH v2] reduce progress updates in background
Updated patch where is_foreground_fd() is only called in display() just before the output is to be displayed. From d87997509fc631b8cdc7db63f289102d6ddfe933 Mon Sep 17 00:00:00 2001 From: Luke Mewburn Date: Mon, 13 Apr 2015 23:30:51 +1000 Subject: [PATCH] progress: no progress in background Disable the display of the progress if stderr is not the current foreground process. Still display the final result when done. Signed-off-by: Luke Mewburn --- progress.c | 22 -- 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/progress.c b/progress.c index 412e6b1..43d9228 100644 --- a/progress.c +++ b/progress.c @@ -72,6 +72,11 @@ static void clear_progress_signal(void) progress_update = 0; } +static int is_foreground_fd(int fd) +{ + return getpgid(0) == tcgetpgrp(fd); +} + static int display(struct progress *progress, unsigned n, const char *done) { const char *eol, *tp; @@ -98,16 +103,21 @@ static int display(struct progress *progress, unsigned n, const char *done) unsigned percent = n * 100 / progress->total; if (percent != progress->last_percent || progress_update) { progress->last_percent = percent; - fprintf(stderr, "%s: %3u%% (%u/%u)%s%s", - progress->title, percent, n, - progress->total, tp, eol); - fflush(stderr); + if (is_foreground_fd(fileno(stderr)) || done) { + fprintf(stderr, "%s: %3u%% (%u/%u)%s%s", + progress->title, percent, n, + progress->total, tp, eol); + fflush(stderr); + } progress_update = 0; return 1; } } else if (progress_update) { - fprintf(stderr, "%s: %u%s%s", progress->title, n, tp, eol); - fflush(stderr); + if (is_foreground_fd(fileno(stderr)) || done) { + fprintf(stderr, "%s: %u%s%s", + progress->title, n, tp, eol); + fflush(stderr); + } progress_update = 0; return 1; } -- 2.3.5.1.gd879975 pgpxkGmuIlerx.pgp Description: PGP signature
Re: [PATCH] reduce progress updates in background
On Mon, Apr 13, 2015 at 10:11:09AM -0400, Nicolas Pitre wrote: | What if you suspend the task and push it into the background? Would be | nice to inhibit progress display in that case, and resume it if the task | returns to the foreground. That's what happens; the suppression only occurs if the process is currently background. If I start a long-running operation (such as "git fsck"), the progress is displayed. I then suspend & background, and the progress is suppressed. If I resume the process in the foreground, the progress starts to display again at the appropriate point. In the proposed patch, the stop_progress display for a given progress (i.e. the one that ends in ", done.") is displayed even if in the background so that there's some indication of progress. E.g. Checking object directories: 100% (256/256), done. Checking objects: 100% (184664/184664), done. Checking connectivity: 184667, done. This is the test 'if (is_foreground || done)'. I'm not 100% happy with my choice here, and the simpler behaviour of "suppress all background progress output" can be achieved by removing '|| done' from those two tests. That still doesn't suppress _all_ output whilst in the background. In order to do that, a larger refactor of various warning methods would be required. I would argue that's a separate orthoganal fix. | Also the display() function may be called quite a lot without | necessarily resulting in a display output. Therefore I'd suggest adding | in_progress_fd() to the if condition right before the printf() instead. That's an easy enough change to make (although I speculate that the testing of the foreground status is not that big a performance issue, especially compared the the existing performance "overhead" of printing the progress to stderr then forcing a flush :) Should I submit a revised patch with (1) call in_progress_fd() just before the fprintf() as requested, and (2) suppress all display output including the "done" call. ? regards, Luke. pgpTyBk7RgIJs.pgp Description: PGP signature
[PATCH] reduce progress updates in background
Hi, I've noticed that when a long-running git operation that generates progress output is suspended and converted to a background process, the terminal still gets spammed with progress updates (to stderr). Many years ago I fixed a similar issue in the NetBSD ftp progress bar code (which I wrote). I've experimented around with a couple of different solutions, including: 1. suppress all progress output whilst in the background 2. suppress "in progress" updates whilst in the background, but display the "done" message even if in the background. In both cases, warnings were still output to the terminal. I've attached a patch that implements (2) above. If the consensus is that all progress messages should be suppressed, I can provide the (simpler) patch for that. I've explicitly separated the in_progress_fd() function so that it's easier to (a) reuse elsewhere where appropriate, and (b) make any portability changes to the test if necessary. I also used getpgid(0) versus getpgrp() to avoid portability issues with the signature in the latter with pre-POSIX. A minor optimisation could be to pass in struct progress * and to cache getpgid(0) in a member of struct progress in start_progress_delay(), since this value shouldn't change during the life of the process. regards, Luke. From 843a367bac87674666dafbaf7fdb7d6b0e1660f7 Mon Sep 17 00:00:00 2001 From: Luke Mewburn Date: Mon, 13 Apr 2015 23:30:51 +1000 Subject: [PATCH] progress: no progress in background Disable the display of the progress if stderr is not the current foreground process. Still display the final result when done. Signed-off-by: Luke Mewburn --- progress.c | 23 +-- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/progress.c b/progress.c index 412e6b1..8094404 100644 --- a/progress.c +++ b/progress.c @@ -72,9 +72,15 @@ static void clear_progress_signal(void) progress_update = 0; } +static int is_foreground_fd(int fd) +{ + return getpgid(0) == tcgetpgrp(fd); +} + static int display(struct progress *progress, unsigned n, const char *done) { const char *eol, *tp; + const int is_foreground = is_foreground_fd(fileno(stderr)); if (progress->delay) { if (!progress_update || --progress->delay) @@ -98,16 +104,21 @@ static int display(struct progress *progress, unsigned n, const char *done) unsigned percent = n * 100 / progress->total; if (percent != progress->last_percent || progress_update) { progress->last_percent = percent; - fprintf(stderr, "%s: %3u%% (%u/%u)%s%s", - progress->title, percent, n, - progress->total, tp, eol); - fflush(stderr); + if (is_foreground || done) { + fprintf(stderr, "%s: %3u%% (%u/%u)%s%s", + progress->title, percent, n, + progress->total, tp, eol); + fflush(stderr); + } progress_update = 0; return 1; } } else if (progress_update) { - fprintf(stderr, "%s: %u%s%s", progress->title, n, tp, eol); - fflush(stderr); + if (is_foreground || done) { + fprintf(stderr, "%s: %u%s%s", + progress->title, n, tp, eol); + fflush(stderr); + } progress_update = 0; return 1; } -- 2.3.5.dirty pgp9CkCjLO94G.pgp Description: PGP signature