[PATCH 0/2] Remove non-blocking fds from run-command.

[PATCH 0/2] Remove non-blocking fds from run-command.

[Stefan Beller]

So as far as I understand, all of the discussion participants (Torsten, Jeff,
Junio and me) are convinced we don't need the non-blocking feature. So remove it.

I developed it on top of d075d2604c0 (Merge branch 'rs/daemon-plug-child-leak' into sb/submodule-parallel-update)
but AFAICT it also applies to sb/submodule-parallel-fetch.

This will fix compilation in Windows without any platform specific hacks.

Thanks,
Stefan

Stefan Beller (2):
  run-command: Remove set_nonblocking
  strbuf: Correct documentation for strbuf_read_once

 run-command.c | 13 -------------
 strbuf.h      |  3 +--
 2 files changed, 1 insertion(+), 15 deletions(-)

-- 
2.6.1.247.ge8f2a41.dirty

[PATCH 1/2] run-command: Remove set_nonblocking

[Stefan Beller]

strbuf_read_once can also operate on blocking file descriptors if we are
sure they are ready. The poll (2) command however makes sure this is the
case.

Reading the manual for poll (2), there may be spurious returns indicating
readiness but that is for network sockets only. Pipes should be unaffected.
By having this patch, we rely on the correctness of poll to return
only pipes ready to read.

This fixes compilation in Windows.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 run-command.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/run-command.c b/run-command.c
index 0a3c24e..51d078c 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1006,17 +1006,6 @@ static void pp_cleanup(struct parallel_processes *pp)
 	sigchain_pop_common();
 }
 
-static void set_nonblocking(int fd)
-{
-	int flags = fcntl(fd, F_GETFL);
-	if (flags < 0)
-		warning("Could not get file status flags, "
-			"output will be degraded");
-	else if (fcntl(fd, F_SETFL, flags | O_NONBLOCK))
-		warning("Could not set file status flags, "
-			"output will be degraded");
-}
-
 /* returns
  *  0 if a new task was started.
  *  1 if no new jobs was started (get_next_task ran out of work, non critical
@@ -1052,8 +1041,6 @@ static int pp_start_one(struct parallel_processes *pp)
 		return code ? -1 : 1;
 	}
 
-	set_nonblocking(pp->children[i].process.err);
-
 	pp->nr_processes++;
 	pp->children[i].in_use = 1;
 	pp->pfd[i].fd = pp->children[i].process.err;
-- 
2.6.1.247.ge8f2a41.dirty

Re: [PATCH 1/2] run-command: Remove set_nonblocking

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> strbuf_read_once can also operate on blocking file descriptors if we are
> sure they are ready. The poll (2) command however makes sure this is the
> case.
>
> Reading the manual for poll (2), there may be spurious returns indicating
> readiness but that is for network sockets only. Pipes should be unaffected.

Given the presence of "for example" in that bug section, I wouldn't
say "only" or "should be unaffected".

> By having this patch, we rely on the correctness of poll to return
> only pipes ready to read.

We rely on two things.  One is for poll to return only pipes that are 
non-empty.  The other is for read from a non-empty pipe not to block.

>
> This fixes compilation in Windows.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

Thanks.  Let's apply these fixes on sb/submodule-parallel-fetch,
merge the result to 'next' and have people play with it.

>  run-command.c | 13 -------------
>  1 file changed, 13 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index 0a3c24e..51d078c 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1006,17 +1006,6 @@ static void pp_cleanup(struct parallel_processes *pp)
>  	sigchain_pop_common();
>  }
>  
> -static void set_nonblocking(int fd)
> -{
> -	int flags = fcntl(fd, F_GETFL);
> -	if (flags < 0)
> -		warning("Could not get file status flags, "
> -			"output will be degraded");
> -	else if (fcntl(fd, F_SETFL, flags | O_NONBLOCK))
> -		warning("Could not set file status flags, "
> -			"output will be degraded");
> -}
> -
>  /* returns
>   *  0 if a new task was started.
>   *  1 if no new jobs was started (get_next_task ran out of work, non critical
> @@ -1052,8 +1041,6 @@ static int pp_start_one(struct parallel_processes *pp)
>  		return code ? -1 : 1;
>  	}
>  
> -	set_nonblocking(pp->children[i].process.err);
> -
>  	pp->nr_processes++;
>  	pp->children[i].in_use = 1;
>  	pp->pfd[i].fd = pp->children[i].process.err;

Re: [PATCH 1/2] run-command: Remove set_nonblocking

[Stefan Beller]

On Thu, Nov 5, 2015 at 10:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> strbuf_read_once can also operate on blocking file descriptors if we are
>> sure they are ready. The poll (2) command however makes sure this is the
>> case.
>>
>> Reading the manual for poll (2), there may be spurious returns indicating
>> readiness but that is for network sockets only. Pipes should be unaffected.
>
> Given the presence of "for example" in that bug section, I wouldn't
> say "only" or "should be unaffected".

Reading the documentation we are in agreement, that we expect
no spurious returns, no?

>
>> By having this patch, we rely on the correctness of poll to return
>> only pipes ready to read.
>
> We rely on two things.  One is for poll to return only pipes that are
> non-empty.  The other is for read from a non-empty pipe not to block.

That's what I meant with 'pipe being ready'.

>
>>
>> This fixes compilation in Windows.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>
> Thanks.  Let's apply these fixes on sb/submodule-parallel-fetch,
> merge the result to 'next' and have people play with it.

Maybe the commit message was weakly crafted. Do you want me to resend?

>
>>  run-command.c | 13 -------------
>>  1 file changed, 13 deletions(-)
>>
>> diff --git a/run-command.c b/run-command.c
>> index 0a3c24e..51d078c 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -1006,17 +1006,6 @@ static void pp_cleanup(struct parallel_processes *pp)
>>       sigchain_pop_common();
>>  }
>>
>> -static void set_nonblocking(int fd)
>> -{
>> -     int flags = fcntl(fd, F_GETFL);
>> -     if (flags < 0)
>> -             warning("Could not get file status flags, "
>> -                     "output will be degraded");
>> -     else if (fcntl(fd, F_SETFL, flags | O_NONBLOCK))
>> -             warning("Could not set file status flags, "
>> -                     "output will be degraded");
>> -}
>> -
>>  /* returns
>>   *  0 if a new task was started.
>>   *  1 if no new jobs was started (get_next_task ran out of work, non critical
>> @@ -1052,8 +1041,6 @@ static int pp_start_one(struct parallel_processes *pp)
>>               return code ? -1 : 1;
>>       }
>>
>> -     set_nonblocking(pp->children[i].process.err);
>> -
>>       pp->nr_processes++;
>>       pp->children[i].in_use = 1;
>>       pp->pfd[i].fd = pp->children[i].process.err;

Re: [PATCH 1/2] run-command: Remove set_nonblocking

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Thu, Nov 5, 2015 at 10:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> strbuf_read_once can also operate on blocking file descriptors if we are
>>> sure they are ready. The poll (2) command however makes sure this is the
>>> case.
>>>
>>> Reading the manual for poll (2), there may be spurious returns indicating
>>> readiness but that is for network sockets only. Pipes should be unaffected.
>>
>> Given the presence of "for example" in that bug section, I wouldn't
>> say "only" or "should be unaffected".
>
> Reading the documentation we are in agreement, that we expect
> no spurious returns, no?

Given the presence of "for example" in that bug section, I wouldn't
say "only" or "should be unaffected".  I cannot say "we expect no
spurious returns".

>> Thanks.  Let's apply these fixes on sb/submodule-parallel-fetch,
>> merge the result to 'next' and have people play with it.
>
> Maybe the commit message was weakly crafted. Do you want me to resend?

I somehow feel that it is prudent to let this cook just above 'next'
for a few days (not just for the log message but to verify the
strategy and wait for others to come up with even better ideas), but
then I'll be offline starting next week, so I expect that merging
the final version to 'next' will be done by our interim maintainer,
which means we still have time to polish ;-)

Here is what I queued for now.

-- >8 --
From: Stefan Beller <sbeller@google.com>
Date: Thu, 5 Nov 2015 10:17:18 -0800
Subject: [PATCH] run-command: remove set_nonblocking()

strbuf_read_once can also operate on blocking file descriptors if we
are sure they are ready.  And the poll(2) we call before calling
this ensures that this is the case.

Reading the manual for poll(2), there may be spurious returns
indicating readiness but that is for network sockets only and pipes
should be unaffected.

With this change, we rely on

 - poll(2) returns only non-empty pipes; and
 - read(2) on a non-empty pipe does not block.

This should fix compilation on Windows.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 run-command.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/run-command.c b/run-command.c
index 1fbd286..07424e9 100644
--- a/run-command.c
+++ b/run-command.c
@@ -996,17 +996,6 @@ static void pp_cleanup(struct parallel_processes *pp)
 	sigchain_pop_common();
 }
 
-static void set_nonblocking(int fd)
-{
-	int flags = fcntl(fd, F_GETFL);
-	if (flags < 0)
-		warning("Could not get file status flags, "
-			"output will be degraded");
-	else if (fcntl(fd, F_SETFL, flags | O_NONBLOCK))
-		warning("Could not set file status flags, "
-			"output will be degraded");
-}
-
 /* returns
  *  0 if a new task was started.
  *  1 if no new jobs was started (get_next_task ran out of work, non critical
@@ -1042,8 +1031,6 @@ static int pp_start_one(struct parallel_processes *pp)
 		return code ? -1 : 1;
 	}
 
-	set_nonblocking(pp->children[i].process.err);
-
 	pp->nr_processes++;
 	pp->children[i].in_use = 1;
 	pp->pfd[i].fd = pp->children[i].process.err;
-- 
2.6.2-539-g1c5cd50

Re: [PATCH 1/2] run-command: Remove set_nonblocking

[Johannes Sixt]

Am 05.11.2015 um 19:17 schrieb Stefan Beller:
> strbuf_read_once can also operate on blocking file descriptors if we are
> sure they are ready. The poll (2) command however makes sure this is the
> case.
> 
> Reading the manual for poll (2), there may be spurious returns indicating
> readiness but that is for network sockets only. Pipes should be unaffected.
> By having this patch, we rely on the correctness of poll to return
> only pipes ready to read.
> 
> This fixes compilation in Windows.

It certainly does (but I haven't tested, yet). But parallel processes
will not work because we do not have a sufficiently complete waitpid
emulation, yet. (waitpid(-1, ...) is not implemented.)

However, I think that the infrastructure can be simplified even further
to a level that we do not need additional emulation on Windows.

First let me say that I find it very questionable that the callbacks
receive a struct child_process. This is an implementation detail. It is
also an implementation detail that stderr of the children is read and
buffered, and that the child's stdout is redirected to stderr. It
should not be the task of the get_next_task callback to set the members
no_stdin, stdout_to_stderr, and err of struct child_process.

If you move that initialization to pp_start_one, you will notice rather
sooner than later that the readable end of the file descriptor is never
closed!

Which makes me think: Other users of start_command/finish_command work
such that they

1. request a pipe by setting .out = -1
2. start_command
3. read from .out until EOF
4. close .out
5. wait for the process with finish_command

But the parallel_process infrastructure does not follow this pattern.
It

1. requests a pipe by setting .err = -1
2. start_command
3. read from .err
4. wait for the process with waitpid

(and forgets to close .err). EOF is not in the picture (but that is
not essential).

I suggest to change this such that we read from the children until EOF,
mark them to be at their end of life, and then wait for them using
finish_command (assuming that a process that closes stdout and stderr
will die very soon if it is not already dead).

Here is a prototype patch. Feel free to pick it up. It marks a process
whose EOF we have found by setting .err to -1. It's probably better to
extend the meaning of the in_use indicator for this purpose. This seems
to work on Linux with test-run-command with sub-processes that produce
100k output each:

./test-run-command run-command-parallel 5 sh -c "printf \"%0100000d\n\" 999"

although error handling would require some polishing according to
t0061-run-command.

diff --git a/run-command.c b/run-command.c
index 51d078c..3e42299 100644
--- a/run-command.c
+++ b/run-command.c
@@ -977,7 +977,7 @@ static struct parallel_processes *pp_init(int n,
 	for (i = 0; i < n; i++) {
 		strbuf_init(&pp->children[i].err, 0);
 		child_process_init(&pp->children[i].process);
-		pp->pfd[i].events = POLLIN;
+		pp->pfd[i].events = POLLIN|POLLHUP;
 		pp->pfd[i].fd = -1;
 	}
 	sigchain_push_common(handle_children_on_signal);
@@ -1061,11 +1061,17 @@ static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout)
 	/* Buffer output from all pipes. */
 	for (i = 0; i < pp->max_processes; i++) {
 		if (pp->children[i].in_use &&
-		    pp->pfd[i].revents & POLLIN)
-			if (strbuf_read_once(&pp->children[i].err,
-					     pp->children[i].process.err, 0) < 0)
+		    pp->pfd[i].revents & (POLLIN|POLLHUP)) {
+			int n = strbuf_read_once(&pp->children[i].err,
+					     pp->children[i].process.err, 0);
+			if (n == 0) {
+				close(pp->children[i].process.err);
+				pp->children[i].process.err = -1;
+			} else if (n < 0) {
 				if (errno != EAGAIN)
 					die_errno("read");
+			}
+		}
 	}
 }
 
@@ -1082,59 +1088,20 @@ static void pp_output(struct parallel_processes *pp)
 static int pp_collect_finished(struct parallel_processes *pp)
 {
 	int i = 0;
-	pid_t pid;
-	int wait_status, code;
+	int code;
 	int n = pp->max_processes;
 	int result = 0;
 
 	while (pp->nr_processes > 0) {
-		pid = waitpid(-1, &wait_status, WNOHANG);
-		if (pid == 0)
-			break;
-
-		if (pid < 0)
-			die_errno("wait");
-
 		for (i = 0; i < pp->max_processes; i++)
 			if (pp->children[i].in_use &&
-			    pid == pp->children[i].process.pid)
+			    pp->children[i].process.err == -1)
 				break;
+
 		if (i == pp->max_processes)
-			die("BUG: found a child process we were not aware of");
-
-		if (strbuf_read(&pp->children[i].err,
-				pp->children[i].process.err, 0) < 0)
-			die_errno("strbuf_read");
-
-		if (WIFSIGNALED(wait_status)) {
-			code = WTERMSIG(wait_status);
-			if (!pp->shutdown &&
-			    code != SIGINT && code != SIGQUIT)
-				strbuf_addf(&pp->children[i].err,
-					    "%s died of signal %d",
-					    pp->children[i].process.argv[0],
-					    code);
-			/*
-			 * This return value is chosen so that code & 0xff
-			 * mimics the exit code that a POSIX shell would report for
-			 * a program that died from this signal.
-			 */
-			code += 128;
-		} else if (WIFEXITED(wait_status)) {
-			code = WEXITSTATUS(wait_status);
-			/*
-			 * Convert special exit code when execvp failed.
-			 */
-			if (code == 127) {
-				code = -1;
-				errno = ENOENT;
-			}
-		} else {
-			strbuf_addf(&pp->children[i].err,
-				    "waitpid is confused (%s)",
-				    pp->children[i].process.argv[0]);
-			code = -1;
-		}
+			break;
+
+		code = finish_command(&pp->children[i].process);
 
 		if (pp->task_finished(code, &pp->children[i].process,
 				      &pp->children[i].err, pp->data,
@@ -1144,7 +1111,6 @@ static int pp_collect_finished(struct parallel_processes *pp)
 		pp->nr_processes--;
 		pp->children[i].in_use = 0;
 		pp->pfd[i].fd = -1;
-		child_process_clear(&pp->children[i].process);
 		child_process_init(&pp->children[i].process);
 
 		if (i != pp->output_owner) {

Re: [PATCH 1/2] run-command: Remove set_nonblocking

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 05.11.2015 um 19:17 schrieb Stefan Beller:
>> strbuf_read_once can also operate on blocking file descriptors if we are
>> sure they are ready. The poll (2) command however makes sure this is the
>> case.
>> 
>> Reading the manual for poll (2), there may be spurious returns indicating
>> readiness but that is for network sockets only. Pipes should be unaffected.
>> By having this patch, we rely on the correctness of poll to return
>> only pipes ready to read.
>> 
>> This fixes compilation in Windows.
>
> It certainly does (but I haven't tested, yet). But parallel processes
> will not work because we do not have a sufficiently complete waitpid
> emulation, yet. (waitpid(-1, ...) is not implemented.)
>
> However, I think that the infrastructure can be simplified even further
> to a level that we do not need additional emulation on Windows.

;-)

This is why I love this list (and in general not rushing any change
too early to 'next').

> Which makes me think: Other users of start_command/finish_command work
> such that they
>
> 1. request a pipe by setting .out = -1
> 2. start_command
> 3. read from .out until EOF
> 4. close .out
> 5. wait for the process with finish_command
>
> But the parallel_process infrastructure does not follow this pattern.
> It
>
> 1. requests a pipe by setting .err = -1
> 2. start_command
> 3. read from .err
> 4. wait for the process with waitpid
>
> (and forgets to close .err). EOF is not in the picture (but that is
> not essential).

Unrelated tangent.  daemon is another one that uses start_command()
but does not use finish_command().

> I suggest to change this such that we read from the children until EOF,
> mark them to be at their end of life, and then wait for them using
> finish_command (assuming that a process that closes stdout and stderr
> will die very soon if it is not already dead).

Hmm, interesting.  This does match the normal "spawn, interact and
wait" cycle for a single process much better.

Re: [PATCH 1/2] run-command: Remove set_nonblocking

[Stefan Beller]

On Thu, Nov 5, 2015 at 12:27 PM, Johannes Sixt <j6t@kdbg.org> wrote:

> diff --git a/run-command.c b/run-command.c
> index 51d078c..3e42299 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -977,7 +977,7 @@ static struct parallel_processes *pp_init(int n,
>         for (i = 0; i < n; i++) {
>                 strbuf_init(&pp->children[i].err, 0);
>                 child_process_init(&pp->children[i].process);
> -               pp->pfd[i].events = POLLIN;
> +               pp->pfd[i].events = POLLIN|POLLHUP;
>                 pp->pfd[i].fd = -1;
>         }
>         sigchain_push_common(handle_children_on_signal);
> @@ -1061,11 +1061,17 @@ static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout)
>         /* Buffer output from all pipes. */
>         for (i = 0; i < pp->max_processes; i++) {
>                 if (pp->children[i].in_use &&
> -                   pp->pfd[i].revents & POLLIN)
> -                       if (strbuf_read_once(&pp->children[i].err,
> -                                            pp->children[i].process.err, 0) < 0)
> +                   pp->pfd[i].revents & (POLLIN|POLLHUP)) {
> +                       int n = strbuf_read_once(&pp->children[i].err,
> +                                            pp->children[i].process.err, 0);
> +                       if (n == 0) {
> +                               close(pp->children[i].process.err);
> +                               pp->children[i].process.err = -1;

So you set .err to -1 to signal the process has ended here...

> -
>                 for (i = 0; i < pp->max_processes; i++)
>                         if (pp->children[i].in_use &&
> -                           pid == pp->children[i].process.pid)
> +                           pp->children[i].process.err == -1)
>                                 break;

to make a decision here if we want to finish_command on it.


> +               code = finish_command(&pp->children[i].process);

> -               child_process_clear(&pp->children[i].process);

but .err stays stays -1 here for the next iteration?
We would need to reset it to 0 again.

So .err is
  0 when the slot is not in use
 -1 when the child has finished awaiting termination
 >0 when the child is living a happy life.

Re: [PATCH 1/2] run-command: Remove set_nonblocking

[Johannes Sixt]

Am 05.11.2015 um 23:20 schrieb Stefan Beller:
> On Thu, Nov 5, 2015 at 12:27 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>
>> diff --git a/run-command.c b/run-command.c
>> index 51d078c..3e42299 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -977,7 +977,7 @@ static struct parallel_processes *pp_init(int n,
>>          for (i = 0; i < n; i++) {
>>                  strbuf_init(&pp->children[i].err, 0);
>>                  child_process_init(&pp->children[i].process);
>> -               pp->pfd[i].events = POLLIN;
>> +               pp->pfd[i].events = POLLIN|POLLHUP;
>>                  pp->pfd[i].fd = -1;
>>          }
>>          sigchain_push_common(handle_children_on_signal);
>> @@ -1061,11 +1061,17 @@ static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout)
>>          /* Buffer output from all pipes. */
>>          for (i = 0; i < pp->max_processes; i++) {
>>                  if (pp->children[i].in_use &&
>> -                   pp->pfd[i].revents & POLLIN)
>> -                       if (strbuf_read_once(&pp->children[i].err,
>> -                                            pp->children[i].process.err, 0) < 0)
>> +                   pp->pfd[i].revents & (POLLIN|POLLHUP)) {
>> +                       int n = strbuf_read_once(&pp->children[i].err,
>> +                                            pp->children[i].process.err, 0);
>> +                       if (n == 0) {
>> +                               close(pp->children[i].process.err);
>> +                               pp->children[i].process.err = -1;
>
> So you set .err to -1 to signal the process has ended here...
>
>> -
>>                  for (i = 0; i < pp->max_processes; i++)
>>                          if (pp->children[i].in_use &&
>> -                           pid == pp->children[i].process.pid)
>> +                           pp->children[i].process.err == -1)
>>                                  break;
>
> to make a decision here if we want to finish_command on it.

Correct.

>> +               code = finish_command(&pp->children[i].process);
>
>> -               child_process_clear(&pp->children[i].process);
>
> but .err stays stays -1 here for the next iteration?
> We would need to reset it to 0 again.

No. In the next round, we need -1 to request a pipe. get_next_task 
callback sets it to -1 as well (and I think it is wrong that the 
callback does it; pp_start_one should do that).

> So .err is
>    0 when the slot is not in use
>   -1 when the child has finished awaiting termination
>   >0 when the child is living a happy life.

But, as I said, .err is not the right place to mark dying processes (it 
was just the simplest way to demonstrate the concept in this patch). 
Better extend .in_use to a tri-state indicator.

-- Hannes

Re: [PATCH 1/2] run-command: Remove set_nonblocking

[Stefan Beller]

On Thu, Nov 5, 2015 at 12:27 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> However, I think that the infrastructure can be simplified even further
> to a level that we do not need additional emulation on Windows.
>
> First let me say that I find it very questionable that the callbacks
> receive a struct child_process.

ok, I try to clean that up as well. However I made this choice as we
need to set the working dir as well as the environment variables.
I guess these will go into an extra argument then


> Here is a prototype patch. Feel free to pick it up. It marks a process
> whose EOF we have found by setting .err to -1. It's probably better to
> extend the meaning of the in_use indicator for this purpose.

Thanks for the proposal, I'll take that and make in_use a tristate for now
(an enum consisting of FREE, WORKING, WAIT_CLEANUP)

Thanks,
Stefan

Re: [PATCH 1/2] run-command: Remove set_nonblocking

[Johannes Sixt]

Am 06.11.2015 um 20:00 schrieb Stefan Beller:
> On Thu, Nov 5, 2015 at 12:27 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> Here is a prototype patch. Feel free to pick it up. It marks a process
>> whose EOF we have found by setting .err to -1. It's probably better to
>> extend the meaning of the in_use indicator for this purpose.
>
> Thanks for the proposal, I'll take that and make in_use a tristate for now
> (an enum consisting of FREE, WORKING, WAIT_CLEANUP)

I'd like to report that the prototype patch works on Windows. I tested 
it lightly using test-run-command with commands producing output from 
around 100 bytes to 10MB. So, I'm confident that this is the right approach.

Thank you for keeping the ball rolling!

-- Hannes

[PATCH 2/2] strbuf: Correct documentation for strbuf_read_once

[Stefan Beller]

No need to document the O_NONBLOCK. We will read just once and return.
In case the read blocks, this works too.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 strbuf.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/strbuf.h b/strbuf.h
index ea69665..7a08da4 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -367,8 +367,7 @@ extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);
 extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);
 
 /**
- * Read from a file descriptor that is marked as O_NONBLOCK without
- * blocking.  Returns the number of new bytes appended to the sb.
+ * Returns the number of new bytes appended to the sb.
  * Negative return value signals there was an error returned from
  * underlying read(2), in which case the caller should check errno.
  * e.g. errno == EAGAIN when the read may have blocked.
-- 
2.6.1.247.ge8f2a41.dirty