{"thread":{"id":"39060","subject":"[PATCH] reduce progress updates in background","startedAt":"2015-04-13T13:48:50Z","lastAt":"2015-04-24T06:25:12Z","messageCount":20,"participants":["Luke Mewburn","Nicolas Pitre","brian m. carlson","Johannes Schindelin","Johannes Sixt","Junio C Hamano","Erik Faye-Lund","rupert thurner"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"259289","messageId":"20150413134850.GC23475@mewburn.net","threadId":"39060","inReplyTo":null,"subject":"[PATCH] reduce progress updates in background","fromName":"Luke Mewburn","fromEmail":"luke@mewburn.net","sentAt":"2015-04-13T13:48:50Z","receivedAt":"2015-04-13T13:48:50Z","isPatch":true,"sender":{"key":"luke@mewburn.net","avatar":"https://avatars.githubusercontent.com/u/629143?v=4"},"body":"Hi,\n\nI've noticed that when a long-running git operation that generates\nprogress output is suspended and converted to a background process,\nthe terminal still gets spammed with progress updates (to stderr).\n\nMany years ago I fixed a similar issue in the NetBSD ftp progress\nbar code (which I wrote).\n\nI've experimented around with a couple of different solutions, including:\n1. suppress all progress output whilst in the background\n2. suppress \"in progress\" updates whilst in the background,\n   but display the \"done\" message even if in the background.\n\nIn both cases, warnings were still output to the terminal.\n\nI've attached a patch that implements (2) above.\n\nIf the consensus is that all progress messages should be suppressed,\nI can provide the (simpler) patch for that.\n\nI've explicitly separated the in_progress_fd() function\nso that it's easier to (a) reuse elsewhere where appropriate,\nand (b) make any portability changes to the test if necessary.\nI also used getpgid(0) versus getpgrp() to avoid portability\nissues with the signature in the latter with pre-POSIX.\n\nA minor optimisation could be to pass in struct progress *\nand to cache getpgid(0) in a member of struct progress\nin start_progress_delay(), since this value shouldn't change\nduring the life of the process.\n\nregards,\nLuke.\n\n\nFrom 843a367bac87674666dafbaf7fdb7d6b0e1660f7 Mon Sep 17 00:00:00 2001\nFrom: Luke Mewburn <luke@mewburn.net>\nDate: Mon, 13 Apr 2015 23:30:51 +1000\nSubject: [PATCH] progress: no progress in background\n\nDisable 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 <luke@mewburn.net>\n---\n progress.c | 23 +++++++++++++++++------\n 1 file changed, 17 insertions(+), 6 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 412e6b1..8094404 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -72,9 +72,15 @@ 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+\tconst int is_foreground = is_foreground_fd(fileno(stderr));\n \n \tif (progress->delay) {\n \t\tif (!progress_update || --progress->delay)\n@@ -98,16 +104,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 || 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 || 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.dirty\n\n"},{"id":"259292","messageId":"alpine.LFD.2.11.1504130954420.5619@knanqh.ubzr","threadId":"39060","inReplyTo":"20150413134850.GC23475@mewburn.net","subject":"Re: [PATCH] reduce progress updates in background","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2015-04-13T14:11:09Z","receivedAt":"2015-04-13T14:11:09Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 13 Apr 2015, Luke Mewburn wrote:\n\n> Hi,\n> \n> I've noticed that when a long-running git operation that generates\n> progress output is suspended and converted to a background process,\n> the terminal still gets spammed with progress updates (to stderr).\n> \n> Many years ago I fixed a similar issue in the NetBSD ftp progress\n> bar code (which I wrote).\n> \n> I've experimented around with a couple of different solutions, including:\n> 1. suppress all progress output whilst in the background\n> 2. suppress \"in progress\" updates whilst in the background,\n>    but display the \"done\" message even if in the background.\n> \n> In both cases, warnings were still output to the terminal.\n> \n> I've attached a patch that implements (2) above.\n> \n> If the consensus is that all progress messages should be suppressed,\n> I can provide the (simpler) patch for that.\n> \n> I've explicitly separated the in_progress_fd() function\n> so that it's easier to (a) reuse elsewhere where appropriate,\n> and (b) make any portability changes to the test if necessary.\n> I also used getpgid(0) versus getpgrp() to avoid portability\n> issues with the signature in the latter with pre-POSIX.\n> \n> A minor optimisation could be to pass in struct progress *\n> and to cache getpgid(0) in a member of struct progress\n> in start_progress_delay(), since this value shouldn't change\n> during the life of the process.\n\nWhat if you suspend the task and push it into the background? Would be \nnice to inhibit progress display in that case, and resume it if the task \nreturns to the foreground.\n\nAlso the display() function may be called quite a lot without \nnecessarily resulting in a display output. Therefore I'd suggest adding \nin_progress_fd() to the if condition right before the printf() instead.\n\n\nNicolas\n"},{"id":"259293","messageId":"20150413144039.GD23475@mewburn.net","threadId":"39060","inReplyTo":"alpine.LFD.2.11.1504130954420.5619@knanqh.ubzr","subject":"Re: [PATCH] reduce progress updates in background","fromName":"Luke Mewburn","fromEmail":"luke@mewburn.net","sentAt":"2015-04-13T14:40:39Z","receivedAt":"2015-04-13T14:40:39Z","isPatch":true,"sender":{"key":"luke@mewburn.net","avatar":"https://avatars.githubusercontent.com/u/629143?v=4"},"body":"On Mon, Apr 13, 2015 at 10:11:09AM -0400, Nicolas Pitre wrote:\n  | What if you suspend the task and push it into the background? Would be \n  | nice to inhibit progress display in that case, and resume it if the task \n  | returns to the foreground.\n\nThat's what happens; the suppression only occurs if the process is\ncurrently background.  If I start a long-running operation (such as \"git\nfsck\"), the progress is displayed. I then suspend & background, and the\nprogress is suppressed.  If I resume the process in the foreground, the\nprogress starts to display again at the appropriate point.\n\nIn the proposed patch, the stop_progress display for a given progress\n(i.e. the one that ends in \", done.\") is displayed even if in the\nbackground so that there's some indication of progress. E.g.\n  Checking object directories: 100% (256/256), done.\n  Checking objects: 100% (184664/184664), done.\n  Checking connectivity: 184667, done.\nThis is the test 'if (is_foreground || done)'.\n\nI'm not 100% happy with my choice here, and the simpler behaviour\nof \"suppress all background progress output\" can be achieved by\nremoving '|| done' from those two tests.\n\nThat still doesn't suppress _all_ output whilst in the background.\nIn order to do that, a larger refactor of various warning methods\nwould be required. I would argue that's a separate orthoganal fix.\n\n\n  | Also the display() function may be called quite a lot without \n  | necessarily resulting in a display output. Therefore I'd suggest adding \n  | in_progress_fd() to the if condition right before the printf() instead.\n\nThat's an easy enough change to make (although I speculate that the\ntesting of the foreground status is not that big a performance issue,\nespecially compared the the existing performance \"overhead\" of printing\nthe progress to stderr then forcing a flush :)\n\n\nShould I submit a revised patch with\n(1) call in_progress_fd() just before the fprintf() as requested, and\n(2) suppress all display output including the \"done\" call.\n?\n\n\nregards,\nLuke.\n"},{"id":"259294","messageId":"alpine.LFD.2.11.1504131052090.5619@knanqh.ubzr","threadId":"39060","inReplyTo":"20150413144039.GD23475@mewburn.net","subject":"Re: [PATCH] reduce progress updates in background","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2015-04-13T15:01:04Z","receivedAt":"2015-04-13T15:01:04Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 14 Apr 2015, Luke Mewburn wrote:\n\n> On Mon, Apr 13, 2015 at 10:11:09AM -0400, Nicolas Pitre wrote:\n>   | What if you suspend the task and push it into the background? Would be \n>   | nice to inhibit progress display in that case, and resume it if the task \n>   | returns to the foreground.\n> \n> That's what happens; the suppression only occurs if the process is\n> currently background.  If I start a long-running operation (such as \"git\n> fsck\"), the progress is displayed. I then suspend & background, and the\n> progress is suppressed.  If I resume the process in the foreground, the\n> progress starts to display again at the appropriate point.\n\nI agree. I was just comenting on your suggestion about caching the \nin_progress_fd() result which would prevent that.\n\n> In the proposed patch, the stop_progress display for a given progress\n> (i.e. the one that ends in \", done.\") is displayed even if in the\n> background so that there's some indication of progress. E.g.\n>   Checking object directories: 100% (256/256), done.\n>   Checking objects: 100% (184664/184664), done.\n>   Checking connectivity: 184667, done.\n> This is the test 'if (is_foreground || done)'.\n\nYes.  And I think this is nice.\n\n>   | Also the display() function may be called quite a lot without \n>   | necessarily resulting in a display output. Therefore I'd suggest adding \n>   | in_progress_fd() to the if condition right before the printf() instead.\n> \n> That's an easy enough change to make (although I speculate that the\n> testing of the foreground status is not that big a performance issue,\n> especially compared the the existing performance \"overhead\" of printing\n> the progress to stderr then forcing a flush :)\n\nSure.  But what I'm saying is that progress() may be called a thousand \ntimes and only one or two of those calls will result in an actual \nprint-out. So it is best to test the foreground status only at that \npoint.\n\n> Should I submit a revised patch with\n> (1) call in_progress_fd() just before the fprintf() as requested, and\n> (2) suppress all display output including the \"done\" call.\n> ?\n\nI'd suggest (1) but not (2).\n\n\nNicolas\n"},{"id":"259320","messageId":"20150414031227.GB591600@vauxhall.crustytoothpaste.net","threadId":"39060","inReplyTo":"20150413134850.GC23475@mewburn.net","subject":"Re: [PATCH] reduce progress updates in background","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-04-14T03:12:28Z","receivedAt":"2015-04-14T03:12:28Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Mon, Apr 13, 2015 at 11:48:50PM +1000, Luke Mewburn wrote:\n> Hi,\n> \n> I've noticed that when a long-running git operation that generates\n> progress output is suspended and converted to a background process,\n> the terminal still gets spammed with progress updates (to stderr).\n> \n> I've explicitly separated the in_progress_fd() function\n> so that it's easier to (a) reuse elsewhere where appropriate,\n> and (b) make any portability changes to the test if necessary.\n> I also used getpgid(0) versus getpgrp() to avoid portability\n> issues with the signature in the latter with pre-POSIX.\n\nI like this patch.  It's simple and seems like a sensible change, and I\nappreciated the opportunity to learn about tcgetpgrp(3).  The Windows\nfolks will probably need to stub that function out, but they're no worse\noff than they were before.\n\nI do agree with Nicolas that optimizing the code to avoid calling\nin_progress_fd as much as possible is a good idea, since system calls\ncan be expensive on some systems.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"259331","messageId":"4c39d8b53964eadeb80f4898c525c42c@www.dscho.org","threadId":"39060","inReplyTo":"20150414031227.GB591600@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH] reduce progress updates in background","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-04-14T08:47:33Z","receivedAt":"2015-04-14T08:47:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Brian,\n\nOn 2015-04-14 05:12, brian m. carlson wrote:\n> On Mon, Apr 13, 2015 at 11:48:50PM +1000, Luke Mewburn wrote:\n>\n> I appreciated the opportunity to learn about tcgetpgrp(3).  The Windows\n> folks will probably need to stub that function out, but they're no worse\n> off than they were before.\n\nThanks for thinking of us!\n\nCiao,\nDscho\n"},{"id":"259338","messageId":"20150414110312.GE23475@mewburn.net","threadId":"39060","inReplyTo":"alpine.LFD.2.11.1504131052090.5619@knanqh.ubzr","subject":"[PATCH v2] reduce progress updates in background","fromName":"Luke Mewburn","fromEmail":"luke@mewburn.net","sentAt":"2015-04-14T11:03:13Z","receivedAt":"2015-04-14T11:03:13Z","isPatch":true,"sender":{"key":"luke@mewburn.net","avatar":"https://avatars.githubusercontent.com/u/629143?v=4"},"body":"Updated patch where is_foreground_fd() is only called in display()\njust before the output is to be displayed.\n\n\nFrom d87997509fc631b8cdc7db63f289102d6ddfe933 Mon Sep 17 00:00:00 2001\nFrom: Luke Mewburn <luke@mewburn.net>\nDate: Mon, 13 Apr 2015 23:30:51 +1000\nSubject: [PATCH] progress: no progress in background\n\nDisable 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 <luke@mewburn.net>\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.gd879975\n\n"},{"id":"259339","messageId":"20150414110814.GF23475@mewburn.net","threadId":"39060","inReplyTo":"alpine.LFD.2.11.1504131052090.5619@knanqh.ubzr","subject":"Re: [PATCH] reduce progress updates in background","fromName":"Luke Mewburn","fromEmail":"luke@mewburn.net","sentAt":"2015-04-14T11:08:14Z","receivedAt":"2015-04-14T11:08:14Z","isPatch":true,"sender":{"key":"luke@mewburn.net","avatar":"https://avatars.githubusercontent.com/u/629143?v=4"},"body":"On Mon, Apr 13, 2015 at 11:01:04AM -0400, Nicolas Pitre wrote:\n  | > That's what happens; the suppression only occurs if the process is\n  | > currently background.  If I start a long-running operation (such as \"git\n  | > fsck\"), the progress is displayed. I then suspend & background, and the\n  | > progress is suppressed.  If I resume the process in the foreground, the\n  | > progress starts to display again at the appropriate point.\n  | \n  | I agree. I was just comenting on your suggestion about caching the \n  | in_progress_fd() result which would prevent that.\n\nAhh.  My suggestion about is_foreground_fd() result caching within\nstruct progress was only about caching the getpgid(0) portion of the\ntest (as that's not expected to change for the life of the process), and\nnot the tcgetpgrp(fd) portion.  I.e, add 'int curpgid' to struct\nprogress, set that to getpgid(0) in start_progress_display(), and\ncompare tcgetpgrp(fd) against progress->curpgid.\n\nIn any case, I think it's a micro optimisation not worth worrying about\nat this point, given is_foreground_fd() is only called each time the\noutput would change, per your feedback.\n\nregards,\nLuke.\n"},{"id":"259352","messageId":"alpine.LFD.2.11.1504141116260.5619@knanqh.ubzr","threadId":"39060","inReplyTo":"20150414110312.GE23475@mewburn.net","subject":"Re: [PATCH v2] reduce progress updates in background","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2015-04-14T15:16:49Z","receivedAt":"2015-04-14T15:16:49Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 14 Apr 2015, Luke Mewburn wrote:\n\n> Updated patch where is_foreground_fd() is only called in display()\n> just before the output is to be displayed.\n\nAcked-by: Nicolas Pitre <nico@fluxnic.net>\n\n\n> \n"},{"id":"259435","messageId":"552EAE0A.3040208@kdbg.org","threadId":"39060","inReplyTo":"20150414110312.GE23475@mewburn.net","subject":"[PATCH] compat/mingw: stubs for getpgid() and tcgetpgrp()","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2015-04-15T18:29:30Z","receivedAt":"2015-04-15T18:29:30Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Windows does not have process groups. It is, therefore, the simplest\nto pretend that each process is in its own process group.\n\nWhile here, move the getppid() stub from its old location (between\ntwo sync related functions) next to the two new functions.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n compat/mingw.h | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex 7b523cf..a552026 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -95,8 +95,6 @@ static inline unsigned int alarm(unsigned int seconds)\n { return 0; }\n static inline int fsync(int fd)\n { return _commit(fd); }\n-static inline pid_t getppid(void)\n-{ return 1; }\n static inline void sync(void)\n {}\n static inline uid_t getuid(void)\n@@ -118,6 +116,12 @@ static inline int sigaddset(sigset_t *set, int signum)\n #define SIG_UNBLOCK 0\n static inline int sigprocmask(int how, const sigset_t *set, sigset_t *oldset)\n { return 0; }\n+static inline pid_t getppid(void)\n+{ return 1; }\n+static inline pid_t getpgid(pid_t pid)\n+{ return pid == 0 ? getpid() : pid; }\n+static inline pid_t tcgetpgrp(int fd)\n+{ return getpid(); }\n \n /*\n  * simple adaptors\n-- \n2.3.2.245.gb5bf9d3\n"},{"id":"259437","messageId":"xmqq3841kz32.fsf@gitster.dls.corp.google.com","threadId":"39060","inReplyTo":"552EAE0A.3040208@kdbg.org","subject":"Re: [PATCH] compat/mingw: stubs for getpgid() and tcgetpgrp()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-15T18:48:01Z","receivedAt":"2015-04-15T18:48:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> Windows does not have process groups. It is, therefore, the simplest\n> to pretend that each process is in its own process group.\n>\n> While here, move the getppid() stub from its old location (between\n> two sync related functions) next to the two new functions.\n>\n> Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n> ---\n\nThanks for a quick update.\n\nThe patch should do for now, but I suspect that it may give us a\nbetter abstraction to make the \"is_foreground_fd(int fd)\" or even\n\"is_foreground(void)\" the public API that would be implemented as\n\n\tint we_are_in_the_foreground(void)\n        {\n\t\treturn getpgid(0) == tcgetpgrp(fileno(stderr));\n\t}\n\nin POSIX and Windows can implement entirely differently.\n\nThoughts?\n\n>  compat/mingw.h | 8 ++++++--\n>  1 file changed, 6 insertions(+), 2 deletions(-)\n>\n> diff --git a/compat/mingw.h b/compat/mingw.h\n> index 7b523cf..a552026 100644\n> --- a/compat/mingw.h\n> +++ b/compat/mingw.h\n> @@ -95,8 +95,6 @@ static inline unsigned int alarm(unsigned int seconds)\n>  { return 0; }\n>  static inline int fsync(int fd)\n>  { return _commit(fd); }\n> -static inline pid_t getppid(void)\n> -{ return 1; }\n>  static inline void sync(void)\n>  {}\n>  static inline uid_t getuid(void)\n> @@ -118,6 +116,12 @@ static inline int sigaddset(sigset_t *set, int signum)\n>  #define SIG_UNBLOCK 0\n>  static inline int sigprocmask(int how, const sigset_t *set, sigset_t *oldset)\n>  { return 0; }\n> +static inline pid_t getppid(void)\n> +{ return 1; }\n> +static inline pid_t getpgid(pid_t pid)\n> +{ return pid == 0 ? getpid() : pid; }\n> +static inline pid_t tcgetpgrp(int fd)\n> +{ return getpid(); }\n>  \n>  /*\n>   * simple adaptors\n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"Git for Windows\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/d/optout.\n"},{"id":"259440","messageId":"CABPQNSZ-7FAgun8mxqXYWgy+Xc9xQMXKZonwujXb5WzLCKmNMw@mail.gmail.com","threadId":"39060","inReplyTo":"552EAE0A.3040208@kdbg.org","subject":"Re: [PATCH] compat/mingw: stubs for getpgid() and tcgetpgrp()","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2015-04-15T19:43:09Z","receivedAt":"2015-04-15T19:43:09Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, Apr 15, 2015 at 8:29 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n> Windows does not have process groups. It is, therefore, the simplest\n> to pretend that each process is in its own process group.\n\nWindows does have some concept of process groups, but probably not\nquite what you want:\n\nhttps://msdn.microsoft.com/en-us/library/windows/desktop/ms682083%28v=vs.85%29.aspx\n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"Git for Windows\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/d/optout.\n"},{"id":"259445","messageId":"552ECB61.3020502@kdbg.org","threadId":"39060","inReplyTo":"xmqq3841kz32.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] compat/mingw: stubs for getpgid() and tcgetpgrp()","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2015-04-15T20:34:41Z","receivedAt":"2015-04-15T20:34:41Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 15.04.2015 um 20:48 schrieb Junio C Hamano:\n> The patch should do for now, but I suspect that it may give us a\n> better abstraction to make the \"is_foreground_fd(int fd)\" or even\n> \"is_foreground(void)\" the public API that would be implemented as\n>\n> \tint we_are_in_the_foreground(void)\n>          {\n> \t\treturn getpgid(0) == tcgetpgrp(fileno(stderr));\n> \t}\n>\n> in POSIX and Windows can implement entirely differently.\n>\n> Thoughts?\n\nIMHO, this level of abstraction goes too far. It may be that I am not \nfamiliar with process groups, and I find the implementation of \nis_foreground_fd() in progress.c close to where it's used quite \neducating and enlightening. Hiding a similar implementation miles away \nin cache.h and/or compat/ would not pay off for me.\n\n-- Hannes\n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"Git for Windows\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/d/optout.\n"},{"id":"259490","messageId":"fac9a525a2fb2b09d176243659cbf3a7@www.dscho.org","threadId":"39060","inReplyTo":"CABPQNSZ-7FAgun8mxqXYWgy+Xc9xQMXKZonwujXb5WzLCKmNMw@mail.gmail.com","subject":"Re: [msysGit] [PATCH] compat/mingw: stubs for getpgid() and tcgetpgrp()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-04-16T12:44:56Z","receivedAt":"2015-04-16T12:44:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi kusma,\n\nOn 2015-04-15 21:43, Erik Faye-Lund wrote:\n> On Wed, Apr 15, 2015 at 8:29 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n>> Windows does not have process groups. It is, therefore, the simplest\n>> to pretend that each process is in its own process group.\n> \n> Windows does have some concept of process groups, but probably not\n> quite what you want:\n> \n> https://msdn.microsoft.com/en-us/library/windows/desktop/ms682083%28v=vs.85%29.aspx\n\nYes, and we actually need that in Git for Windows anyway because shooting down a process does not kill its child processes:\n\nhttps://github.com/git-for-windows/msys2-runtime/commit/15f209511985092588b171703e5046eba937b47b#diff-8753cda163376cee6c80aab11eb8701fR402\n\nHowever, using this code for `getppid()` would be serious overkill (not to mention an unbearable performance hit because you have to enumerate *all* processes to get that information).\n\nCiao,\nDscho\n"},{"id":"259491","messageId":"02c7702c204914ddded4014f292a90bf@www.dscho.org","threadId":"39060","inReplyTo":"xmqq3841kz32.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] compat/mingw: stubs for getpgid() and tcgetpgrp()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-04-16T12:48:54Z","receivedAt":"2015-04-16T12:48:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn 2015-04-15 20:48, Junio C Hamano wrote:\n> Johannes Sixt <j6t@kdbg.org> writes:\n> \n>> Windows does not have process groups. It is, therefore, the simplest\n>> to pretend that each process is in its own process group.\n>>\n>> While here, move the getppid() stub from its old location (between\n>> two sync related functions) next to the two new functions.\n>>\n>> Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n>> ---\n> \n> Thanks for a quick update.\n> \n> The patch should do for now, but I suspect that it may give us a\n> better abstraction to make the \"is_foreground_fd(int fd)\" or even\n> \"is_foreground(void)\" the public API that would be implemented as\n> \n> \tint we_are_in_the_foreground(void)\n>         {\n> \t\treturn getpgid(0) == tcgetpgrp(fileno(stderr));\n> \t}\n> \n> in POSIX and Windows can implement entirely differently.\n\nI really like it. We already require a couple of workarounds to be able to use `mintty` (which is a replacement terminal emulator that is more flexible than the default Windows terminal window, but it comes at the price that most Win32 programs think they are not running interactively inside a `mintty` session). I would really like to avoid having to finagle some really ugly code into `getpgid()` to make that test work.\n\nIn general, I find it rewarding not only from a portability point of view, but *especially* from a readability one if the code contains functions that are named semantically, i.e. they describe *why* they are called, not *how* they answer the question.\n\nIn this case, I would prefer the `is_foreground_fd(int fd)` one, but I am sure I can make the other signature work for us, too.\n\nCiao,\nDscho\n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"Git for Windows\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/d/optout.\n"},{"id":"259499","messageId":"xmqq1tjkjeyh.fsf@gitster.dls.corp.google.com","threadId":"39060","inReplyTo":"02c7702c204914ddded4014f292a90bf@www.dscho.org","subject":"Re: [PATCH] compat/mingw: stubs for getpgid() and tcgetpgrp()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-16T15:00:22Z","receivedAt":"2015-04-16T15:00:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n\n> On 2015-04-15 20:48, Junio C Hamano wrote:\n>\n>> The patch should do for now, but I suspect that it may give us a\n>> better abstraction to make the \"is_foreground_fd(int fd)\" or even\n>> \"is_foreground(void)\" the public API that would be implemented as\n>> \n>> \tint we_are_in_the_foreground(void)\n>>         {\n>> \t\treturn getpgid(0) == tcgetpgrp(fileno(stderr));\n>> \t}\n>> \n>> in POSIX and Windows can implement entirely differently.\n> ...\n> In general, I find it rewarding not only from a portability point of\n> view, but *especially* from a readability one if the code contains\n> functions that are named semantically, i.e. they describe *why* they\n> are called, not *how* they answer the question.\n\nYeah, that was the rationale behind the suggestion (i.e. how may be\ndifferent depending on the platform, and the main code flow cares\nmore about why and not how).\n\nI'll queue Luke's patch with J6t's compat/ for now, but if you find\nmore suitable implementation for the higher-level abstraction,\nplease do send a follow-up patch to update it.\n\nTwo Johannes'es may need to talk between themselves to agree what\nthe right level of abstraction is, though.  I trust two of you a lot\nmore than my gut feeling when it comes to POSIX vs Windows API\ndifferences ;-)\n\nThanks.\n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"Git for Windows\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/d/optout.\n"},{"id":"259551","messageId":"20150417012541.GI23475@mewburn.net","threadId":"39060","inReplyTo":"552EAE0A.3040208@kdbg.org","subject":"Re: [PATCH] compat/mingw: stubs for getpgid() and tcgetpgrp()","fromName":"Luke Mewburn","fromEmail":"luke@mewburn.net","sentAt":"2015-04-17T01:25:41Z","receivedAt":"2015-04-17T01:25:41Z","isPatch":true,"sender":{"key":"luke@mewburn.net","avatar":"https://avatars.githubusercontent.com/u/629143?v=4"},"body":"On Wed, Apr 15, 2015 at 08:29:30PM +0200, Johannes Sixt wrote:\n  | Windows does not have process groups. It is, therefore, the simplest\n  | to pretend that each process is in its own process group.\n  | \n  |  [...]\n  | \n  | diff --git a/compat/mingw.h b/compat/mingw.h\n  | index 7b523cf..a552026 100644\n  | @@ -118,6 +116,12 @@ static inline int sigaddset(sigset_t *set, int signum)\n  |  #define SIG_UNBLOCK 0\n  |  static inline int sigprocmask(int how, const sigset_t *set, sigset_t *oldset)\n  |  { return 0; }\n  | +static inline pid_t getppid(void)\n  | +{ return 1; }\n  | +static inline pid_t getpgid(pid_t pid)\n  | +{ return pid == 0 ? getpid() : pid; }\n  | +static inline pid_t tcgetpgrp(int fd)\n  | +{ return getpid(); }\n\n\nThis appears to be similar to the approach that tcsh uses too;\nreturn the current process ID for the process group ID.\nSee https://github.com/tcsh-org/tcsh/blob/master/win32/ntport.h\nfor tcsh's implementation of getpgrp() (a variation of getpgid())\nand tcgetpgrp().\n\n\nregards,\nLuke.\n"},{"id":"259909","messageId":"cc79154d-4e18-41a4-938f-ca6e30b2e6d9@googlegroups.com","threadId":"39060","inReplyTo":"fac9a525a2fb2b09d176243659cbf3a7@www.dscho.org","subject":"Re: [PATCH] compat/mingw: stubs for getpgid() and tcgetpgrp()","fromName":"rupert thurner","fromEmail":"rupert.thurner@gmail.com","sentAt":"2015-04-23T19:25:49Z","receivedAt":"2015-04-23T19:25:49Z","isPatch":true,"sender":{"key":"rupert.thurner@gmail.com","avatar":null},"body":"On Thursday, April 16, 2015 at 2:45:11 PM UTC+2, Johannes Schindelin wrote:\n>\n> Hi kusma, \n>\n> On 2015-04-15 21:43, Erik Faye-Lund wrote: \n> > On Wed, Apr 15, 2015 at 8:29 PM, Johannes Sixt <j...@kdbg.org \n> <javascript:>> wrote: \n> >> Windows does not have process groups. It is, therefore, the simplest \n> >> to pretend that each process is in its own process group. \n> > \n> > Windows does have some concept of process groups, but probably not \n> > quite what you want: \n> > \n> > \n> https://msdn.microsoft.com/en-us/library/windows/desktop/ms682083%28v=vs.85%29.aspx \n>\n> Yes, and we actually need that in Git for Windows anyway because shooting \n> down a process does not kill its child processes: \n>\n>\n> https://github.com/git-for-windows/msys2-runtime/commit/15f209511985092588b171703e5046eba937b47b#diff-8753cda163376cee6c80aab11eb8701fR402 \n>\n> However, using this code for `getppid()` would be serious overkill (not to \n> mention an unbearable performance hit because you have to enumerate *all* \n> processes to get that information). \n>\n>\nis the windows \"JobObject\" similar to processgroup? at least killing the \nparent process in a jobobject will kill all childs as well:\nhttps://msdn.microsoft.com/en-us/library/windows/desktop/ms681949%28v=vs.85%29.aspx \n\nbest,\nrupert\n \n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"Git for Windows\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/d/optout.\n"},{"id":"259928","messageId":"97173c38-4f76-49cd-8365-e97fde34ed35@googlegroups.com","threadId":"39060","inReplyTo":"cc79154d-4e18-41a4-938f-ca6e30b2e6d9@googlegroups.com","subject":"Re: [PATCH] compat/mingw: stubs for getpgid() and tcgetpgrp()","fromName":"rupert thurner","fromEmail":"rupert.thurner@gmail.com","sentAt":"2015-04-24T03:03:26Z","receivedAt":"2015-04-24T03:03:26Z","isPatch":true,"sender":{"key":"rupert.thurner@gmail.com","avatar":null},"body":"On Thursday, April 23, 2015 at 9:25:49 PM UTC+2, rupert thurner wrote:\n>\n> On Thursday, April 16, 2015 at 2:45:11 PM UTC+2, Johannes Schindelin wrote:\n>>\n>> Hi kusma, \n>>\n>> On 2015-04-15 21:43, Erik Faye-Lund wrote: \n>> > On Wed, Apr 15, 2015 at 8:29 PM, Johannes Sixt <j...@kdbg.org> wrote: \n>> >> Windows does not have process groups. It is, therefore, the simplest \n>> >> to pretend that each process is in its own process group. \n>> > \n>> > Windows does have some concept of process groups, but probably not \n>> > quite what you want: \n>> > \n>> > \n>> https://msdn.microsoft.com/en-us/library/windows/desktop/ms682083%28v=vs.85%29.aspx \n>>\n>> Yes, and we actually need that in Git for Windows anyway because shooting \n>> down a process does not kill its child processes: \n>>\n>>\n>> https://github.com/git-for-windows/msys2-runtime/commit/15f209511985092588b171703e5046eba937b47b#diff-8753cda163376cee6c80aab11eb8701fR402 \n>>\n>> However, using this code for `getppid()` would be serious overkill (not \n>> to mention an unbearable performance hit because you have to enumerate \n>> *all* processes to get that information). \n>>\n>>\n> is the windows \"JobObject\" similar to processgroup? at least killing the \n> parent process in a jobobject will kill all childs as well:\n>\n> https://msdn.microsoft.com/en-us/library/windows/desktop/ms681949%28v=vs.85%29.aspx\n>  \n>\n\nhere some discussion of windows console process group, \nCREATE_NEW_PROCESS_GROUP, GenerateConsoleCtrlEvent, and job object from the \nlast century:\nhttps://www.microsoft.com/msj/0698/win320698.aspx\n\nrupert\n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"Git for Windows\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/d/optout.\n"},{"id":"259930","messageId":"f96d3bd305941f06f634733ede93a482@www.dscho.org","threadId":"39060","inReplyTo":"cc79154d-4e18-41a4-938f-ca6e30b2e6d9@googlegroups.com","subject":"Re: [msysGit] [PATCH] compat/mingw: stubs for getpgid() and tcgetpgrp()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-04-24T06:25:12Z","receivedAt":"2015-04-24T06:25:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Rupert,\n\nOn 2015-04-23 21:25, rupert thurner wrote:\n> On Thursday, April 16, 2015 at 2:45:11 PM UTC+2, Johannes Schindelin wrote:\n>>\n>> However, using this code for `getppid()` would be serious overkill (not to\n>> mention an unbearable performance hit because you have to enumerate *all*\n>> processes to get that information).\n>>\n>>\n> is the windows \"JobObject\" similar to processgroup? at least killing the \n> parent process in a jobobject will kill all childs as well:\n> https://msdn.microsoft.com/en-us/library/windows/desktop/ms681949%28v=vs.85%29.aspx\n\nReading this page carefully reveals that you have to construct JobObjects explicitly. So you cannot get from a process ID to a JobObject; there is probably none. Or there is one. Or many.\n\nCiao,\nJohannes\n"}]}