Re: [PATCH] progress: no progress in background

From: Jeff King <peff@peff.net>
Date: May 19, 2015, 05:17 UTC
Message-ID: <20150519051752.GA16173@peff.net>
In-Reply-To: <20150415093418.GH23475@mewburn.net>

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

Show 12 quoted lines

> 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