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

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.

The current implementation of start_command/finish_command as well as its
callers don't follow these rules (because they are not documented
anywhere). As a nasty side-effect we have double-closes in many places
because some callers close fds even though they are not supposed to do it.
(That's the reason why the close() calls in finish_command cannot check
for errors!)

I've a patch cooking that cleans up this mess. IMHO, your patch makes
things messier.

-- Hannes