Re: [PATCH 1/3] connect: treat generic proxy processes like ssh processes

Am 16.05.2011 08:46, schrieb Jeff King:

The git_connect function returns two ends of a pipe for
talking with a remote, plus a struct child_process
representing the other end of the pipe. If we have a direct
socket connection, then this points to a special "no_fork"
child process.

The code path for doing git-over-pipes or git-over-ssh sets
up this child process to point to the child git command or
the ssh process. When we call finish_connect eventually, we
check wait() on the command and report its return value.

The code path for git://, on the other hand, always sets it
to no_fork. In the case of a direct TCP connection, this
makes sense; we have no child process. But in the case of a
proxy command (configured by core.gitproxy), we do have a
child process, but we throw away its pid, and therefore
ignore its return code.

Instead, let's keep that information in the proxy case, and
respect its return code, which can help catch some errors

This patch looks strikingly familiar. I had written an almost identical
change more than 3 years ago and forgot about it, though the
justification I noted in the commit was more to properly shutdown the
proxy process rather than to abandon it and let it be collected by
init(8). Your justification is much better.

There's one problem with your implementation, though:

At this point, proxy->argv would point to automatic storage; but we
need argv[0] in finish_command() for error reporting. In my
implementation, I xmalloced the pointer array and leaked it. (And
that's probably the reason that I never submitted the patch.) I
wouldn't dare to make argv just static because this limits us to have
just one open connection at a time established via git_proxy_connect().
Dunno...

Below is the interdiff that turns your patch into mine, mostly for
exposition: The two hunks in git_connect() are just cosmetic
differences. But the first hunk should be squashed into your patch to
fix a potential crash in an error situation (e.g., when the proxy
dies from a signal) at the cost of a small memory leak.

diff --git a/connect.c b/connect.c
index a28e084..8668913 100644
--- a/connect.c
+++ b/connect.c

@@ -399,9 +399,10 @@ static struct child_process *git_proxy_connect(int fd[2], char *host)
 {
 	const char *port = STR(DEFAULT_GIT_PORT);
-	const char *argv[4];
+	const char **argv;
 	struct child_process *proxy;
 
 	get_host_and_port(&host, &port);
 
+	argv = xmalloc(4*sizeof(*argv));
 	argv[0] = git_proxy_command;
 	argv[1] = host;

@@ -457,5 +458,5 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
 	char *end;
 	int c;
-	struct child_process *conn;
+	struct child_process *conn = &no_fork;
 	enum protocol protocol = PROTO_LOCAL;
 	int free_path = 0;

@@ -543,8 +544,6 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
 		if (git_use_proxy(host))
 			conn = git_proxy_connect(fd, host);
-		else {
+		else
 			git_tcp_connect(fd, host, flags);
-			conn = &no_fork;
-		}
 		/*
 		 * Separate original protocol components prog and path