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
-
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
