Re: [PATCH 05/12] check_connected: relay errors to alternate descriptor

From: Jeff King <peff@peff.net>
Date: Jul 16, 2016, 01:27 UTC
Message-ID: <20160716012755.GC30820@sigill.intra.peff.net>
In-Reply-To: <xmqqbn1yn4r8.fsf@gitster.mtv.corp.google.com>

On Fri, Jul 15, 2016 at 11:19:23AM -0700, Junio C Hamano wrote:

Show 16 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > +	/*
> > +	 * If non-zero, send error messages to this descriptor rather
> > +	 * than stderr. The descriptor is closed before check_connected
> > +	 * returns.
> > +	 */
> > +	int err_fd;
> 
> Theoretically speaking it may be possible that a caller may want to
> write to fd#0 if it closed the standard input before creating the
> output channel for multiplexing into a sideband, but I think this
> design strikes a good balance between the theoretical correctness
> and usability.  Using err_fd == -1 as "no redirect" may allow the
> caller to redirect the errors to fd#0, but that forces normal users
> to explicitly set this field to -1.

Yep, I had all of those thoughts while writing it, but decided that "0" was worth it to keep the initialization simple (though since we have CHECK_CONNECTED_INIT, the "-1" would only have to appear there).

What decided me were two things:

  - directing to fd#0 really is theoretical; you'd have to close stdin,
    and you shouldn't do that. You should redirect it from /dev/null
    (and our sanitize_stdfds() makes sure that we're not surprised by
    any callers)

  - "struct child_process" uses a similar zero-initialization

-Peff