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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Sun, 30 Sep 2007, Johannes Sixt wrote:

> -	pid_t pid;
> +	struct child_process *chld;

Why not call it "child"?  It's only one letter more, and much less 
confusing.

> -	pid = git_connect(fd, url, exec, 0);
> -	if (pid < 0)
> -		return pid;
> +	chld = git_connect(fd, url, exec, 0);

chld never gets free()d.  (I'm was about to suggest doing that in 
finish_command(), but I'm not quite sure what this would break.)

> @@ -773,16 +773,14 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
>  			st.st_mtime = 0;
>  	}
>  
> -	pid = git_connect(fd, (char *)dest, uploadpack,
> +	chld = git_connect(fd, (char *)dest, uploadpack,
>                            args.verbose ? CONNECT_VERBOSE : 0);
> -	if (pid < 0)
> -		return NULL;
>  	if (heads && nr_heads)
>  		nr_heads = remove_duplicates(nr_heads, heads);
>  	ref = do_fetch_pack(fd, nr_heads, heads, pack_lockfile);

In general, I would not lightly do away with the "return NULL" on error, 
since it is really hard to assess what do_fetch_pack() or packet_write() 
do (or don't) when git_connect() failed.

> -	while (waitpid(pid, NULL, 0) < 0) {
> +	while (waitpid(chld->pid, NULL, 0) < 0) {

Shouldn't this be converted to finish_command() already?

Ciao,
Dscho