Re: [PATCH 1/2] daemon: add tests

[Jeff King <peff@peff.net>]

On Thu, Jan 05, 2012 at 05:06:15PM +0100, Clemens Buchacher wrote:

> On Wed, Jan 04, 2012 at 09:55:59PM -0500, Jeff King wrote:
> > 
> > It so happens that I have just the patch you need. I've been meaning to
> > go over it again and submit it:
> > 
> >   run-command: optionally kill children on exit
> >   https://github.com/peff/git/commit/5523d7ebf2a0386c9c61d7bfbc21375041df4989
> 
> Thanks, looks great. But if I add this on top (to enable this for
> "git daemon"), then t0001 kills my entire X session. Not sure yet
> what's going.

Yikes. Thanks for noticing.

What happens is we have a failure case in start_command, set pid to -1,
and then fall through to the end of the function. So we end up marking
"-1" for cleanup, which attempts to kill all processes.

I never noticed it because it can only happen when fork() fails, or when
a child process signals an exec failure (which happens all the time with
aliases, but could not be triggered until your patch).

The fix is to move the recording of the PID up to a spot where we are
certain that it's a real PID. Fixup patch is below, and I'll push a new
version out to my github repo.

-Peff

diff --git a/run-command.c b/run-command.c
index aeb9c6e..614b722 100644
--- a/run-command.c
+++ b/run-command.c
@@ -353,6 +353,8 @@ fail_pipe:
 	if (cmd->pid < 0)
 		error("cannot fork() for %s: %s", cmd->argv[0],
 			strerror(failed_errno = errno));
+	else if (cmd->clean_on_exit)
+		mark_child_for_cleanup(cmd->pid);
 
 	/*
 	 * Wait for child's execvp. If the execvp succeeds (or if fork()
@@ -374,8 +376,6 @@ fail_pipe:
 	}
 	close(notify_pipe[0]);
 
-	if (cmd->clean_on_exit)
-		mark_child_for_cleanup(cmd->pid);
 }
 #else
 {