From: Johannes Schindelin Date: Sun, 30 Sep 2007 21:04:08 GMT Subject: Re: [PATCH 1/5] Change git_connect() to return a struct child_process instead of a pid_t. Message-ID: In-Reply-To: <1191183001-5368-2-git-send-email-johannes.sixt@telecom.at> 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