Re: [PATCH 2/6] Automatically close stderr pipes created by run_command

From: Johannes Sixt <johannes.sixt@telecom.at>
Date: Feb 15, 2008, 19:45 UTC
Message-ID: <200802152045.37755.johannes.sixt@telecom.at>
In-Reply-To: <7vfxvui1pz.fsf@gitster.siamese.dyndns.org>

On Friday 15 February 2008 18:11, Junio C Hamano wrote:

Show 31 quoted lines

> Johannes Sixt <j.sixt@viscovery.net> writes:
> > Shawn O. Pearce schrieb:
> >> Like the out pipe and in pipe, we now automatically close the err
> >> pipe if it was requested by the caller and it hasn't been closed
> >> by the caller.  This simplifies anyone who wants to get a pipe to
> >> the stderr stream for another process.
> >
> > IMHO, this is backwards.
> >
> > The .in, .out, .err members of struct child_process serve two different
> > purposes:
> >
> > 1. Caller sets them to some fd > 0. This means:
> >    "Here is a readable (.in) or writable (.out, .err) fd; use it."
> >
> > 2. Caller sets them to -1: This means:
> >    "Create a pipe and give me the writable (.in) or readable (.out,
> >    .err) end of it back."
> >
> > Notice that in a clean implementation:
> >
> > - case 1. would imply that the fd is "given away", i.e.
> >   start_command/finish_command take ownership and close it;
> >
> > - case 2. would imply that the caller takes ownership of the returned
> >   fd and has to close it.
>
> I am puzzled.  In a clean implementation perhaps the fds belong
> to the caller in both cases.  Or belong to the callee in both
> cases.  Or perhaps sometimes belong to the caller and someimes
> to the callee as you wrote above, but that seems arbitrary.

Heh. In a clean implementation you also don't use the same thing to do two completely different things. For example, note how in one case .in contains a readable, in the other a writable fd, and how fds are used in completely different ways.

> I suspect from the caller's point of view, if we can always make
> them owned by the caller, that would be the most consistent and
> convenient.  "Here is a fd --- I lend it to you so use it but do
> not close --- I may have other uses for it later".

Well, I think we could do that, theoretically. But practically in the case where a fd > 0 is assigned to .in/.out/.err:

- case .out, .err: the caller is required to close the fd early after 
start_command() because (if this is a pipe) the child won't see EOF;

- case .in: the caller must not read from the fd anyway, else the child gets 
inconsistent input.

So, while there *is* some inconsistency, the inconsistent cases can be clearly separated into the cases fd > 0 and fd == -1.

-- Hannes