From 5cbf8246d2e68470648d123e356665fca9ffca73 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 16 May 2011 02:46:07 -0400 Subject: connect: treat generic proxy processes like ssh processes 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 (though depending on your proxy command, it will be errors reported by the proxy command itself, and not propagated from git commands. Still, it is probably better to propagate such errors than to ignore them). It also means that the child_process field can reliably be used to determine whether the returned descriptors are actually a full-duplex socket, which means we should be using shutdown() instead of a simple close. Signed-off-by: Jeff King Helped-by: Johannes Sixt Signed-off-by: Junio C Hamano diff --git a/connect.c b/connect.c index db965c9..5884f6d 100644 --- a/connect.c +++ b/connect.c @@ -403,12 +403,12 @@ static int git_use_proxy(const char *host) return (git_proxy_command && *git_proxy_command); } -static void git_proxy_connect(int fd[2], char *host) +static struct child_process *git_proxy_connect(int fd[2], char *host) { const char *port = STR(DEFAULT_GIT_PORT); char *colon, *end; - const char *argv[4]; - struct child_process proxy; + const char **argv; + struct child_process *proxy; if (host[0] == '[') { end = strchr(host + 1, ']'); @@ -427,18 +427,20 @@ static void git_proxy_connect(int fd[2], char *host) port = colon + 1; } + argv = xmalloc(sizeof(*argv) * 4); argv[0] = git_proxy_command; argv[1] = host; argv[2] = port; argv[3] = NULL; - memset(&proxy, 0, sizeof(proxy)); - proxy.argv = argv; - proxy.in = -1; - proxy.out = -1; - if (start_command(&proxy)) + proxy = xcalloc(1, sizeof(*proxy)); + proxy->argv = argv; + proxy->in = -1; + proxy->out = -1; + if (start_command(proxy)) die("cannot start proxy %s", argv[0]); - fd[0] = proxy.out; /* read from proxy stdout */ - fd[1] = proxy.in; /* write to proxy stdin */ + fd[0] = proxy->out; /* read from proxy stdout */ + fd[1] = proxy->in; /* write to proxy stdin */ + return proxy; } #define MAX_CMD_LEN 1024 @@ -479,7 +481,7 @@ struct child_process *git_connect(int fd[2], const char *url_orig, char *host, *path; char *end; int c; - struct child_process *conn; + struct child_process *conn = &no_fork; enum protocol protocol = PROTO_LOCAL; int free_path = 0; char *port = NULL; @@ -553,7 +555,7 @@ struct child_process *git_connect(int fd[2], const char *url_orig, */ char *target_host = xstrdup(host); if (git_use_proxy(host)) - git_proxy_connect(fd, host); + conn = git_proxy_connect(fd, host); else git_tcp_connect(fd, host, flags); /* @@ -571,7 +573,7 @@ struct child_process *git_connect(int fd[2], const char *url_orig, free(url); if (free_path) free(path); - return &no_fork; + return conn; } conn = xcalloc(1, sizeof(*conn)); -- cgit v0.10.2-6-g49f6 From 7ffe853b106680720ddec999e1daf5c186997a1f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 16 May 2011 02:52:11 -0400 Subject: connect: let callers know if connection is a socket They might care because they want to do a half-duplex close. With pipes, that means simply closing the output descriptor; with a socket, you must actually call shutdown. Instead of exposing the magic no_fork child_process struct, let's encapsulate the test in a function. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/cache.h b/cache.h index bf468e5..724aad4 100644 --- a/cache.h +++ b/cache.h @@ -865,6 +865,7 @@ extern struct ref *find_ref_by_name(const struct ref *list, const char *name); #define CONNECT_VERBOSE (1u << 0) extern struct child_process *git_connect(int fd[2], const char *url, const char *prog, int flags); extern int finish_connect(struct child_process *conn); +extern int git_connection_is_socket(struct child_process *conn); extern int path_match(const char *path, int nr, char **match); struct extra_have_objects { int nr, alloc; diff --git a/connect.c b/connect.c index 5884f6d..9b31af0 100644 --- a/connect.c +++ b/connect.c @@ -633,10 +633,15 @@ struct child_process *git_connect(int fd[2], const char *url_orig, return conn; } +int git_connection_is_socket(struct child_process *conn) +{ + return conn == &no_fork; +} + int finish_connect(struct child_process *conn) { int code; - if (!conn || conn == &no_fork) + if (!conn || git_connection_is_socket(conn)) return 0; code = finish_command(conn); -- cgit v0.10.2-6-g49f6 From a1a3fd1f40a67165b849a2a1210ff3a7a2cbcdd6 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 16 May 2011 02:52:57 -0400 Subject: send-pack: avoid deadlock on git:// push with failed pack-objects Commit 09c9957c fixes a deadlock in which pack-objects fails, the remote end is still waiting for pack data, and we are still waiting for the remote end to say something (see that commit for a much more in-depth explanation). We solved the problem there by making sure the output pipe is closed on error; thus the remote sees EOF, and proceeds to complain and close its end of the connection. However, in the special case of push over git://, we don't have a pipe, but rather a full-duplex socket, with another dup()-ed descriptor in place of the second half of the pipe. In this case, closing the second descriptor signals nothing to the remote end, and we still deadlock. This patch calls shutdown() explicitly to signal EOF to the other side. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/builtin-send-pack.c b/builtin-send-pack.c index dfd3d11..eef19cb 100644 --- a/builtin-send-pack.c +++ b/builtin-send-pack.c @@ -520,6 +520,8 @@ int send_pack(struct send_pack_args *args, ref->status = REF_STATUS_NONE; if (args->stateless_rpc) close(out); + if (git_connection_is_socket(conn)) + shutdown(fd[0], SHUT_WR); if (use_sideband) finish_async(&demux); return -1; -- cgit v0.10.2-6-g49f6 From c7730e6f5f5757ac278319f33b94c32458e21f12 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 May 2011 04:57:44 -0400 Subject: test core.gitproxy configuration This is just a basic sanity test to see whether core.gitproxy works at all. Until now, we were not testing anywhere. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/t/t5532-fetch-proxy.sh b/t/t5532-fetch-proxy.sh new file mode 100755 index 0000000..62f2460 --- /dev/null +++ b/t/t5532-fetch-proxy.sh @@ -0,0 +1,43 @@ +#!/bin/sh + +test_description='fetching via git:// using core.gitproxy' +. ./test-lib.sh + +test_expect_success 'setup remote repo' ' + git init remote && + (cd remote && + echo content >file && + git add file && + git commit -m one + ) +' + +cat >proxy <<'EOF' +#!/bin/sh +echo >&2 "proxying for $*" +cmd=`perl -e ' + read(STDIN, $buf, 4); + my $n = hex($buf) - 4; + read(STDIN, $buf, $n); + my ($cmd, $other) = split /\0/, $buf; + # drop absolute-path on repo name + $cmd =~ s{ /}{ }; + print $cmd; +'` +echo >&2 "Running '$cmd'" +exec $cmd +EOF +chmod +x proxy +test_expect_success 'setup local repo' ' + git remote add fake git://example.com/remote && + git config core.gitproxy ./proxy +' + +test_expect_success 'fetch through proxy works' ' + git fetch fake && + echo one >expect && + git log -1 --format=%s FETCH_HEAD >actual && + test_cmp expect actual +' + +test_done -- cgit v0.10.2-6-g49f6