Johannes Sixt <j.sixt@xxxxxxxxxxxxx> writes: > Junio C Hamano schrieb: >> Johannes Sixt <johannes.sixt@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
