[PATCH] progress: no progress in background

[PATCH] progress: no progress in background

[Luke Mewburn]

[-- Attachment #1: Type: text/plain, Size: 1682 bytes --]

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 <luke@mewburn.net>
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


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] progress: no progress in background

[Jeff King]

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 <luke@mewburn.net>
> 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

[PATCH] progress: treat "no terminal" as being in the foreground

[Jeff King]

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 <peff@peff.net>
---
 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

Re: [PATCH] progress: no progress in background

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> +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.

Yeah, thanks.  The other day when an unrelated progress issues came,
I realized that this patch has that breakage (and immediately forgot
about it ;-).

> ... Whatever the reason, it probably makes sense to err on the
> side of printing the progress.

Yup.  Thanks.