[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 <redacted>
Acked-by: Nicolas Pitre <nico@fluxnic.net>
---
 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

On Wed, Apr 15, 2015 at 07:34:18PM +1000, Luke Mewburn wrote:

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 <redacted>
Acked-by: Nicolas Pitre <nico@fluxnic.net>
---
[...]
+static int is_foreground_fd(int fd)
+{
+	return getpgid(0) == tcgetpgrp(fd);
+}

I've noticed that this patch causes a regression when we are
transmitting progress over the sideband channel of the git protocol. You
can see it pretty easily by cloning something large (like the kernel):

  git clone --no-local /path/to/linux.git

The "counting objects" phase is generated by the server side and sent
over the sideband, where we show it locally. So you'll get:

  remote: Counting objects: 499771

and so on, progressively, until we get to the final value. But with your
patch, we get silence for tens of seconds, and then the final value.
is_foreground_fd never returns true, and we print only once the "done"
flag is set.

The problem is that tcgetpgrp() returns -1 on the server side, because
of course there is no terminal. I suspect this may also break other
esoteric cases where "--progress" has been explicitly specified, but we
don't actually have a terminal (e.g., even something as simple as "ssh
host 'cd repo && git fsck --progress" exhibits the same behavior).

One reasonable fix (I think) would be to treat an error return from
tcgetpgrp() as "yes, we are the foreground", like:

diff --git a/progress.c b/progress.c
index 43d9228..2e31bec 100644
--- a/progress.c
+++ b/progress.c

@@ -74,7 +74,8 @@ static void clear_progress_signal(void)
 
 static int is_foreground_fd(int fd)
 {
-	return getpgid(0) == tcgetpgrp(fd);
+	int tpgrp = tcgetpgrp(fd);
+	return tpgrp < 0 || tpgrp == getpgid(0);
 }
 
 static int display(struct progress *progress, unsigned n, const char *done)

But I don't know if that messes up any other cases you were trying to
hit. We could also check that errno == ENOTTY, but I'm not sure it's
worth it. Whatever the reason, it probably makes sense to err on the
side of printing the progress.

-Peff

On Tue, May 19, 2015 at 01:17:52AM -0400, Jeff King wrote:

One reasonable fix (I think) would be to treat an error return from
tcgetpgrp() as "yes, we are the foreground", like:

I think I convinced myself this is the right fix. So here it is, all
wrapped up with a commit message. I'd love to hear confirmation, though.
:)

-- >8 --
progress: treat "no terminal" as being in the foreground

Commit 85cb890 (progress: no progress in background,
2015-04-13) avoids sending progress from background
processes by checking that the process group id of the
current process is the same as that of the controlling
terminal.

If we don't have a terminal, however, this check never
succeeds, and we print no progress at all (until the final
"done" message). This can be seen when cloning a large
repository; instead of getting progress updates for
"counting objects", it will appear to hang then print the
final count.

We can fix this by treating an error return from tcgetpgrp()
as a signal to show the progress.

Signed-off-by: Jeff King <redacted>
---
 progress.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/progress.c b/progress.c
index 43d9228..2e31bec 100644
--- a/progress.c
+++ b/progress.c

@@ -74,7 +74,8 @@ static void clear_progress_signal(void)
 
 static int is_foreground_fd(int fd)
 {
-	return getpgid(0) == tcgetpgrp(fd);
+	int tpgrp = tcgetpgrp(fd);
+	return tpgrp < 0 || tpgrp == getpgid(0);
 }
 
 static int display(struct progress *progress, unsigned n, const char *done)

-- 
2.4.1.396.g7ba6d7b