{"thread":{"id":"10655","subject":"[RFC PATCH] Reduce the number of connects when fetching","startedAt":"2007-11-04T21:28:38Z","lastAt":"2007-11-06T08:34:00Z","messageCount":6,"participants":["Daniel Barkalow","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"58309","messageId":"Pine.LNX.4.64.0711041614390.7357@iabervon.org","threadId":"10655","inReplyTo":null,"subject":"[RFC PATCH] Reduce the number of connects when fetching","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2007-11-04T21:28:38Z","receivedAt":"2007-11-04T21:28:38Z","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\nIt passes all of the tests, and should be fine, but I don't entirely \nunderstand the git protocol, so I'd like people who know it better to \ncheck this over. In particular, I don't know if there's a way to have the \nconnection end up in a state where objects for more refs can be requested \nafter some refs have been requested and the resulting objects read.\n\nThe idea is to keep the open connection in the data for the transport in \nbetween getting the list of refs and doing anything further. This \ntherefore moves the connection-handling aspects outside of fetch-pack() \nand handles them primarily in transport.c.\n\nThis is on top of current next.\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 862652b..7783c05 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@@ -558,14 +559,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@@ -583,10 +584,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@@ -660,7 +657,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@@ -716,9 +713,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@@ -729,16 +751,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[], pid_t pid,\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@@ -747,29 +768,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@@ -798,8 +805,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 f4577b7..175cf2c 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -567,6 +567,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@@ -597,20 +599,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@@ -621,7 +623,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@@ -636,13 +638,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 0;\n }\n@@ -723,6 +739,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.3.4.1206.g5f96\n"},{"id":"58476","messageId":"7v1wb4kuoc.fsf@gitster.siamese.dyndns.org","threadId":"10655","inReplyTo":"Pine.LNX.4.64.0711041614390.7357@iabervon.org","subject":"Re: [RFC PATCH] Reduce the number of connects when fetching","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-06T01:51:47Z","receivedAt":"2007-11-06T01:51:47Z","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> The idea is to keep the open connection in the data for the transport in \n> between getting the list of refs and doing anything further. This \n> therefore moves the connection-handling aspects outside of fetch-pack() \n> and handles them primarily in transport.c.\n\nThe idea is very sound.  The scripted version of git-fetch used\na separate ls-remote only because peek-remote and fetch-pack\nwere separate programs.\n\n> ... In particular, I don't know if there's a way to have the \n> connection end up in a state where objects for more refs can be requested \n> after some refs have been requested and the resulting objects read.\n\nThe upload-pack protocol goes \"S: here are what I have, C: I\nwant these, C: I have these, S: ok, continue, C: I have these,\nS: ok, continue, C: I have these, S: ok, I've heard enough, C:\ndone, S: packfile is here\", so after packfile generation starts\nthere is nothing further the downloader can say.\n\nOtherwise you would be able to do the tag following using the\nsame connection, but that is unfortunately not a case.\n"},{"id":"58480","messageId":"Pine.LNX.4.64.0711052150340.7357@iabervon.org","threadId":"10655","inReplyTo":"7v1wb4kuoc.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC PATCH] Reduce the number of connects when fetching","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2007-11-06T03:04:49Z","receivedAt":"2007-11-06T03:04:49Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Mon, 5 Nov 2007, Junio C Hamano wrote:\n\n> Daniel Barkalow <barkalow@iabervon.org> writes:\n> \n> > The idea is to keep the open connection in the data for the transport in \n> > between getting the list of refs and doing anything further. This \n> > therefore moves the connection-handling aspects outside of fetch-pack() \n> > and handles them primarily in transport.c.\n> \n> The idea is very sound.  The scripted version of git-fetch used\n> a separate ls-remote only because peek-remote and fetch-pack\n> were separate programs.\n\nI figured that had to be the case, due to the way the protocol acts at the \nbeginning.\n\n> > ... In particular, I don't know if there's a way to have the \n> > connection end up in a state where objects for more refs can be requested \n> > after some refs have been requested and the resulting objects read.\n> \n> The upload-pack protocol goes \"S: here are what I have, C: I\n> want these, C: I have these, S: ok, continue, C: I have these,\n> S: ok, continue, C: I have these, S: ok, I've heard enough, C:\n> done, S: packfile is here\", so after packfile generation starts\n> there is nothing further the downloader can say.\n> \n> Otherwise you would be able to do the tag following using the\n> same connection, but that is unfortunately not a case.\n\nIt would be nice if this could continue: \"C: I also want these, S: ok, \nheard enough, C: done, S: another packfile is here\"; we should be able to \nidentify the end of the packfile on both ends to resume doing other \nthings.\n\nOr, maybe, \"C: I also want these single objects, S: here's a thin pack of \nthem\", since it's exclusively tags pointing to objects we have just \ngotten.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"58489","messageId":"7vir4gj78b.fsf@gitster.siamese.dyndns.org","threadId":"10655","inReplyTo":"Pine.LNX.4.64.0711052150340.7357@iabervon.org","subject":"Re: [RFC PATCH] Reduce the number of connects when fetching","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-06T05:03:32Z","receivedAt":"2007-11-06T05:03:32Z","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> On Mon, 5 Nov 2007, Junio C Hamano wrote:\n>\n>> The upload-pack protocol goes \"S: here are what I have, C: I\n>> want these, C: I have these, S: ok, continue, C: I have these,\n>> S: ok, continue, C: I have these, S: ok, I've heard enough, C:\n>> done, S: packfile is here\", so after packfile generation starts\n>> there is nothing further the downloader can say.\n>> \n>> Otherwise you would be able to do the tag following using the\n>> same connection, but that is unfortunately not a case.\n>\n> It would be nice if this could continue...\n\nYou would need a protocol extension for this, as the protocol\ndefines all the remainder after the have-ack exchange to be\nintended for unpack-objects, not just the data for a single pack\nthat immediately follows the exchange.  Mysteriously,\nunpack-objects even has code to write out the remainder after\nthe pack data.\n\nThe protocol extension would probably need to depend on the\nexistence of sideband.  Make the sending side signal the end of\nthe pack data over the sideband after sending a pack, and then\nboth sides can go back to the \"C: I want these too, S: Ok, here\nit is\" exchange.  You may optionally want to do another have-ack\nexchange here, but for the purpose of tag following I suspect\nthere is no need for that.\n"},{"id":"58503","messageId":"47302218.3060409@viscovery.net","threadId":"10655","inReplyTo":"7v1wb4kuoc.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC PATCH] Reduce the number of connects when fetching","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2007-11-06T08:13:12Z","receivedAt":"2007-11-06T08:13:12Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> Daniel Barkalow <barkalow@iabervon.org> writes:\n>> ... In particular, I don't know if there's a way to have the \n>> connection end up in a state where objects for more refs can be requested \n>> after some refs have been requested and the resulting objects read.\n> \n> The upload-pack protocol goes \"S: here are what I have, C: I\n> want these, C: I have these, S: ok, continue, C: I have these,\n> S: ok, continue, C: I have these, S: ok, I've heard enough, C:\n> done, S: packfile is here\", so after packfile generation starts\n> there is nothing further the downloader can say.\n> \n> Otherwise you would be able to do the tag following using the\n> same connection, but that is unfortunately not a case.\n\nHow about:\n\nS: here are what I have\nC: I want these\nC: want tags   <-- new\nC: I have these\nS: ok, continue\nC: I have these\nS: ok, continue\nC: I have these\nS: ok, these are the tags  <-- new\nS: I've heard enough\nC: done\nS: packfile is here\n\nThe tags that the server provides are those (and only those[*]) that \nreference objects in the packfile that it's going to send.\n\n[*] This way the client doesn't have to figure out which tags it wants; as a \nside-effect it won't accidentally fetch tags for objects that it happens to \nhave in the repository, but aren't reachable from any ref (like what used to \nhappen).\n\n-- Hannes\n"},{"id":"58506","messageId":"7v7ikvkc1z.fsf@gitster.siamese.dyndns.org","threadId":"10655","inReplyTo":"47302218.3060409@viscovery.net","subject":"Re: [RFC PATCH] Reduce the number of connects when fetching","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-06T08:34:00Z","receivedAt":"2007-11-06T08:34:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> The tags that the server provides are those (and only those[*]) that\n> reference objects in the packfile that it's going to send.\n>\n> [*] This way the client doesn't have to figure out which tags it\n> wants; ...\n\nYes, but that shifts the burden to the sending side which is\nalways bad.  We want to make the client work as much as possible\nwhen it is practical.\n"}]}