Re: [PATCH 2/5] progress: return early when in the background
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2019-03-25 11:08:56
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.
quoted hunk ↗ jump to hunk
Signed-off-by: SZEDER Gábor <redacted> --- 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(...)). * Is that "|| done" part in the "progress_update" case something that needs to happen? I.e. can we entirely skip the "setup signal handler" part in start_progress_delay() if we detect that we're not in the foreground, and then rely on the stop_progress() call to print the "done"? Although we set "progress_update = 1" in stop_progress_msg(), so it's not *just* the signal handler but also us "faking" it, and we'd still need to stash away "progress->last_value = n" in display() in that backgrounding case. So maybe it's as simple as it's going to get.