From: Johannes Sixt <johannes.sixt@telecom.at>
Date: Sun, 17 Feb 2008 09:29:17 GMT
Subject: Re: [PATCH] start_command(), if .in/.out > 0, closes file descriptors, not the callers
Message-ID: <200802171029.17850.johannes.sixt@telecom.at>
In-Reply-To: <7vmyq07bqe.fsf@gitster.siamese.dyndns.org>

On Saturday 16 February 2008 23:55, Junio C Hamano wrote:
> Johannes Sixt <johannes.sixt@telecom.at> writes:
> >  	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.

> > +	 * - 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