# [PATCH] Reduce the number of connects when fetching 4 messages from 2008-01-25 to 2008-01-26. Participants: Daniel Barkalow, Johannes Schindelin. Thread: https://gitlist.dev/t/11734 ## Daniel Barkalow, 2008-01-25 18:33 Subject: [PATCH] Reduce the number of connects when fetching Message-ID: URL: https://gitlist.dev/e/alpine.LNX.1.00.0801251326260.13593%40iabervon.org ``` This shares the connection between getting the remote ref list and getting objects in the first batch. (A second connection is still used to follow tags) --- This got a certain amount of approval back in October and no comments against it. There was further discussion on how to deal with the current need for a second connection, but this patch is, in any case, a necessary first step. I think it was primarily not included at the time because it depended on the transport.c code which hadn't gotten into master at the time. builtin-fetch-pack.c | 74 ++++++++++++++++++++++++++----------------------- fetch-pack.h | 2 + transport.c | 41 +++++++++++++++++++-------- 3 files changed, 70 insertions(+), 47 deletions(-) diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c index e68e015..0f63c81 100644 --- a/builtin-fetch-pack.c +++ b/builtin-fetch-pack.c @@ -7,6 +7,7 @@ #include "pack.h" #include "sideband.h" #include "fetch-pack.h" +#include "remote.h" #include "run-command.h" static int transfer_unpack_limit = -1; @@ -548,14 +549,14 @@ static int get_pack(int xd[2], char **pack_lockfile) } static struct ref *do_fetch_pack(int fd[2], + const struct ref *orig_ref, int nr_match, char **match, char **pack_lockfile) { - struct ref *ref; + struct ref *ref = copy_ref_list(orig_ref); unsigned char sha1[20]; - get_remote_heads(fd[0], &ref, 0, NULL, 0); if (is_repository_shallow() && !server_supports("shallow")) die("Server does not support shallow clients"); if (server_supports("multi_ack")) { @@ -573,10 +574,6 @@ static struct ref *do_fetch_pack(int fd[2], fprintf(stderr, "Server supports side-band\n"); use_sideband = 1; } - if (!ref) { - packet_flush(fd[1]); - die("no matching remote head"); - } if (everything_local(&ref, nr_match, match)) { packet_flush(fd[1]); goto all_done; @@ -650,7 +647,7 @@ static void fetch_pack_setup(void) int cmd_fetch_pack(int argc, const char **argv, const char *prefix) { int i, ret, nr_heads; - struct ref *ref; + struct ref *ref = NULL; char *dest = NULL, **heads; nr_heads = 0; @@ -706,9 +703,34 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) if (!dest) usage(fetch_pack_usage); - ref = fetch_pack(&args, dest, nr_heads, heads, NULL); + int fd[2]; + struct child_process *conn = git_connect(fd, (char *)dest, args.uploadpack, + args.verbose ? CONNECT_VERBOSE : 0); + if (conn) { + get_remote_heads(fd[0], &ref, 0, NULL, 0); + + ref = fetch_pack(&args, fd, conn, ref, dest, nr_heads, heads, NULL); + close(fd[0]); + close(fd[1]); + if (finish_connect(conn)) + ref = NULL; + } else { + ref = NULL; + } ret = !ref; + if (!ret && nr_heads) { + /* If the heads to pull were given, we should have + * consumed all of them by matching the remote. + * Otherwise, 'git-fetch remote no-such-ref' would + * silently succeed without issuing an error. + */ + for (i = 0; i < nr_heads; i++) + if (heads[i] && heads[i][0]) { + error("no such remote ref %s", heads[i]); + ret = 1; + } + } while (ref) { printf("%s %s\n", sha1_to_hex(ref->old_sha1), ref->name); @@ -719,16 +741,15 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) } struct ref *fetch_pack(struct fetch_pack_args *my_args, + int fd[], struct child_process *conn, + const struct ref *ref, const char *dest, int nr_heads, char **heads, char **pack_lockfile) { - int i, ret; - int fd[2]; - struct child_process *conn; - struct ref *ref; struct stat st; + struct ref *ref_cpy; fetch_pack_setup(); memcpy(&args, my_args, sizeof(args)); @@ -737,29 +758,15 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args, st.st_mtime = 0; } - conn = git_connect(fd, (char *)dest, args.uploadpack, - args.verbose ? CONNECT_VERBOSE : 0); if (heads && nr_heads) nr_heads = remove_duplicates(nr_heads, heads); - ref = do_fetch_pack(fd, nr_heads, heads, pack_lockfile); - close(fd[0]); - close(fd[1]); - ret = finish_connect(conn); - - if (!ret && nr_heads) { - /* If the heads to pull were given, we should have - * consumed all of them by matching the remote. - * Otherwise, 'git-fetch remote no-such-ref' would - * silently succeed without issuing an error. - */ - for (i = 0; i < nr_heads; i++) - if (heads[i] && heads[i][0]) { - error("no such remote ref %s", heads[i]); - ret = 1; - } + if (!ref) { + packet_flush(fd[1]); + die("no matching remote head"); } + ref_cpy = do_fetch_pack(fd, ref, nr_heads, heads, pack_lockfile); - if (!ret && args.depth > 0) { + if (args.depth > 0) { struct cache_time mtime; char *shallow = git_path("shallow"); int fd; @@ -787,8 +794,5 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args, } } - if (ret) - ref = NULL; - - return ref; + return ref_cpy; } diff --git a/fetch-pack.h b/fetch-pack.h index a7888ea..8d35ef6 100644 --- a/fetch-pack.h +++ b/fetch-pack.h @@ -16,6 +16,8 @@ struct fetch_pack_args }; struct ref *fetch_pack(struct fetch_pack_args *args, + int fd[], struct child_process *conn, + const struct ref *ref, const char *dest, int nr_heads, char **heads, diff --git a/transport.c b/transport.c index babaa21..53fb2ec 100644 --- a/transport.c +++ b/transport.c @@ -563,6 +563,8 @@ struct git_transport_data { unsigned thin : 1; unsigned keep : 1; int depth; + struct child_process *conn; + int fd[2]; const char *uploadpack; const char *receivepack; }; @@ -593,20 +595,20 @@ static int set_git_option(struct transport *connection, return 1; } +static int connect_setup(struct transport *transport) +{ + struct git_transport_data *data = transport->data; + data->conn = git_connect(data->fd, transport->url, data->uploadpack, 0); + return 0; +} + static struct ref *get_refs_via_connect(struct transport *transport) { struct git_transport_data *data = transport->data; struct ref *refs; - int fd[2]; - char *dest = xstrdup(transport->url); - struct child_process *conn = git_connect(fd, dest, data->uploadpack, 0); - get_remote_heads(fd[0], &refs, 0, NULL, 0); - packet_flush(fd[1]); - - finish_connect(conn); - - free(dest); + connect_setup(transport); + get_remote_heads(data->fd[0], &refs, 0, NULL, 0); return refs; } @@ -617,7 +619,7 @@ static int fetch_refs_via_pack(struct transport *transport, struct git_transport_data *data = transport->data; char **heads = xmalloc(nr_heads * sizeof(*heads)); char **origh = xmalloc(nr_heads * sizeof(*origh)); - struct ref *refs; + const struct ref *refs; char *dest = xstrdup(transport->url); struct fetch_pack_args args; int i; @@ -632,13 +634,27 @@ static int fetch_refs_via_pack(struct transport *transport, for (i = 0; i < nr_heads; i++) origh[i] = heads[i] = xstrdup(to_fetch[i]->name); - refs = fetch_pack(&args, dest, nr_heads, heads, &transport->pack_lockfile); + + refs = transport_get_remote_refs(transport); + if (!data->conn) { + struct ref *refs_tmp; + connect_setup(transport); + get_remote_heads(data->fd[0], &refs_tmp, 0, NULL, 0); + free_refs(refs_tmp); + } + + refs = fetch_pack(&args, data->fd, data->conn, transport->remote_refs, + dest, nr_heads, heads, &transport->pack_lockfile); + close(data->fd[0]); + close(data->fd[1]); + if (finish_connect(data->conn)) + refs = NULL; + data->conn = NULL; for (i = 0; i < nr_heads; i++) free(origh[i]); free(origh); free(heads); - free_refs(refs); free(dest); return (refs ? 0 : -1); } @@ -721,6 +737,7 @@ struct transport *transport_get(struct remote *remote, const char *url) ret->disconnect = disconnect_git; data->thin = 1; + data->conn = NULL; data->uploadpack = "git-upload-pack"; if (remote && remote->uploadpack) data->uploadpack = remote->uploadpack; -- 1.5.4.rc3.4.g16335 ``` ## Johannes Schindelin, 2008-01-26 17:36 Subject: Re: [PATCH] Reduce the number of connects when fetching Message-ID: URL: https://gitlist.dev/e/alpine.LSU.1.00.0801261734360.23907%40racer.site In-Reply-To: ``` Hi, On Fri, 25 Jan 2008, Daniel Barkalow wrote: > This shares the connection between getting the remote ref list and > getting objects in the first batch. (A second connection is still used > to follow tags) I applied this locally, and now I get a few "fatal: The remote end hung up unexpectedly" errors; when I started "git fetch" in a gdb session, it did not stop at die_builtin(), which leads me to believe that the error comes from upload-pack. Any ideas? Ciao, Dscho ``` ## Daniel Barkalow, 2008-01-26 21:25 Subject: Re: [PATCH] Reduce the number of connects when fetching Message-ID: URL: https://gitlist.dev/e/alpine.LNX.1.00.0801261618470.13593%40iabervon.org In-Reply-To: ``` On Sat, 26 Jan 2008, Johannes Schindelin wrote: > Hi, > > On Fri, 25 Jan 2008, Daniel Barkalow wrote: > > > This shares the connection between getting the remote ref list and > > getting objects in the first batch. (A second connection is still used > > to follow tags) > > I applied this locally, and now I get a few "fatal: The remote end hung up > unexpectedly" errors; when I started "git fetch" in a gdb session, it did > not stop at die_builtin(), which leads me to believe that the error comes > from upload-pack. > > Any ideas? That sounds vaguely like the symptom of one of the bug I had when I wrote it, but I think that was causing test failures and this isn't. In my testing, I've gotten it to happen, but only with ssh. What is your test case like? I haven't gotten it to fail when it had anything to do, so I'm thinking it might be forgetting to let the remote end know that it isn't interested. -Daniel *This .sig left intentionally blank* ``` ## Daniel Barkalow, 2008-01-26 21:37 Subject: Re: [PATCH] Reduce the number of connects when fetching Message-ID: URL: https://gitlist.dev/e/alpine.LNX.1.00.0801261635230.13593%40iabervon.org In-Reply-To: ``` On Sat, 26 Jan 2008, Johannes Schindelin wrote: > Hi, > > On Fri, 25 Jan 2008, Daniel Barkalow wrote: > > > This shares the connection between getting the remote ref list and > > getting objects in the first batch. (A second connection is still used > > to follow tags) > > I applied this locally, and now I get a few "fatal: The remote end hung up > unexpectedly" errors; when I started "git fetch" in a gdb session, it did > not stop at die_builtin(), which leads me to believe that the error comes > from upload-pack. > > Any ideas? In fact, it seems to me that quickfetch short-circuits the fetch portion of the connection, and, if it takes care of everything (i.e., there's nothing to do), the connection gets dropped while the remote is still expecting communication. Essentially harmless but wrong, and I'll clean it up. -Daniel *This .sig left intentionally blank* ```