{"thread":{"id":"11872","subject":"[PATCH 1/2] Reduce the number of connects when fetching","startedAt":"2008-02-04T18:26:23Z","lastAt":"2008-02-05T19:20:21Z","messageCount":3,"participants":["Daniel Barkalow","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"67397","messageId":"alpine.LNX.1.00.0802041323450.13593@iabervon.org","threadId":"11872","inReplyTo":null,"subject":"[PATCH 1/2] Reduce the number of connects when fetching","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-02-04T18:26:23Z","receivedAt":"2008-02-04T18:26:23Z","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---\nThere's a bug here, fixed in the next patch, but it's just cosmetic (we \nhang up on the remote end unexpectedly if we turn out not to need \nanything), and I think reorganizing changes to not do that makes the \nchanges harder to follow.\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\n"},{"id":"67474","messageId":"7v8x20gkue.fsf@gitster.siamese.dyndns.org","threadId":"11872","inReplyTo":"alpine.LNX.1.00.0802041323450.13593@iabervon.org","subject":"Re: [PATCH 1/2] Reduce the number of connects when fetching","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-05T03:09:45Z","receivedAt":"2008-02-05T03:09:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow <barkalow@iabervon.org> writes:\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\nNo signoff.\n\n> There's a bug here, fixed in the next patch, but it's just cosmetic (we \n> hang up on the remote end unexpectedly if we turn out not to need \n> anything), and I think reorganizing changes to not do that makes the \n> changes harder to follow.\n\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\nDecl after statement.\n\nBut other than that, good job.  I did not realize it would be\ndoable this cleanly.\n\nWill squash all three and queue for 'next'.\n"},{"id":"67538","messageId":"alpine.LNX.1.00.0802051417140.13593@iabervon.org","threadId":"11872","inReplyTo":"7v8x20gkue.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] Reduce the number of connects when fetching","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-02-05T19:20:21Z","receivedAt":"2008-02-05T19:20:21Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Mon, 4 Feb 2008, Junio C Hamano wrote:\n\n> Daniel Barkalow <barkalow@iabervon.org> writes:\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> \n> No signoff.\n\nI think I must have switched from signing off my emails to signing off my \ncommits some time since last october.\n\nSigned-off-by: Daniel Barkalow <barkalow@iabervon.org>\n\n> > There's a bug here, fixed in the next patch, but it's just cosmetic (we \n> > hang up on the remote end unexpectedly if we turn out not to need \n> > anything), and I think reorganizing changes to not do that makes the \n> > changes harder to follow.\n> \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> \n> Decl after statement.\n> \n> But other than that, good job.  I did not realize it would be\n> doable this cleanly.\n\nI designed the transport structure to permit it, and the protocol was \noriginally designed to work that way, so it should be easy... the hard \npart is getting rid of a second connection for fetching new tags, but \nthat's a relatively unusual situation anyway.\n\n> Will squash all three and queue for 'next'.\n\nGreat. I assume you'll fix my C99-ism, as well?\n\n\t-Daniel\n*This .sig left intentionally blank*\n"}]}