{"thread":{"id":"11734","subject":"[PATCH] Reduce the number of connects when fetching","startedAt":"2008-01-25T18:33:56Z","lastAt":"2008-01-26T21:37:34Z","messageCount":4,"participants":["Daniel Barkalow","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"66582","messageId":"alpine.LNX.1.00.0801251326260.13593@iabervon.org","threadId":"11734","inReplyTo":null,"subject":"[PATCH] Reduce the number of connects when fetching","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-01-25T18:33:56Z","receivedAt":"2008-01-25T18:33:56Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"This shares the connection between getting the remote ref list and\ngetting objects in the first batch. (A second connection is still used\nto follow tags)\n---\nThis got a certain amount of approval back in October and no comments \nagainst it. There was further discussion on how to deal with the current \nneed for a second connection, but this patch is, in any case, a necessary \nfirst step. I think it was primarily not included at the time because it \ndepended on the transport.c code which hadn't gotten into master at the \ntime.\n\n builtin-fetch-pack.c |   74 ++++++++++++++++++++++++++-----------------------\n fetch-pack.h         |    2 +\n transport.c          |   41 +++++++++++++++++++--------\n 3 files changed, 70 insertions(+), 47 deletions(-)\n\ndiff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c\nindex e68e015..0f63c81 100644\n--- a/builtin-fetch-pack.c\n+++ b/builtin-fetch-pack.c\n@@ -7,6 +7,7 @@\n #include \"pack.h\"\n #include \"sideband.h\"\n #include \"fetch-pack.h\"\n+#include \"remote.h\"\n #include \"run-command.h\"\n \n static int transfer_unpack_limit = -1;\n@@ -548,14 +549,14 @@ static int get_pack(int xd[2], char **pack_lockfile)\n }\n \n static struct ref *do_fetch_pack(int fd[2],\n+\t\tconst struct ref *orig_ref,\n \t\tint nr_match,\n \t\tchar **match,\n \t\tchar **pack_lockfile)\n {\n-\tstruct ref *ref;\n+\tstruct ref *ref = copy_ref_list(orig_ref);\n \tunsigned char sha1[20];\n \n-\tget_remote_heads(fd[0], &ref, 0, NULL, 0);\n \tif (is_repository_shallow() && !server_supports(\"shallow\"))\n \t\tdie(\"Server does not support shallow clients\");\n \tif (server_supports(\"multi_ack\")) {\n@@ -573,10 +574,6 @@ static struct ref *do_fetch_pack(int fd[2],\n \t\t\tfprintf(stderr, \"Server supports side-band\\n\");\n \t\tuse_sideband = 1;\n \t}\n-\tif (!ref) {\n-\t\tpacket_flush(fd[1]);\n-\t\tdie(\"no matching remote head\");\n-\t}\n \tif (everything_local(&ref, nr_match, match)) {\n \t\tpacket_flush(fd[1]);\n \t\tgoto all_done;\n@@ -650,7 +647,7 @@ static void fetch_pack_setup(void)\n int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n {\n \tint i, ret, nr_heads;\n-\tstruct ref *ref;\n+\tstruct ref *ref = NULL;\n \tchar *dest = NULL, **heads;\n \n \tnr_heads = 0;\n@@ -706,9 +703,34 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \tif (!dest)\n \t\tusage(fetch_pack_usage);\n \n-\tref = fetch_pack(&args, dest, nr_heads, heads, NULL);\n+\tint fd[2];\n+\tstruct child_process *conn = git_connect(fd, (char *)dest, args.uploadpack,\n+                          args.verbose ? CONNECT_VERBOSE : 0);\n+\tif (conn) {\n+\t\tget_remote_heads(fd[0], &ref, 0, NULL, 0);\n+\n+\t\tref = fetch_pack(&args, fd, conn, ref, dest, nr_heads, heads, NULL);\n+\t\tclose(fd[0]);\n+\t\tclose(fd[1]);\n+\t\tif (finish_connect(conn))\n+\t\t\tref = NULL;\n+\t} else {\n+\t\tref = NULL;\n+\t}\n \tret = !ref;\n \n+\tif (!ret && nr_heads) {\n+\t\t/* If the heads to pull were given, we should have\n+\t\t * consumed all of them by matching the remote.\n+\t\t * Otherwise, 'git-fetch remote no-such-ref' would\n+\t\t * silently succeed without issuing an error.\n+\t\t */\n+\t\tfor (i = 0; i < nr_heads; i++)\n+\t\t\tif (heads[i] && heads[i][0]) {\n+\t\t\t\terror(\"no such remote ref %s\", heads[i]);\n+\t\t\t\tret = 1;\n+\t\t\t}\n+\t}\n \twhile (ref) {\n \t\tprintf(\"%s %s\\n\",\n \t\t       sha1_to_hex(ref->old_sha1), ref->name);\n@@ -719,16 +741,15 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n }\n \n struct ref *fetch_pack(struct fetch_pack_args *my_args,\n+\t\t       int fd[], struct child_process *conn,\n+\t\t       const struct ref *ref,\n \t\tconst char *dest,\n \t\tint nr_heads,\n \t\tchar **heads,\n \t\tchar **pack_lockfile)\n {\n-\tint i, ret;\n-\tint fd[2];\n-\tstruct child_process *conn;\n-\tstruct ref *ref;\n \tstruct stat st;\n+\tstruct ref *ref_cpy;\n \n \tfetch_pack_setup();\n \tmemcpy(&args, my_args, sizeof(args));\n@@ -737,29 +758,15 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,\n \t\t\tst.st_mtime = 0;\n \t}\n \n-\tconn = git_connect(fd, (char *)dest, args.uploadpack,\n-                          args.verbose ? CONNECT_VERBOSE : 0);\n \tif (heads && nr_heads)\n \t\tnr_heads = remove_duplicates(nr_heads, heads);\n-\tref = do_fetch_pack(fd, nr_heads, heads, pack_lockfile);\n-\tclose(fd[0]);\n-\tclose(fd[1]);\n-\tret = finish_connect(conn);\n-\n-\tif (!ret && nr_heads) {\n-\t\t/* If the heads to pull were given, we should have\n-\t\t * consumed all of them by matching the remote.\n-\t\t * Otherwise, 'git-fetch remote no-such-ref' would\n-\t\t * silently succeed without issuing an error.\n-\t\t */\n-\t\tfor (i = 0; i < nr_heads; i++)\n-\t\t\tif (heads[i] && heads[i][0]) {\n-\t\t\t\terror(\"no such remote ref %s\", heads[i]);\n-\t\t\t\tret = 1;\n-\t\t\t}\n+\tif (!ref) {\n+\t\tpacket_flush(fd[1]);\n+\t\tdie(\"no matching remote head\");\n \t}\n+\tref_cpy = do_fetch_pack(fd, ref, nr_heads, heads, pack_lockfile);\n \n-\tif (!ret && args.depth > 0) {\n+\tif (args.depth > 0) {\n \t\tstruct cache_time mtime;\n \t\tchar *shallow = git_path(\"shallow\");\n \t\tint fd;\n@@ -787,8 +794,5 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,\n \t\t}\n \t}\n \n-\tif (ret)\n-\t\tref = NULL;\n-\n-\treturn ref;\n+\treturn ref_cpy;\n }\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex a7888ea..8d35ef6 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -16,6 +16,8 @@ struct fetch_pack_args\n };\n \n struct ref *fetch_pack(struct fetch_pack_args *args,\n+\t\tint fd[], struct child_process *conn,\n+\t\tconst struct ref *ref,\n \t\tconst char *dest,\n \t\tint nr_heads,\n \t\tchar **heads,\ndiff --git a/transport.c b/transport.c\nindex babaa21..53fb2ec 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -563,6 +563,8 @@ struct git_transport_data {\n \tunsigned thin : 1;\n \tunsigned keep : 1;\n \tint depth;\n+\tstruct child_process *conn;\n+\tint fd[2];\n \tconst char *uploadpack;\n \tconst char *receivepack;\n };\n@@ -593,20 +595,20 @@ static int set_git_option(struct transport *connection,\n \treturn 1;\n }\n \n+static int connect_setup(struct transport *transport)\n+{\n+\tstruct git_transport_data *data = transport->data;\n+\tdata->conn = git_connect(data->fd, transport->url, data->uploadpack, 0);\n+\treturn 0;\n+}\n+\n static struct ref *get_refs_via_connect(struct transport *transport)\n {\n \tstruct git_transport_data *data = transport->data;\n \tstruct ref *refs;\n-\tint fd[2];\n-\tchar *dest = xstrdup(transport->url);\n-\tstruct child_process *conn = git_connect(fd, dest, data->uploadpack, 0);\n \n-\tget_remote_heads(fd[0], &refs, 0, NULL, 0);\n-\tpacket_flush(fd[1]);\n-\n-\tfinish_connect(conn);\n-\n-\tfree(dest);\n+\tconnect_setup(transport);\n+\tget_remote_heads(data->fd[0], &refs, 0, NULL, 0);\n \n \treturn refs;\n }\n@@ -617,7 +619,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n \tstruct git_transport_data *data = transport->data;\n \tchar **heads = xmalloc(nr_heads * sizeof(*heads));\n \tchar **origh = xmalloc(nr_heads * sizeof(*origh));\n-\tstruct ref *refs;\n+\tconst struct ref *refs;\n \tchar *dest = xstrdup(transport->url);\n \tstruct fetch_pack_args args;\n \tint i;\n@@ -632,13 +634,27 @@ static int fetch_refs_via_pack(struct transport *transport,\n \n \tfor (i = 0; i < nr_heads; i++)\n \t\torigh[i] = heads[i] = xstrdup(to_fetch[i]->name);\n-\trefs = fetch_pack(&args, dest, nr_heads, heads, &transport->pack_lockfile);\n+\n+\trefs = transport_get_remote_refs(transport);\n+\tif (!data->conn) {\n+\t\tstruct ref *refs_tmp;\n+\t\tconnect_setup(transport);\n+\t\tget_remote_heads(data->fd[0], &refs_tmp, 0, NULL, 0);\n+\t\tfree_refs(refs_tmp);\n+\t}\n+\n+\trefs = fetch_pack(&args, data->fd, data->conn, transport->remote_refs,\n+\t\t\t  dest, nr_heads, heads, &transport->pack_lockfile);\n+\tclose(data->fd[0]);\n+\tclose(data->fd[1]);\n+\tif (finish_connect(data->conn))\n+\t\trefs = NULL;\n+\tdata->conn = NULL;\n \n \tfor (i = 0; i < nr_heads; i++)\n \t\tfree(origh[i]);\n \tfree(origh);\n \tfree(heads);\n-\tfree_refs(refs);\n \tfree(dest);\n \treturn (refs ? 0 : -1);\n }\n@@ -721,6 +737,7 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \t\tret->disconnect = disconnect_git;\n \n \t\tdata->thin = 1;\n+\t\tdata->conn = NULL;\n \t\tdata->uploadpack = \"git-upload-pack\";\n \t\tif (remote && remote->uploadpack)\n \t\t\tdata->uploadpack = remote->uploadpack;\n-- \n1.5.4.rc3.4.g16335\n"},{"id":"66641","messageId":"alpine.LSU.1.00.0801261734360.23907@racer.site","threadId":"11734","inReplyTo":"alpine.LNX.1.00.0801251326260.13593@iabervon.org","subject":"Re: [PATCH] Reduce the number of connects when fetching","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-01-26T17:36:22Z","receivedAt":"2008-01-26T17:36:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 25 Jan 2008, Daniel Barkalow wrote:\n\n> This shares the connection between getting the remote ref list and \n> getting objects in the first batch. (A second connection is still used \n> to follow tags)\n\nI applied this locally, and now I get a few \"fatal: The remote end hung up \nunexpectedly\" errors; when I started \"git fetch\" in a gdb session, it did \nnot stop at die_builtin(), which leads me to believe that the error comes \nfrom upload-pack.\n\nAny ideas?\n\nCiao,\nDscho\n"},{"id":"66647","messageId":"alpine.LNX.1.00.0801261618470.13593@iabervon.org","threadId":"11734","inReplyTo":"alpine.LSU.1.00.0801261734360.23907@racer.site","subject":"Re: [PATCH] Reduce the number of connects when fetching","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-01-26T21:25:23Z","receivedAt":"2008-01-26T21:25:23Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sat, 26 Jan 2008, Johannes Schindelin wrote:\n\n> Hi,\n> \n> On Fri, 25 Jan 2008, Daniel Barkalow wrote:\n> \n> > This shares the connection between getting the remote ref list and \n> > getting objects in the first batch. (A second connection is still used \n> > to follow tags)\n> \n> I applied this locally, and now I get a few \"fatal: The remote end hung up \n> unexpectedly\" errors; when I started \"git fetch\" in a gdb session, it did \n> not stop at die_builtin(), which leads me to believe that the error comes \n> from upload-pack.\n> \n> Any ideas?\n\nThat sounds vaguely like the symptom of one of the bug I had when I wrote \nit, but I think that was causing test failures and this isn't. In my \ntesting, I've gotten it to happen, but only with ssh. What is your test \ncase like?\n\nI haven't gotten it to fail when it had anything to do, so I'm thinking it \nmight be forgetting to let the remote end know that it isn't interested.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"66648","messageId":"alpine.LNX.1.00.0801261635230.13593@iabervon.org","threadId":"11734","inReplyTo":"alpine.LSU.1.00.0801261734360.23907@racer.site","subject":"Re: [PATCH] Reduce the number of connects when fetching","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-01-26T21:37:34Z","receivedAt":"2008-01-26T21:37:34Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sat, 26 Jan 2008, Johannes Schindelin wrote:\n\n> Hi,\n> \n> On Fri, 25 Jan 2008, Daniel Barkalow wrote:\n> \n> > This shares the connection between getting the remote ref list and \n> > getting objects in the first batch. (A second connection is still used \n> > to follow tags)\n> \n> I applied this locally, and now I get a few \"fatal: The remote end hung up \n> unexpectedly\" errors; when I started \"git fetch\" in a gdb session, it did \n> not stop at die_builtin(), which leads me to believe that the error comes \n> from upload-pack.\n> \n> Any ideas?\n\nIn fact, it seems to me that quickfetch short-circuits the fetch portion \nof the connection, and, if it takes care of everything (i.e., there's \nnothing to do), the connection gets dropped while the remote is still \nexpecting communication. Essentially harmless but wrong, and I'll clean it \nup.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"}]}