{"thread":{"id":"39082","subject":"[PATCH] progress: no progress in background","startedAt":"2015-04-15T09:34:18Z","lastAt":"2015-05-19T16:12:29Z","messageCount":4,"participants":["Luke Mewburn","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"259400","messageId":"20150415093418.GH23475@mewburn.net","threadId":"39082","inReplyTo":null,"subject":"[PATCH] progress: no progress in background","fromName":"Luke Mewburn","fromEmail":"luke@mewburn.net","sentAt":"2015-04-15T09:34:18Z","receivedAt":"2015-04-15T09:34:18Z","isPatch":true,"sender":{"key":"luke@mewburn.net","avatar":"https://avatars.githubusercontent.com/u/629143?v=4"},"body":"Disable the display of the progress if stderr is not the\ncurrent foreground process.\nStill display the final result when done.\n\nSigned-off-by: Luke Mewburn \nAcked-by: Nicolas Pitre \n---\n progress.c | 22 ++++++++++++++++------\n 1 file changed, 16 insertions(+), 6 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 412e6b1..43d9228 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -72,6 +72,11 @@ static void clear_progress_signal(void)\n \tprogress_update = 0;\n }\n \n+static int is_foreground_fd(int fd)\n+{\n+\treturn getpgid(0) == tcgetpgrp(fd);\n+}\n+\n static int display(struct progress *progress, unsigned n, const char *done)\n {\n \tconst char *eol, *tp;\n@@ -98,16 +103,21 @@ static int display(struct progress *progress, unsigned n, const char *done)\n \t\tunsigned percent = n * 100 / progress->total;\n \t\tif (percent != progress->last_percent || progress_update) {\n \t\t\tprogress->last_percent = percent;\n-\t\t\tfprintf(stderr, \"%s: %3u%% (%u/%u)%s%s\",\n-\t\t\t\tprogress->title, percent, n,\n-\t\t\t\tprogress->total, tp, eol);\n-\t\t\tfflush(stderr);\n+\t\t\tif (is_foreground_fd(fileno(stderr)) || done) {\n+\t\t\t\tfprintf(stderr, \"%s: %3u%% (%u/%u)%s%s\",\n+\t\t\t\t\tprogress->title, percent, n,\n+\t\t\t\t\tprogress->total, tp, eol);\n+\t\t\t\tfflush(stderr);\n+\t\t\t}\n \t\t\tprogress_update = 0;\n \t\t\treturn 1;\n \t\t}\n \t} else if (progress_update) {\n-\t\tfprintf(stderr, \"%s: %u%s%s\", progress->title, n, tp, eol);\n-\t\tfflush(stderr);\n+\t\tif (is_foreground_fd(fileno(stderr)) || done) {\n+\t\t\tfprintf(stderr, \"%s: %u%s%s\",\n+\t\t\t\tprogress->title, n, tp, eol);\n+\t\t\tfflush(stderr);\n+\t\t}\n \t\tprogress_update = 0;\n \t\treturn 1;\n \t}\n-- \n2.3.5.1.g63ef1a0\n\n"},{"id":"261534","messageId":"20150519051752.GA16173@peff.net","threadId":"39082","inReplyTo":"20150415093418.GH23475@mewburn.net","subject":"Re: [PATCH] progress: no progress in background","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-19T05:17:53Z","receivedAt":"2015-05-19T05:17:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 15, 2015 at 07:34:18PM +1000, Luke Mewburn wrote:\n\n> Disable the display of the progress if stderr is not the\n> current foreground process.\n> Still display the final result when done.\n> \n> Signed-off-by: Luke Mewburn \n> Acked-by: Nicolas Pitre \n> ---\n> [...]\n> +static int is_foreground_fd(int fd)\n> +{\n> +\treturn getpgid(0) == tcgetpgrp(fd);\n> +}\n\nI've noticed that this patch causes a regression when we are\ntransmitting progress over the sideband channel of the git protocol. You\ncan see it pretty easily by cloning something large (like the kernel):\n\n git clone --no-local /path/to/linux.git\n\nThe \"counting objects\" phase is generated by the server side and sent\nover the sideband, where we show it locally. So you'll get:\n\n remote: Counting objects: 499771\n\nand so on, progressively, until we get to the final value. But with your\npatch, we get silence for tens of seconds, and then the final value.\nis_foreground_fd never returns true, and we print only once the \"done\"\nflag is set.\n\nThe problem is that tcgetpgrp() returns -1 on the server side, because\nof course there is no terminal. I suspect this may also break other\nesoteric cases where \"--progress\" has been explicitly specified, but we\ndon't actually have a terminal (e.g., even something as simple as \"ssh\nhost 'cd repo && git fsck --progress\" exhibits the same behavior).\n\nOne reasonable fix (I think) would be to treat an error return from\ntcgetpgrp() as \"yes, we are the foreground\", like:\n\ndiff --git a/progress.c b/progress.c\nindex 43d9228..2e31bec 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -74,7 +74,8 @@ static void clear_progress_signal(void)\n \n static int is_foreground_fd(int fd)\n {\n-\treturn getpgid(0) == tcgetpgrp(fd);\n+\tint tpgrp = tcgetpgrp(fd);\n+\treturn tpgrp < 0 || tpgrp == getpgid(0);\n }\n \n static int display(struct progress *progress, unsigned n, const char *done)\n\nBut I don't know if that messes up any other cases you were trying to\nhit. We could also check that errno == ENOTTY, but I'm not sure it's\nworth it. Whatever the reason, it probably makes sense to err on the\nside of printing the progress.\n\n-Peff\n"},{"id":"261535","messageId":"20150519052457.GA18772@peff.net","threadId":"39082","inReplyTo":"20150519051752.GA16173@peff.net","subject":"[PATCH] progress: treat \"no terminal\" as being in the foreground","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-19T05:24:57Z","receivedAt":"2015-05-19T05:24:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 19, 2015 at 01:17:52AM -0400, Jeff King wrote:\n\n> One reasonable fix (I think) would be to treat an error return from\n> tcgetpgrp() as \"yes, we are the foreground\", like:\n\nI think I convinced myself this is the right fix. So here it is, all\nwrapped up with a commit message. I'd love to hear confirmation, though.\n:)\n\n-- >8 --\nprogress: treat \"no terminal\" as being in the foreground\n\nCommit 85cb890 (progress: no progress in background,\n2015-04-13) avoids sending progress from background\nprocesses by checking that the process group id of the\ncurrent process is the same as that of the controlling\nterminal.\n\nIf we don't have a terminal, however, this check never\nsucceeds, and we print no progress at all (until the final\n\"done\" message). This can be seen when cloning a large\nrepository; instead of getting progress updates for\n\"counting objects\", it will appear to hang then print the\nfinal count.\n\nWe can fix this by treating an error return from tcgetpgrp()\nas a signal to show the progress.\n\nSigned-off-by: Jeff King \n---\n progress.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/progress.c b/progress.c\nindex 43d9228..2e31bec 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -74,7 +74,8 @@ static void clear_progress_signal(void)\n \n static int is_foreground_fd(int fd)\n {\n-\treturn getpgid(0) == tcgetpgrp(fd);\n+\tint tpgrp = tcgetpgrp(fd);\n+\treturn tpgrp < 0 || tpgrp == getpgid(0);\n }\n \n static int display(struct progress *progress, unsigned n, const char *done)\n-- \n2.4.1.396.g7ba6d7b\n"},{"id":"261561","messageId":"xmqqwq048s2a.fsf@gitster.dls.corp.google.com","threadId":"39082","inReplyTo":"20150519051752.GA16173@peff.net","subject":"Re: [PATCH] progress: no progress in background","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-19T16:12:29Z","receivedAt":"2015-05-19T16:12:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> +static int is_foreground_fd(int fd)\n>> +{\n>> +\treturn getpgid(0) == tcgetpgrp(fd);\n>> +}\n>\n> I've noticed that this patch causes a regression when we are\n> transmitting progress over the sideband channel of the git protocol.\n\nYeah, thanks. The other day when an unrelated progress issues came,\nI realized that this patch has that breakage (and immediately forgot\nabout it ;-).\n\n> ... Whatever the reason, it probably makes sense to err on the\n> side of printing the progress.\n\nYup. Thanks.\n"}]}