Re: [PATCH 1/5] Change git_connect() to return a struct child_process instead of a pid_t.

From: Johannes Sixt <j.sixt@viscovery.net>
Date: Oct 1, 2007, 07:23 UTC
Message-ID: <4700A067.3010004@viscovery.net>
In-Reply-To: <7vbqbjg9zz.fsf@gitster.siamese.dyndns.org>

Junio C Hamano schrieb:

Show 11 quoted lines

> Johannes Sixt <johannes.sixt@telecom.at> writes:
> 
>> This prepares the API of git_connect() and finish_connect() to operate on
>> a struct child_process. Currently, we just use that object as a placeholder
>> for the pid that we used to return. A follow-up patch will change the
>> implementation of git_connect() and finish_connect() to make full use
>> of the object.
> 
> Good description, except removal of checks for negative return
> of the calling sites raised my eyebrow and had me spend a few
> more minutes to review than necessary (see below).

I've thought about this issue a bit more.

Letting git_connect() die on error unconditionally is poison for any libification efforts. So here's a plan:

1. Let git_connect() return a struct child_process even for the
    non-forking case. This way a return value of NULL can uniquely
    identify a failure.

2. Keep the error checks in the callers (adjust to test for NULL).

3. Change the die() calls to return failure.

4. Note that the int fd[2] parameter to git_connect() is really an
    output: Remove it and use .in and .out of the returned struct
    child_process instead.

And maybe:

5. Reuse somehow the struct child_process that git_proxy_connect()
    already fills in.

Since my patch doesn't do (1), it can't do (2), i.e. keep the error checks - they must be removed because no unique failure value exists. So I could complete (1) in a new version of this patch, in order to also do (2). What is your preference?

-- Hannes

PS: I've postponed the completion of this plan - in favor of the MinGW port 
integration - because it only helps libification.