Re: [PATCH] start_command(), if .in/.out > 0, closes file descriptors, not the callers

On Saturday 16 February 2008 23:55, Junio C Hamano wrote:

Johannes Sixt [off-list ref] writes:

quoted

 	need_in = !cmd->no_stdin && cmd->in < 0;
 	if (need_in) {
-		if (pipe(fdin) < 0)
+		if (pipe(fdin) < 0) {
+			if (cmd->out > 1)
+				close(cmd->out);

Why check for "2 or more"?

Later in the code, where we set up the redirections in the child process, we 
have this:
		...
		} else if (cmd->out > 1) {
			dup2(cmd->out, 1);
			close(cmd->out);
		}

and I thought it was good to also compare cmd->out > 1 in other situations. 
But now that I think about it, this > 1 is to be understood like this:

		if (cmd->out > 0)
			dup2(cmd->out, 1) unless cmd->out == 1;

so it's an optimization - an unnecessary one since dup2(1, 1) is supposed to 
succeed and be a noop.

quoted

+	 * - Specify > 0 to give away a FD as follows:
+	 *     .in: a readable FD, becomes child's stdin
+	 *     .out: a writable FD, becomes child's stdout/stderr
+	 *     .err > 0 not supported
+	 *   The specified FD is closed by start_command(), even in case
+	 *   of errors!

Perhaps you would need to spell out the semantic differences you
are assigning to "inherit" vs "give away".  I presume the former
is something run_command() would not touch vs the latter is
closed by run_command()?

I'll clearify it.

-- Hannes