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, 09:08 UTC
Message-ID: <4700B8FC.70704@viscovery.net>
In-Reply-To: <7vtzpbrzye.fsf@gitster.siamese.dyndns.org>

Junio C Hamano schrieb:

Show 18 quoted lines

> Johannes Sixt <j.sixt@viscovery.net> writes:
>> 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.
>> ...
>> 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?
> 
> In any case, I'd rather first have one that hides fork/exec
> behind child_process first without changing the call to die() in
> git_connect() in this round.  I am still in "post feature
> release clean-up" mood ;-)

Sure: The die()s are converted in a later step.

My problem is that if I don't wrap the non-fork connections somehow in this first round, I *must* remove the error checks because there is no unique failure return value anymore.

Show 5 quoted lines

> As to error indication, it somehow does not feel right to return
> something called "child _process_" structure when we want to
> tell the caller that there is no process to wait for in the
> no-error case, although the fact that we can use .in/.out fd in
> the structure when we _do_ have child process is attractive.

Did you mean: "even if we don't have a child process"?

How about a typedef if you dislike the name?

> As an alternative, we could keep the "NULL return means there
> was no need to fork" semantics of git_connect(), and instead add
> "int *status_ret" parameter for the caller to check.

Seriously? Add an *out* parameter when we can get rid of one and have a return value, too?

-- Hannes