Johannes Sixt <j.sixt@viscovery.net> writes:

quoted text
> Junio C Hamano schrieb:
>> 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.
> ...
> 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 ;-)

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.

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.
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html