Re: [PATCH 1/5] Change git_connect() to return a struct child_process instead of a pid_t.
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Sep 30, 2007, 21:04 UTC
- Message-ID
- <Pine.LNX.4.64.0709302158110.28395@racer.site>
- 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.)
Show 12 quoted lines
> @@ -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