[PATCH] status: read submodule process output before calling wait()

[Jeff King <peff@peff.net>]

On Sat, Mar 21, 2015 at 10:56:54PM -0700, Wincent Colaiuta wrote:

> I've never seen this hang before despite frequent use of submodules.
> Oddly, I was able to work around the hang by moving the submodule in
> two hops (one from Ansible v1.6.10 to v1.7.0, then from v1.7.0 to
> v1.8.4). I am not sure if this is specific to the Ansible repo, or
> whether the length of the summary is crossing some threshold that
> triggers the bug to manifest. If I run the forked commands manually
> from an interactive shell, they complete just fine.

It's the length of the summary. The fix is below.

-- >8 --
The status code tries to read the output of "git submodule
summary" over a pipe by waiting for the program to finish
and then reading its output, like this:

  run_command(&sm_summary);
  len = strbuf_read(&cmd_stdout, sm_summary.out, 1024);

Besides being a violation of the run-command API (which
makes no promises about the state of the struct after
run_command returns), this can easily lead to deadlock. The
"submodule status" process may fill up the pipe buffer and
block on write(). Meanwhile, the reading side in the parent
process is blocked in wait(), waiting for the child to
finish.

Instead, we should start the process, read everything it
produces, and only then call wait() to finish it off.

Signed-off-by: Jeff King <peff@peff.net>
---
I notice that we also don't detect when the sub-command fails. I don't
know what we would do in that case (abort the status? print a message?)
and it's orthogonal to this issue, so I left it for somebody more
clueful in the area to think about.

 wt-status.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index 7036fa2..96f0033 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -748,9 +748,9 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt
 	fflush(s->fp);
 	sm_summary.out = -1;
 
-	run_command(&sm_summary);
-
+	start_command(&sm_summary);
 	len = strbuf_read(&cmd_stdout, sm_summary.out, 1024);
+	finish_command(&sm_summary);
 
 	/* prepend header, only if there's an actual output */
 	if (len) {
-- 
2.3.3.618.ga041503