On Mon, Mar 25, 2019 at 12:08:47PM +0100, Ævar Arnfjörð Bjarmason wrote:

On Mon, Mar 25 2019, SZEDER Gábor wrote:

quoted

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

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

It depends on whether you consider the following case worth caring
about:

  $ git long-cmd
  <shows progress>
  Ctrl-Z!
  $ bg
  <silent>
  $ fg
  <shows progress>

Or:

  $ git long-cmd &
  <silent>
  $ fg
  <shows progress>

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)

 * 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"?

This, too, depends on how (or whether at all) we want to handle the
user sending the process to the background and bringing it back.

   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.