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

[Johannes Sixt <j.sixt@viscovery.net>]

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