{"thread":{"id":"21901","subject":"[REROLL PATCH 0/8] Remote helpers smart transport extensions","startedAt":"2009-12-08T13:16:09Z","lastAt":"2009-12-09T21:42:16Z","messageCount":23,"participants":["Ilari Liusvaara","Junio C Hamano","Bert Wesarg"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"129521","messageId":"1260278177-9029-1-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"21901","inReplyTo":null,"subject":"[REROLL PATCH 0/8] Remote helpers smart transport extensions","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-08T13:16:09Z","receivedAt":"2009-12-08T13:16:09Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"This is rerolled version of remote helpers exensions. The changes\nfrom RFC patch v3 are:\n\n- Enable stream buffering where possible.\n- Pass the connection directly, not indirected via disown method.\n- \"invoke/connect\" -> \"connect\" in commit message.\n- Don't try to send disconnect command after connecting.\n\nThis series is based on same version as appiled previous version is.\n\nIlari Liusvaara (8):\n Add remote helper debug mode\n Support mandatory capabilities\n Pass unknown protocols to external protocol handlers\n Refactor git transport options parsing\n Support taking over transports\n Support remote helpers implementing smart transports\n Support remote archive from external protocol helpers\n Remove special casing of http, https and ftp\n\n .gitignore | 4 +\n Documentation/git-remote-helpers.txt | 30 ++++-\n Makefile | 24 +++-\n builtin-archive.c | 17 ++-\n transport-helper.c | 291 ++++++++++++++++++++++++++++++----\n transport.c | 228 +++++++++++++++++++++------\n transport.h | 22 +++\n 7 files changed, 523 insertions(+), 93 deletions(-)\n"},{"id":"129526","messageId":"1260278177-9029-2-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"21901","inReplyTo":"1260278177-9029-1-git-send-email-ilari.liusvaara@elisanet.fi","subject":"[REROLL PATCH 1/8] Add remote helper debug mode","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-08T13:16:10Z","receivedAt":"2009-12-08T13:16:10Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"Remote helpers deadlock easily, so support debug mode which shows the\ninteraction steps.\n\nSigned-off-by: Ilari Liusvaara \nSigned-off-by: Junio C Hamano \nSigned-off-by: Ilari Liusvaara \n---\n transport-helper.c | 94 ++++++++++++++++++++++++++++++++++++++--------------\n 1 files changed, 69 insertions(+), 25 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 11f3d7e..a721dc2 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -8,6 +8,8 @@\n #include \"quote.h\"\n #include \"remote.h\"\n \n+static int debug;\n+\n struct helper_data\n {\n \tconst char *name;\n@@ -22,6 +24,45 @@ struct helper_data\n \tint refspec_nr;\n };\n \n+static void sendline(struct helper_data *helper, struct strbuf *buffer)\n+{\n+\tif (debug)\n+\t\tfprintf(stderr, \"Debug: Remote helper: -> %s\", buffer->buf);\n+\tif (write_in_full(helper->helper->in, buffer->buf, buffer->len)\n+\t\t!= buffer->len)\n+\t\tdie_errno(\"Full write to remote helper failed\");\n+}\n+\n+static int recvline(struct helper_data *helper, struct strbuf *buffer)\n+{\n+\tstrbuf_reset(buffer);\n+\tif (debug)\n+\t\tfprintf(stderr, \"Debug: Remote helper: Waiting...\\n\");\n+\tif (strbuf_getline(buffer, helper->out, '\\n') == EOF) {\n+\t\tif (debug)\n+\t\t\tfprintf(stderr, \"Debug: Remote helper quit.\\n\");\n+\t\texit(128);\n+\t}\n+\n+\tif (debug)\n+\t\tfprintf(stderr, \"Debug: Remote helper: <- %s\\n\", buffer->buf);\n+\treturn 0;\n+}\n+\n+static void xchgline(struct helper_data *helper, struct strbuf *buffer)\n+{\n+\tsendline(helper, buffer);\n+\trecvline(helper, buffer);\n+}\n+\n+static void write_constant(int fd, const char *str)\n+{\n+\tif (debug)\n+\t\tfprintf(stderr, \"Debug: Remote helper: -> %s\", str);\n+\tif (write_in_full(fd, str, strlen(str)) != strlen(str))\n+\t\tdie_errno(\"Full write to remote helper failed\");\n+}\n+\n static struct child_process *get_helper(struct transport *transport)\n {\n \tstruct helper_data *data = transport->data;\n@@ -48,15 +89,16 @@ static struct child_process *get_helper(struct transport *transport)\n \t\tdie(\"Unable to run helper: git %s\", helper->argv[0]);\n \tdata->helper = helper;\n \n-\twrite_str_in_full(helper->in, \"capabilities\\n\");\n+\twrite_constant(helper->in, \"capabilities\\n\");\n \n \tdata->out = xfdopen(helper->out, \"r\");\n \twhile (1) {\n-\t\tif (strbuf_getline(&buf, data->out, '\\n') == EOF)\n-\t\t\texit(128); /* child died, message supplied already */\n+\t\trecvline(data, &buf);\n \n \t\tif (!*buf.buf)\n \t\t\tbreak;\n+\t\tif (debug)\n+\t\t\tfprintf(stderr, \"Debug: Got cap %s\\n\", buf.buf);\n \t\tif (!strcmp(buf.buf, \"fetch\"))\n \t\t\tdata->fetch = 1;\n \t\tif (!strcmp(buf.buf, \"option\"))\n@@ -82,14 +124,21 @@ static struct child_process *get_helper(struct transport *transport)\n \t\tfree(refspecs);\n \t}\n \tstrbuf_release(&buf);\n+\tif (debug)\n+\t\tfprintf(stderr, \"Debug: Capabilities complete.\\n\");\n \treturn data->helper;\n }\n \n static int disconnect_helper(struct transport *transport)\n {\n \tstruct helper_data *data = transport->data;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n \tif (data->helper) {\n-\t\twrite_str_in_full(data->helper->in, \"\\n\");\n+\t\tif (debug)\n+\t\t\tfprintf(stderr, \"Debug: Disconnecting.\\n\");\n+\t\tstrbuf_addf(&buf, \"\\n\");\n+\t\tsendline(data, &buf);\n \t\tclose(data->helper->in);\n \t\tfclose(data->out);\n \t\tfinish_command(data->helper);\n@@ -117,10 +166,11 @@ static int set_helper_option(struct transport *transport,\n \t\t\t const char *name, const char *value)\n {\n \tstruct helper_data *data = transport->data;\n-\tstruct child_process *helper = get_helper(transport);\n \tstruct strbuf buf = STRBUF_INIT;\n \tint i, ret, is_bool = 0;\n \n+\tget_helper(transport);\n+\n \tif (!data->option)\n \t\treturn 1;\n \n@@ -143,12 +193,7 @@ static int set_helper_option(struct transport *transport,\n \t\tquote_c_style(value, &buf, NULL, 0);\n \tstrbuf_addch(&buf, '\\n');\n \n-\tif (write_in_full(helper->in, buf.buf, buf.len) != buf.len)\n-\t\tdie_errno(\"cannot send option to %s\", data->name);\n-\n-\tstrbuf_reset(&buf);\n-\tif (strbuf_getline(&buf, data->out, '\\n') == EOF)\n-\t\texit(128); /* child died, message supplied already */\n+\txchgline(data, &buf);\n \n \tif (!strcmp(buf.buf, \"ok\"))\n \t\tret = 0;\n@@ -208,13 +253,10 @@ static int fetch_with_fetch(struct transport *transport,\n \t}\n \n \tstrbuf_addch(&buf, '\\n');\n-\tif (write_in_full(data->helper->in, buf.buf, buf.len) != buf.len)\n-\t\tdie_errno(\"cannot send fetch to %s\", data->name);\n+\tsendline(data, &buf);\n \n \twhile (1) {\n-\t\tstrbuf_reset(&buf);\n-\t\tif (strbuf_getline(&buf, data->out, '\\n') == EOF)\n-\t\t\texit(128); /* child died, message supplied already */\n+\t\trecvline(data, &buf);\n \n \t\tif (!prefixcmp(buf.buf, \"lock \")) {\n \t\t\tconst char *name = buf.buf + 5;\n@@ -249,12 +291,13 @@ static int fetch_with_import(struct transport *transport,\n \t\t\t int nr_heads, struct ref **to_fetch)\n {\n \tstruct child_process fastimport;\n-\tstruct child_process *helper = get_helper(transport);\n \tstruct helper_data *data = transport->data;\n \tint i;\n \tstruct ref *posn;\n \tstruct strbuf buf = STRBUF_INIT;\n \n+\tget_helper(transport);\n+\n \tif (get_importer(transport, &fastimport))\n \t\tdie(\"Couldn't run fast-import\");\n \n@@ -264,7 +307,7 @@ static int fetch_with_import(struct transport *transport,\n \t\t\tcontinue;\n \n \t\tstrbuf_addf(&buf, \"import %s\\n\", posn->name);\n-\t\twrite_in_full(helper->in, buf.buf, buf.len);\n+\t\tsendline(data, &buf);\n \t\tstrbuf_reset(&buf);\n \t}\n \tdisconnect_helper(transport);\n@@ -369,17 +412,14 @@ static int push_refs(struct transport *transport,\n \t}\n \n \tstrbuf_addch(&buf, '\\n');\n-\tif (write_in_full(helper->in, buf.buf, buf.len) != buf.len)\n-\t\texit(128);\n+\tsendline(data, &buf);\n \n \tref = remote_refs;\n \twhile (1) {\n \t\tchar *refname, *msg;\n \t\tint status;\n \n-\t\tstrbuf_reset(&buf);\n-\t\tif (strbuf_getline(&buf, data->out, '\\n') == EOF)\n-\t\t\texit(128); /* child died, message supplied already */\n+\t\trecvline(data, &buf);\n \t\tif (!buf.len)\n \t\t\tbreak;\n \n@@ -471,8 +511,7 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)\n \n \twhile (1) {\n \t\tchar *eov, *eon;\n-\t\tif (strbuf_getline(&buf, data->out, '\\n') == EOF)\n-\t\t\texit(128); /* child died, message supplied already */\n+\t\trecvline(data, &buf);\n \n \t\tif (!*buf.buf)\n \t\t\tbreak;\n@@ -497,6 +536,8 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)\n \t\t}\n \t\ttail = &((*tail)->next);\n \t}\n+\tif (debug)\n+\t\tfprintf(stderr, \"Debug: Read ref listing.\\n\");\n \tstrbuf_release(&buf);\n \n \tfor (posn = ret; posn; posn = posn->next)\n@@ -510,6 +551,9 @@ int transport_helper_init(struct transport *transport, const char *name)\n \tstruct helper_data *data = xcalloc(sizeof(*data), 1);\n \tdata->name = name;\n \n+\tif (getenv(\"GIT_TRANSPORT_HELPER_DEBUG\"))\n+\t\tdebug = 1;\n+\n \ttransport->data = data;\n \ttransport->set_option = set_helper_option;\n \ttransport->get_refs_list = get_refs_list;\n-- \n1.6.6.rc1.300.gfbc27\n"},{"id":"129525","messageId":"1260278177-9029-3-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"21901","inReplyTo":"1260278177-9029-1-git-send-email-ilari.liusvaara@elisanet.fi","subject":"[REROLL PATCH 2/8] Support mandatory capabilities","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-08T13:16:11Z","receivedAt":"2009-12-08T13:16:11Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"Add support for marking capability as mandatory for hosting git version\nto understand. This is useful for helpers which require various types\nof assistance from main git binary.\n\nSigned-off-by: Ilari Liusvaara \nSigned-off-by: Junio C Hamano \nSigned-off-by: Ilari Liusvaara \n---\n Documentation/git-remote-helpers.txt | 5 ++++-\n transport-helper.c | 26 ++++++++++++++++++++------\n 2 files changed, 24 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-remote-helpers.txt b/Documentation/git-remote-helpers.txt\nindex 5cfdc0c..20a05fe 100644\n--- a/Documentation/git-remote-helpers.txt\n+++ b/Documentation/git-remote-helpers.txt\n@@ -25,7 +25,10 @@ Commands are given by the caller on the helper's standard input, one per line.\n \n 'capabilities'::\n \tLists the capabilities of the helper, one per line, ending\n-\twith a blank line.\n+\twith a blank line. Each capability may be preceeded with '*'.\n+\tThis marks them mandatory for git version using the remote\n+\thelper to understand (unknown mandatory capability is fatal\n+\terror).\n \n 'list'::\n \tLists the refs, one per line, in the format \" \ndiff --git a/transport-helper.c b/transport-helper.c\nindex a721dc2..f977d28 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -93,25 +93,39 @@ static struct child_process *get_helper(struct transport *transport)\n \n \tdata->out = xfdopen(helper->out, \"r\");\n \twhile (1) {\n+\t\tconst char *capname;\n+\t\tint mandatory = 0;\n \t\trecvline(data, &buf);\n \n \t\tif (!*buf.buf)\n \t\t\tbreak;\n+\n+\t\tif (*buf.buf == '*') {\n+\t\t\tcapname = buf.buf + 1;\n+\t\t\tmandatory = 1;\n+\t\t} else\n+\t\t\tcapname = buf.buf;\n+\n \t\tif (debug)\n-\t\t\tfprintf(stderr, \"Debug: Got cap %s\\n\", buf.buf);\n-\t\tif (!strcmp(buf.buf, \"fetch\"))\n+\t\t\tfprintf(stderr, \"Debug: Got cap %s\\n\", capname);\n+\t\tif (!strcmp(capname, \"fetch\"))\n \t\t\tdata->fetch = 1;\n-\t\tif (!strcmp(buf.buf, \"option\"))\n+\t\telse if (!strcmp(capname, \"option\"))\n \t\t\tdata->option = 1;\n-\t\tif (!strcmp(buf.buf, \"push\"))\n+\t\telse if (!strcmp(capname, \"push\"))\n \t\t\tdata->push = 1;\n-\t\tif (!strcmp(buf.buf, \"import\"))\n+\t\telse if (!strcmp(capname, \"import\"))\n \t\t\tdata->import = 1;\n-\t\tif (!data->refspecs && !prefixcmp(buf.buf, \"refspec \")) {\n+\t\telse if (!data->refspecs && !prefixcmp(capname, \"refspec \")) {\n \t\t\tALLOC_GROW(refspecs,\n \t\t\t\t refspec_nr + 1,\n \t\t\t\t refspec_alloc);\n \t\t\trefspecs[refspec_nr++] = strdup(buf.buf + strlen(\"refspec \"));\n+\t\t} else if (mandatory) {\n+\t\t\tfflush(stderr);\n+\t\t\tdie(\"Unknown madatory capability %s. This remote \"\n+\t\t\t \"helper probably needs newer version of Git.\\n\",\n+\t\t\t capname);\n \t\t}\n \t}\n \tif (refspecs) {\n-- \n1.6.6.rc1.300.gfbc27\n"},{"id":"129523","messageId":"1260278177-9029-4-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"21901","inReplyTo":"1260278177-9029-1-git-send-email-ilari.liusvaara@elisanet.fi","subject":"[REROLL PATCH 3/8] Pass unknown protocols to external protocol handlers","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-08T13:16:12Z","receivedAt":"2009-12-08T13:16:12Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"Change URL handling to allow external protocol handlers to implement\nnew protocols without the '::' syntax if helper name does not conflict\nwith any built-in protocol.\n\nfoo:// now invokes git-remote-foo with foo:// URL.\n\nSigned-off-by: Ilari Liusvaara \nSigned-off-by: Junio C Hamano \nSigned-off-by: Ilari Liusvaara \n---\n transport-helper.c | 22 ++++++++++++-\n transport.c | 90 +++++++++++++++++++++++++++++++++++++++++++---------\n 2 files changed, 96 insertions(+), 16 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex f977d28..0e82553 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -63,6 +63,26 @@ static void write_constant(int fd, const char *str)\n \t\tdie_errno(\"Full write to remote helper failed\");\n }\n \n+const char *remove_ext_force(const char *url)\n+{\n+\tconst char *url2 = url;\n+\tconst char *first_colon = NULL;\n+\n+\tif (!url)\n+\t\treturn NULL;\n+\n+\twhile (*url2 && !first_colon)\n+\t\tif (*url2 == ':')\n+\t\t\tfirst_colon = url2;\n+\t\telse\n+\t\t\turl2++;\n+\n+\tif (first_colon && first_colon[1] == ':')\n+\t\treturn first_colon + 2;\n+\telse\n+\t\treturn url;\n+}\n+\n static struct child_process *get_helper(struct transport *transport)\n {\n \tstruct helper_data *data = transport->data;\n@@ -83,7 +103,7 @@ static struct child_process *get_helper(struct transport *transport)\n \tstrbuf_addf(&buf, \"remote-%s\", data->name);\n \thelper->argv[0] = strbuf_detach(&buf, NULL);\n \thelper->argv[1] = transport->remote->name;\n-\thelper->argv[2] = transport->url;\n+\thelper->argv[2] = remove_ext_force(transport->url);\n \thelper->git_cmd = 1;\n \tif (start_command(helper))\n \t\tdie(\"Unable to run helper: git %s\", helper->argv[0]);\ndiff --git a/transport.c b/transport.c\nindex 3eea836..e42a82b 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -780,6 +780,58 @@ static int is_file(const char *url)\n \treturn S_ISREG(buf.st_mode);\n }\n \n+static const char *strchrc(const char *str, int c)\n+{\n+\twhile (*str)\n+\t\tif (*str == c)\n+\t\t\treturn str;\n+\t\telse\n+\t\t\tstr++;\n+\treturn NULL;\n+}\n+\n+static int is_url(const char *url)\n+{\n+\tconst char *url2, *first_slash;\n+\n+\tif (!url)\n+\t\treturn 0;\n+\turl2 = url;\n+\tfirst_slash = strchrc(url, '/');\n+\n+\t/* Input with no slash at all or slash first can't be URL. */\n+\tif (!first_slash || first_slash == url)\n+\t\treturn 0;\n+\t/* Character before must be : and next must be /. */\n+\tif (first_slash[-1] != ':' || first_slash[1] != '/')\n+\t\treturn 0;\n+\t/* There must be something before the :// */\n+\tif (first_slash == url + 1)\n+\t\treturn 0;\n+\t/*\n+\t * Check all characters up to first slash. Only alpha, num and\n+\t * colon (\":\") are allowed. \":\" must be followed by \":\" or \"/\".\n+\t */\n+\turl2 = url;\n+\twhile (url2 < first_slash) {\n+\t\tif (*url2 != ':' && !isalnum((unsigned char)*url2))\n+\t\t\treturn 0;\n+\t\tif (*url2 == ':' && url2[1] != ':' && url2[1] != '/')\n+\t\t\treturn 0;\n+\t\tif (*url2 == ':')\n+\t\t\turl2++;\t\t/* Skip second : */\n+\t\turl2++;\n+\t}\n+\n+\t/* Valid enough. */\n+\treturn 1;\n+}\n+\n+static int external_specification_len(const char *url)\n+{\n+\treturn strchrc(url, ':') - url;\n+}\n+\n struct transport *transport_get(struct remote *remote, const char *url)\n {\n \tstruct transport *ret = xcalloc(1, sizeof(*ret));\n@@ -805,30 +857,23 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \n \tif (remote && remote->foreign_vcs) {\n \t\ttransport_helper_init(ret, remote->foreign_vcs);\n-\t\treturn ret;\n-\t}\n-\n-\tif (!prefixcmp(url, \"rsync:\")) {\n+\t} else if (!prefixcmp(url, \"rsync:\")) {\n \t\tret->get_refs_list = get_refs_via_rsync;\n \t\tret->fetch = fetch_objs_via_rsync;\n \t\tret->push = rsync_transport_push;\n-\n-\t} else if (!prefixcmp(url, \"http://\")\n-\t || !prefixcmp(url, \"https://\")\n-\t || !prefixcmp(url, \"ftp://\")) {\n-\t\ttransport_helper_init(ret, \"curl\");\n-#ifdef NO_CURL\n-\t\terror(\"git was compiled without libcurl support.\");\n-#endif\n-\n \t} else if (is_local(url) && is_file(url)) {\n \t\tstruct bundle_transport_data *data = xcalloc(1, sizeof(*data));\n \t\tret->data = data;\n \t\tret->get_refs_list = get_refs_from_bundle;\n \t\tret->fetch = fetch_refs_from_bundle;\n \t\tret->disconnect = close_bundle;\n-\n-\t} else {\n+\t} else if (!is_url(url)\n+\t\t|| !prefixcmp(url, \"file://\")\n+\t\t|| !prefixcmp(url, \"git://\")\n+\t\t|| !prefixcmp(url, \"ssh://\")\n+\t\t|| !prefixcmp(url, \"git+ssh://\")\n+\t\t|| !prefixcmp(url, \"ssh+git://\")) {\n+\t\t/* These are builtin smart transports. */\n \t\tstruct git_transport_data *data = xcalloc(1, sizeof(*data));\n \t\tret->data = data;\n \t\tret->set_option = set_git_option;\n@@ -845,6 +890,21 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \t\tdata->receivepack = \"git-receive-pack\";\n \t\tif (remote->receivepack)\n \t\t\tdata->receivepack = remote->receivepack;\n+\t} else if (!prefixcmp(url, \"http://\")\n+\t\t|| !prefixcmp(url, \"https://\")\n+\t\t|| !prefixcmp(url, \"ftp://\")) {\n+\t\t/* These three are just plain special. */\n+\t\ttransport_helper_init(ret, \"curl\");\n+#ifdef NO_CURL\n+\t\terror(\"git was compiled without libcurl support.\");\n+#endif\n+\t} else {\n+\t\t/* Unknown protocol in URL. Pass to external handler. */\n+\t\tint len = external_specification_len(url);\n+\t\tchar *handler = xmalloc(len + 1);\n+\t\thandler[len] = 0;\n+\t\tstrncpy(handler, url, len);\n+\t\ttransport_helper_init(ret, handler);\n \t}\n \n \treturn ret;\n-- \n1.6.6.rc1.300.gfbc27\n"},{"id":"129522","messageId":"1260278177-9029-5-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"21901","inReplyTo":"1260278177-9029-1-git-send-email-ilari.liusvaara@elisanet.fi","subject":"[REROLL PATCH 4/8] Refactor git transport options parsing","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-08T13:16:13Z","receivedAt":"2009-12-08T13:16:13Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"Refactor the transport options parsing so that protocols that aren't\ndirectly smart transports (file://, git://, ssh:// & co) can record\nthe smart transport options for the case if it turns that transport\ncan actually be smart.\n\nSigned-off-by: Ilari Liusvaara \nSigned-off-by: Junio C Hamano \nSigned-off-by: Ilari Liusvaara \n---\n transport.c | 78 +++++++++++++++++++++++++++++++++++-----------------------\n transport.h | 15 +++++++++++\n 2 files changed, 62 insertions(+), 31 deletions(-)\n\ndiff --git a/transport.c b/transport.c\nindex e42a82b..b3e22ec 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -395,41 +395,35 @@ static int close_bundle(struct transport *transport)\n }\n \n struct git_transport_data {\n-\tunsigned thin : 1;\n-\tunsigned keep : 1;\n-\tunsigned followtags : 1;\n-\tint depth;\n+\tstruct git_transport_options options;\n \tstruct child_process *conn;\n \tint fd[2];\n-\tconst char *uploadpack;\n-\tconst char *receivepack;\n \tstruct extra_have_objects extra_have;\n };\n \n-static int set_git_option(struct transport *connection,\n+static int set_git_option(struct git_transport_options *opts,\n \t\t\t const char *name, const char *value)\n {\n-\tstruct git_transport_data *data = connection->data;\n \tif (!strcmp(name, TRANS_OPT_UPLOADPACK)) {\n-\t\tdata->uploadpack = value;\n+\t\topts->uploadpack = value;\n \t\treturn 0;\n \t} else if (!strcmp(name, TRANS_OPT_RECEIVEPACK)) {\n-\t\tdata->receivepack = value;\n+\t\topts->receivepack = value;\n \t\treturn 0;\n \t} else if (!strcmp(name, TRANS_OPT_THIN)) {\n-\t\tdata->thin = !!value;\n+\t\topts->thin = !!value;\n \t\treturn 0;\n \t} else if (!strcmp(name, TRANS_OPT_FOLLOWTAGS)) {\n-\t\tdata->followtags = !!value;\n+\t\topts->followtags = !!value;\n \t\treturn 0;\n \t} else if (!strcmp(name, TRANS_OPT_KEEP)) {\n-\t\tdata->keep = !!value;\n+\t\topts->keep = !!value;\n \t\treturn 0;\n \t} else if (!strcmp(name, TRANS_OPT_DEPTH)) {\n \t\tif (!value)\n-\t\t\tdata->depth = 0;\n+\t\t\topts->depth = 0;\n \t\telse\n-\t\t\tdata->depth = atoi(value);\n+\t\t\topts->depth = atoi(value);\n \t\treturn 0;\n \t}\n \treturn 1;\n@@ -439,7 +433,8 @@ static int connect_setup(struct transport *transport, int for_push, int verbose)\n {\n \tstruct git_transport_data *data = transport->data;\n \tdata->conn = git_connect(data->fd, transport->url,\n-\t\t\t\t for_push ? data->receivepack : data->uploadpack,\n+\t\t\t\t for_push ? data->options.receivepack :\n+\t\t\t\t data->options.uploadpack,\n \t\t\t\t verbose ? CONNECT_VERBOSE : 0);\n \treturn 0;\n }\n@@ -469,15 +464,15 @@ static int fetch_refs_via_pack(struct transport *transport,\n \tstruct ref *refs_tmp = NULL;\n \n \tmemset(&args, 0, sizeof(args));\n-\targs.uploadpack = data->uploadpack;\n-\targs.keep_pack = data->keep;\n+\targs.uploadpack = data->options.uploadpack;\n+\targs.keep_pack = data->options.keep;\n \targs.lock_pack = 1;\n-\targs.use_thin_pack = data->thin;\n-\targs.include_tag = data->followtags;\n+\targs.use_thin_pack = data->options.thin;\n+\targs.include_tag = data->options.followtags;\n \targs.verbose = (transport->verbose > 0);\n \targs.quiet = (transport->verbose < 0);\n \targs.no_progress = args.quiet || (!transport->progress && !isatty(1));\n-\targs.depth = data->depth;\n+\targs.depth = data->options.depth;\n \n \tfor (i = 0; i < nr_heads; i++)\n \t\torigh[i] = heads[i] = xstrdup(to_fetch[i]->name);\n@@ -734,7 +729,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n \tmemset(&args, 0, sizeof(args));\n \targs.send_mirror = !!(flags & TRANSPORT_PUSH_MIRROR);\n \targs.force_update = !!(flags & TRANSPORT_PUSH_FORCE);\n-\targs.use_thin_pack = data->thin;\n+\targs.use_thin_pack = data->options.thin;\n \targs.verbose = !!(flags & TRANSPORT_PUSH_VERBOSE);\n \targs.quiet = !!(flags & TRANSPORT_PUSH_QUIET);\n \targs.dry_run = !!(flags & TRANSPORT_PUSH_DRY_RUN);\n@@ -861,12 +856,14 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \t\tret->get_refs_list = get_refs_via_rsync;\n \t\tret->fetch = fetch_objs_via_rsync;\n \t\tret->push = rsync_transport_push;\n+\t\tret->smart_options = NULL;\n \t} else if (is_local(url) && is_file(url)) {\n \t\tstruct bundle_transport_data *data = xcalloc(1, sizeof(*data));\n \t\tret->data = data;\n \t\tret->get_refs_list = get_refs_from_bundle;\n \t\tret->fetch = fetch_refs_from_bundle;\n \t\tret->disconnect = close_bundle;\n+\t\tret->smart_options = NULL;\n \t} else if (!is_url(url)\n \t\t|| !prefixcmp(url, \"file://\")\n \t\t|| !prefixcmp(url, \"git://\")\n@@ -876,20 +873,14 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \t\t/* These are builtin smart transports. */\n \t\tstruct git_transport_data *data = xcalloc(1, sizeof(*data));\n \t\tret->data = data;\n-\t\tret->set_option = set_git_option;\n+\t\tret->set_option = NULL;\n \t\tret->get_refs_list = get_refs_via_connect;\n \t\tret->fetch = fetch_refs_via_pack;\n \t\tret->push_refs = git_transport_push;\n \t\tret->disconnect = disconnect_git;\n+\t\tret->smart_options = &(data->options);\n \n-\t\tdata->thin = 1;\n \t\tdata->conn = NULL;\n-\t\tdata->uploadpack = \"git-upload-pack\";\n-\t\tif (remote->uploadpack)\n-\t\t\tdata->uploadpack = remote->uploadpack;\n-\t\tdata->receivepack = \"git-receive-pack\";\n-\t\tif (remote->receivepack)\n-\t\t\tdata->receivepack = remote->receivepack;\n \t} else if (!prefixcmp(url, \"http://\")\n \t\t|| !prefixcmp(url, \"https://\")\n \t\t|| !prefixcmp(url, \"ftp://\")) {\n@@ -907,14 +898,39 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \t\ttransport_helper_init(ret, handler);\n \t}\n \n+\tif (ret->smart_options) {\n+\t\tret->smart_options->thin = 1;\n+\t\tret->smart_options->uploadpack = \"git-upload-pack\";\n+\t\tif (remote->uploadpack)\n+\t\t\tret->smart_options->uploadpack = remote->uploadpack;\n+\t\tret->smart_options->receivepack = \"git-receive-pack\";\n+\t\tif (remote->receivepack)\n+\t\t\tret->smart_options->receivepack = remote->receivepack;\n+\t}\n+\n \treturn ret;\n }\n \n int transport_set_option(struct transport *transport,\n \t\t\t const char *name, const char *value)\n {\n+\tint git_reports = 1, protocol_reports = 1;\n+\n+\tif (transport->smart_options)\n+\t\tgit_reports = set_git_option(transport->smart_options,\n+\t\t\t\t\t name, value);\n+\n \tif (transport->set_option)\n-\t\treturn transport->set_option(transport, name, value);\n+\t\tprotocol_reports = transport->set_option(transport, name,\n+\t\t\t\t\t\t\tvalue);\n+\n+\t/* If either report is 0, report 0 (success). */\n+\tif (!git_reports || !protocol_reports)\n+\t\treturn 0;\n+\t/* If either reports -1 (invalid value), report -1. */\n+\tif ((git_reports == -1) || (protocol_reports == -1))\n+\t\treturn -1;\n+\t/* Otherwise if both report unknown, report unknown. */\n \treturn 1;\n }\n \ndiff --git a/transport.h b/transport.h\nindex 9e74406..e90c285 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -4,6 +4,15 @@\n #include \"cache.h\"\n #include \"remote.h\"\n \n+struct git_transport_options {\n+\tunsigned thin : 1;\n+\tunsigned keep : 1;\n+\tunsigned followtags : 1;\n+\tint depth;\n+\tconst char *uploadpack;\n+\tconst char *receivepack;\n+};\n+\n struct transport {\n \tstruct remote *remote;\n \tconst char *url;\n@@ -65,6 +74,12 @@ struct transport {\n \tsigned verbose : 3;\n \t/* Force progress even if the output is not a tty */\n \tunsigned progress : 1;\n+\t/*\n+\t * If transport is at least potentially smart, this points to\n+\t * git_transport_options structure to use in case transport\n+\t * actually turns out to be smart.\n+\t */\n+\tstruct git_transport_options *smart_options;\n };\n \n #define TRANSPORT_PUSH_ALL 1\n-- \n1.6.6.rc1.300.gfbc27\n"},{"id":"129528","messageId":"1260278177-9029-6-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"21901","inReplyTo":"1260278177-9029-1-git-send-email-ilari.liusvaara@elisanet.fi","subject":"[REROLL PATCH 5/8] Support taking over transports","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-08T13:16:14Z","receivedAt":"2009-12-08T13:16:14Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"Add support for taking over transports that turn out to be smart.\n\nSigned-off-by: Ilari Liusvaara \nSigned-off-by: Junio C Hamano \nSigned-off-by: Ilari Liusvaara \n---\n transport-helper.c | 16 +++++++++++++++-\n transport.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++-----\n transport.h | 2 ++\n 3 files changed, 63 insertions(+), 6 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 0e82553..3b7340c 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -22,6 +22,7 @@ struct helper_data\n \t/* These go from remote name (as in \"list\") to private name */\n \tstruct refspec *refspecs;\n \tint refspec_nr;\n+\tstruct git_transport_options gitoptions;\n };\n \n static void sendline(struct helper_data *helper, struct strbuf *buffer)\n@@ -91,6 +92,7 @@ static struct child_process *get_helper(struct transport *transport)\n \tconst char **refspecs = NULL;\n \tint refspec_nr = 0;\n \tint refspec_alloc = 0;\n+\tint duped;\n \n \tif (data->helper)\n \t\treturn data->helper;\n@@ -109,9 +111,19 @@ static struct child_process *get_helper(struct transport *transport)\n \t\tdie(\"Unable to run helper: git %s\", helper->argv[0]);\n \tdata->helper = helper;\n \n+\t/*\n+\t * Open the output as FILE* so strbuf_getline() can be used.\n+\t * Do this with duped fd because fclose() will close the fd,\n+\t * and stuff like taking over will require the fd to remain.\n+\t *\n+\t */\n+\tduped = dup(helper->out);\n+\tif (duped < 0)\n+\t\tdie_errno(\"Can't dup helper output fd\");\n+\tdata->out = xfdopen(duped, \"r\");\n+\n \twrite_constant(helper->in, \"capabilities\\n\");\n \n-\tdata->out = xfdopen(helper->out, \"r\");\n \twhile (1) {\n \t\tconst char *capname;\n \t\tint mandatory = 0;\n@@ -174,6 +186,7 @@ static int disconnect_helper(struct transport *transport)\n \t\tstrbuf_addf(&buf, \"\\n\");\n \t\tsendline(data, &buf);\n \t\tclose(data->helper->in);\n+\t\tclose(data->helper->out);\n \t\tfclose(data->out);\n \t\tfinish_command(data->helper);\n \t\tfree((char *)data->helper->argv[0]);\n@@ -594,5 +607,6 @@ int transport_helper_init(struct transport *transport, const char *name)\n \ttransport->fetch = fetch;\n \ttransport->push_refs = push_refs;\n \ttransport->disconnect = release_helper;\n+\ttransport->smart_options = &(data->gitoptions);\n \treturn 0;\n }\ndiff --git a/transport.c b/transport.c\nindex b3e22ec..c4ecec4 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -398,6 +398,7 @@ struct git_transport_data {\n \tstruct git_transport_options options;\n \tstruct child_process *conn;\n \tint fd[2];\n+\tunsigned got_remote_heads : 1;\n \tstruct extra_have_objects extra_have;\n };\n \n@@ -432,10 +433,15 @@ static int set_git_option(struct git_transport_options *opts,\n static int connect_setup(struct transport *transport, int for_push, int verbose)\n {\n \tstruct git_transport_data *data = transport->data;\n+\n+\tif (data->conn)\n+\t\treturn 0;\n+\n \tdata->conn = git_connect(data->fd, transport->url,\n \t\t\t\t for_push ? data->options.receivepack :\n \t\t\t\t data->options.uploadpack,\n \t\t\t\t verbose ? CONNECT_VERBOSE : 0);\n+\n \treturn 0;\n }\n \n@@ -447,6 +453,7 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus\n \tconnect_setup(transport, for_push, 0);\n \tget_remote_heads(data->fd[0], &refs, 0, NULL,\n \t\t\t for_push ? REF_NORMAL : 0, &data->extra_have);\n+\tdata->got_remote_heads = 1;\n \n \treturn refs;\n }\n@@ -477,9 +484,10 @@ static int fetch_refs_via_pack(struct transport *transport,\n \tfor (i = 0; i < nr_heads; i++)\n \t\torigh[i] = heads[i] = xstrdup(to_fetch[i]->name);\n \n-\tif (!data->conn) {\n+\tif (!data->got_remote_heads) {\n \t\tconnect_setup(transport, 0, 0);\n \t\tget_remote_heads(data->fd[0], &refs_tmp, 0, NULL, 0, NULL);\n+\t\tdata->got_remote_heads = 1;\n \t}\n \n \trefs = fetch_pack(&args, data->fd, data->conn,\n@@ -490,6 +498,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n \tif (finish_connect(data->conn))\n \t\trefs = NULL;\n \tdata->conn = NULL;\n+\tdata->got_remote_heads = 0;\n \n \tfree_refs(refs_tmp);\n \n@@ -718,12 +727,13 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n \tstruct send_pack_args args;\n \tint ret;\n \n-\tif (!data->conn) {\n+\tif (!data->got_remote_heads) {\n \t\tstruct ref *tmp_refs;\n \t\tconnect_setup(transport, 1, 0);\n \n \t\tget_remote_heads(data->fd[0], &tmp_refs, 0, NULL, REF_NORMAL,\n \t\t\t\t NULL);\n+\t\tdata->got_remote_heads = 1;\n \t}\n \n \tmemset(&args, 0, sizeof(args));\n@@ -741,6 +751,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n \tclose(data->fd[0]);\n \tret |= finish_connect(data->conn);\n \tdata->conn = NULL;\n+\tdata->got_remote_heads = 0;\n \n \treturn ret;\n }\n@@ -749,7 +760,8 @@ static int disconnect_git(struct transport *transport)\n {\n \tstruct git_transport_data *data = transport->data;\n \tif (data->conn) {\n-\t\tpacket_flush(data->fd[1]);\n+\t\tif (data->got_remote_heads)\n+\t\t\tpacket_flush(data->fd[1]);\n \t\tclose(data->fd[0]);\n \t\tclose(data->fd[1]);\n \t\tfinish_connect(data->conn);\n@@ -759,6 +771,32 @@ static int disconnect_git(struct transport *transport)\n \treturn 0;\n }\n \n+void transport_take_over(struct transport *transport,\n+\t\t\t struct child_process *child)\n+{\n+\tstruct git_transport_data *data;\n+\n+\tif (!transport->smart_options)\n+\t\tdie(\"Bug detected: Taking over transport requires non-NULL \"\n+\t\t \"smart_options field.\");\n+\n+\tdata = xcalloc(1, sizeof(*data));\n+\tdata->options = *transport->smart_options;\n+\tdata->conn = child;\n+\tdata->fd[0] = data->conn->out;\n+\tdata->fd[1] = data->conn->in;\n+\tdata->got_remote_heads = 0;\n+\ttransport->data = data;\n+\n+\ttransport->set_option = NULL;\n+\ttransport->get_refs_list = get_refs_via_connect;\n+\ttransport->fetch = fetch_refs_via_pack;\n+\ttransport->push = NULL;\n+\ttransport->push_refs = git_transport_push;\n+\ttransport->disconnect = disconnect_git;\n+\ttransport->smart_options = &(data->options);\n+}\n+\n static int is_local(const char *url)\n {\n \tconst char *colon = strchr(url, ':');\n@@ -881,6 +919,7 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \t\tret->smart_options = &(data->options);\n \n \t\tdata->conn = NULL;\n+\t\tdata->got_remote_heads = 0;\n \t} else if (!prefixcmp(url, \"http://\")\n \t\t|| !prefixcmp(url, \"https://\")\n \t\t|| !prefixcmp(url, \"ftp://\")) {\n@@ -941,9 +980,9 @@ int transport_push(struct transport *transport,\n \t*nonfastforward = 0;\n \tverify_remote_names(refspec_nr, refspec);\n \n-\tif (transport->push)\n+\tif (transport->push) {\n \t\treturn transport->push(transport, refspec_nr, refspec, flags);\n-\tif (transport->push_refs) {\n+\t} else if (transport->push_refs) {\n \t\tstruct ref *remote_refs =\n \t\t\ttransport->get_refs_list(transport, 1);\n \t\tstruct ref *local_refs = get_local_heads();\n@@ -987,6 +1026,7 @@ const struct ref *transport_get_remote_refs(struct transport *transport)\n {\n \tif (!transport->remote_refs)\n \t\ttransport->remote_refs = transport->get_refs_list(transport, 0);\n+\n \treturn transport->remote_refs;\n }\n \n@@ -1021,6 +1061,7 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)\n \t}\n \n \trc = transport->fetch(transport, nr_heads, heads);\n+\n \tfree(heads);\n \treturn rc;\n }\ndiff --git a/transport.h b/transport.h\nindex e90c285..781db2e 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -130,6 +130,8 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs);\n void transport_unlock_pack(struct transport *transport);\n int transport_disconnect(struct transport *transport);\n char *transport_anonymize_url(const char *url);\n+void transport_take_over(struct transport *transport,\n+\t\t\t struct child_process *child);\n \n /* Transport methods defined outside transport.c */\n int transport_helper_init(struct transport *transport, const char *name);\n-- \n1.6.6.rc1.300.gfbc27\n"},{"id":"129524","messageId":"1260278177-9029-7-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"21901","inReplyTo":"1260278177-9029-1-git-send-email-ilari.liusvaara@elisanet.fi","subject":"[REROLL PATCH 6/8] Support remote helpers implementing smart transports","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-08T13:16:15Z","receivedAt":"2009-12-08T13:16:15Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"Signed-off-by: Ilari Liusvaara \nSigned-off-by: Junio C Hamano \nSigned-off-by: Ilari Liusvaara \n---\n Documentation/git-remote-helpers.txt | 25 +++++++-\n transport-helper.c | 126 ++++++++++++++++++++++++++++++++--\n 2 files changed, 144 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-remote-helpers.txt b/Documentation/git-remote-helpers.txt\nindex 20a05fe..b957813 100644\n--- a/Documentation/git-remote-helpers.txt\n+++ b/Documentation/git-remote-helpers.txt\n@@ -93,6 +93,20 @@ Supported if the helper has the \"push\" capability.\n +\n Supported if the helper has the \"import\" capability.\n \n+'connect' ::\n+\tConnects to given service. Stdin and stdout of helper are\n+\tconnected to specified service (git prefix is included in service\n+\tname so e.g. fetching uses 'git-upload-pack' as service) on\n+\tremote side. Valid replies to this command are empty line\n+\t(connection established), 'fallback' (no smart transport support,\n+\tfall back to dumb transports) and just exiting with error message\n+\tprinted (can't connect, don't bother trying to fall back). After\n+\tline feed terminating the positive (empty) response, the output\n+\tof service starts. After the connection ends, the remote\n+\thelper exits.\n++\n+Supported if the helper has the \"connect\" capability.\n+\n If a fatal error occurs, the program writes the error message to\n stderr and exits. The caller should expect that a suitable error\n message has been printed if the child closes the connection without\n@@ -126,6 +140,9 @@ CAPABILITIES\n \tall, it must cover all refs reported by the list command; if\n \tit is not used, it is effectively \"*:*\"\n \n+'connect'::\n+\tThis helper supports the 'connect' command.\n+\n REF LIST ATTRIBUTES\n -------------------\n \n@@ -168,9 +185,15 @@ OPTIONS\n \tbut don't actually change any repository data.\tFor most\n \thelpers this only applies to the 'push', if supported.\n \n+'option servpath '::\n+\tSet service path (--upload-pack, --receive-pack etc.) for\n+\tnext connect. Remote helper MAY support this option. Remote\n+\thelper MUST NOT rely on this option being set before\n+\tconnect request occurs.\n+\n Documentation\n -------------\n-Documentation by Daniel Barkalow.\n+Documentation by Daniel Barkalow and Ilari Liusvaara\n \n GIT\n ---\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 3b7340c..e30c914 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -18,7 +18,9 @@ struct helper_data\n \tunsigned fetch : 1,\n \t\timport : 1,\n \t\toption : 1,\n-\t\tpush : 1;\n+\t\tpush : 1,\n+\t\tconnect : 1,\n+\t\tno_disconnect_req : 1;\n \t/* These go from remote name (as in \"list\") to private name */\n \tstruct refspec *refspecs;\n \tint refspec_nr;\n@@ -34,12 +36,12 @@ static void sendline(struct helper_data *helper, struct strbuf *buffer)\n \t\tdie_errno(\"Full write to remote helper failed\");\n }\n \n-static int recvline(struct helper_data *helper, struct strbuf *buffer)\n+static int _recvline(FILE *helper, struct strbuf *buffer)\n {\n \tstrbuf_reset(buffer);\n \tif (debug)\n \t\tfprintf(stderr, \"Debug: Remote helper: Waiting...\\n\");\n-\tif (strbuf_getline(buffer, helper->out, '\\n') == EOF) {\n+\tif (strbuf_getline(buffer, helper, '\\n') == EOF) {\n \t\tif (debug)\n \t\t\tfprintf(stderr, \"Debug: Remote helper quit.\\n\");\n \t\texit(128);\n@@ -50,6 +52,11 @@ static int recvline(struct helper_data *helper, struct strbuf *buffer)\n \treturn 0;\n }\n \n+static int recvline(struct helper_data *helper, struct strbuf *buffer)\n+{\n+\treturn _recvline(helper->out, buffer);\n+}\n+\n static void xchgline(struct helper_data *helper, struct strbuf *buffer)\n {\n \tsendline(helper, buffer);\n@@ -84,6 +91,15 @@ const char *remove_ext_force(const char *url)\n \t\treturn url;\n }\n \n+static void do_take_over(struct transport *transport)\n+{\n+\tstruct helper_data *data;\n+\tdata = (struct helper_data*)transport->data;\n+\ttransport_take_over(transport, data->helper);\n+\tfclose(data->out);\n+\tfree(data);\n+}\n+\n static struct child_process *get_helper(struct transport *transport)\n {\n \tstruct helper_data *data = transport->data;\n@@ -110,12 +126,12 @@ static struct child_process *get_helper(struct transport *transport)\n \tif (start_command(helper))\n \t\tdie(\"Unable to run helper: git %s\", helper->argv[0]);\n \tdata->helper = helper;\n+\tdata->no_disconnect_req = 0;\n \n \t/*\n \t * Open the output as FILE* so strbuf_getline() can be used.\n \t * Do this with duped fd because fclose() will close the fd,\n \t * and stuff like taking over will require the fd to remain.\n-\t *\n \t */\n \tduped = dup(helper->out);\n \tif (duped < 0)\n@@ -153,6 +169,8 @@ static struct child_process *get_helper(struct transport *transport)\n \t\t\t\t refspec_nr + 1,\n \t\t\t\t refspec_alloc);\n \t\t\trefspecs[refspec_nr++] = strdup(buf.buf + strlen(\"refspec \"));\n+\t\t} else if (!strcmp(capname, \"connect\")) {\n+\t\t\tdata->connect = 1;\n \t\t} else if (mandatory) {\n \t\t\tfflush(stderr);\n \t\t\tdie(\"Unknown madatory capability %s. This remote \"\n@@ -183,8 +201,10 @@ static int disconnect_helper(struct transport *transport)\n \tif (data->helper) {\n \t\tif (debug)\n \t\t\tfprintf(stderr, \"Debug: Disconnecting.\\n\");\n-\t\tstrbuf_addf(&buf, \"\\n\");\n-\t\tsendline(data, &buf);\n+\t\tif(!data->no_disconnect_req) {\n+\t\t\tstrbuf_addf(&buf, \"\\n\");\n+\t\t\tsendline(data, &buf);\n+\t\t}\n \t\tclose(data->helper->in);\n \t\tclose(data->helper->out);\n \t\tfclose(data->out);\n@@ -378,12 +398,96 @@ static int fetch_with_import(struct transport *transport,\n \treturn 0;\n }\n \n+static int _process_connect(struct transport *transport,\n+\t\t\t\t const char *name, const char *exec)\n+{\n+\tstruct helper_data *data = transport->data;\n+\tstruct strbuf cmdbuf = STRBUF_INIT;\n+\tstruct child_process *helper;\n+\tint r, duped, ret = 0;\n+\tFILE *input;\n+\n+\thelper = get_helper(transport);\n+\n+\t/*\n+\t * Yes, dup the pipe another time, as we need unbuffered version\n+\t * of input pipe as FILE*. fclose() closes the underlying fd and\n+\t * stream buffering only can be changed before first I/O operation\n+\t * on it.\n+\t */\n+\tduped = dup(helper->out);\n+\tif (duped < 0)\n+\t\tdie_errno(\"Can't dup helper output fd\");\n+\tinput = xfdopen(duped, \"r\");\n+\tsetvbuf(input, NULL, _IONBF, 0);\n+\n+\t/*\n+\t * Handle --upload-pack and friends. This is fire and forget...\n+\t * just warn if it fails.\n+\t */\n+\tif (strcmp(name, exec)) {\n+\t\tr = set_helper_option(transport, \"servpath\", exec);\n+\t\tif (r > 0)\n+\t\t\tfprintf(stderr, \"Warning: Setting remote service path \"\n+\t\t\t\t\"not supported by protocol.\\n\");\n+\t\telse if (r < 0)\n+\t\t\tfprintf(stderr, \"Warning: Invalid remote service \"\n+\t\t\t\t\"path.\\n\");\n+\t}\n+\n+\tif (data->connect)\n+\t\tstrbuf_addf(&cmdbuf, \"connect %s\\n\", name);\n+\telse\n+\t\tgoto exit;\n+\n+\tsendline(data, &cmdbuf);\n+\t_recvline(input, &cmdbuf);\n+\tif (!strcmp(cmdbuf.buf, \"\")) {\n+\t\tdata->no_disconnect_req = 1;\n+\t\tif (debug)\n+\t\t\tfprintf(stderr, \"Debug: Smart transport connection \"\n+\t\t\t\t\"ready.\\n\");\n+\t\tret = 1;\n+\t} else if (!strcmp(cmdbuf.buf, \"fallback\")) {\n+\t\tif (debug)\n+\t\t\tfprintf(stderr, \"Debug: Falling back to dumb \"\n+\t\t\t\t\"transport.\\n\");\n+\t} else\n+\t\tdie(\"Unknown response to connect: %s\",\n+\t\t\tcmdbuf.buf);\n+\n+exit:\n+\tfclose(input);\n+\treturn ret;\n+}\n+\n+static int process_connect(struct transport *transport,\n+\t\t\t\t int for_push)\n+{\n+\tstruct helper_data *data = transport->data;\n+\tconst char *name;\n+\tconst char *exec;\n+\n+\tname = for_push ? \"git-receive-pack\" : \"git-upload-pack\";\n+\tif (for_push)\n+\t\texec = data->gitoptions.receivepack;\n+\telse\n+\t\texec = data->gitoptions.uploadpack;\n+\n+\treturn _process_connect(transport, name, exec);\n+}\n+\n static int fetch(struct transport *transport,\n \t\t int nr_heads, struct ref **to_fetch)\n {\n \tstruct helper_data *data = transport->data;\n \tint i, count;\n \n+\tif (process_connect(transport, 0)) {\n+\t\tdo_take_over(transport);\n+\t\treturn transport->fetch(transport, nr_heads, to_fetch);\n+\t}\n+\n \tcount = 0;\n \tfor (i = 0; i < nr_heads; i++)\n \t\tif (!(to_fetch[i]->status & REF_STATUS_UPTODATE))\n@@ -411,6 +515,11 @@ static int push_refs(struct transport *transport,\n \tstruct child_process *helper;\n \tstruct ref *ref;\n \n+\tif (process_connect(transport, 1)) {\n+\t\tdo_take_over(transport);\n+\t\treturn transport->push_refs(transport, remote_refs, flags);\n+\t}\n+\n \tif (!remote_refs)\n \t\treturn 0;\n \n@@ -551,6 +660,11 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)\n \n \thelper = get_helper(transport);\n \n+\tif (process_connect(transport, for_push)) {\n+\t\tdo_take_over(transport);\n+\t\treturn transport->get_refs_list(transport, for_push);\n+\t}\n+\n \tif (data->push && for_push)\n \t\twrite_str_in_full(helper->in, \"list for-push\\n\");\n \telse\n-- \n1.6.6.rc1.300.gfbc27\n"},{"id":"129527","messageId":"1260278177-9029-8-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"21901","inReplyTo":"1260278177-9029-1-git-send-email-ilari.liusvaara@elisanet.fi","subject":"[REROLL PATCH 7/8] Support remote archive from external protocol helpers","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-08T13:16:16Z","receivedAt":"2009-12-08T13:16:16Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"Helpers which support connect also should support remote archive\nsnapshot (or at least there's only one way to attempt it). So support\nremote snapshotting for protocol helpers.\n\nSigned-off-by: Ilari Liusvaara \nSigned-off-by: Junio C Hamano \nSigned-off-by: Ilari Liusvaara \n---\n builtin-archive.c | 17 ++++++++++-------\n transport-helper.c | 19 +++++++++++++++++++\n transport.c | 21 +++++++++++++++++++++\n transport.h | 5 +++++\n 4 files changed, 55 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin-archive.c b/builtin-archive.c\nindex 12351e9..d34b3fd 100644\n--- a/builtin-archive.c\n+++ b/builtin-archive.c\n@@ -5,6 +5,7 @@\n #include \"cache.h\"\n #include \"builtin.h\"\n #include \"archive.h\"\n+#include \"transport.h\"\n #include \"parse-options.h\"\n #include \"pkt-line.h\"\n #include \"sideband.h\"\n@@ -25,12 +26,16 @@ static void create_output_file(const char *output_file)\n static int run_remote_archiver(int argc, const char **argv,\n \t\t\t const char *remote, const char *exec)\n {\n-\tchar *url, buf[LARGE_PACKET_MAX];\n+\tchar buf[LARGE_PACKET_MAX];\n \tint fd[2], i, len, rv;\n-\tstruct child_process *conn;\n+\tstruct transport *transport;\n+\tstruct remote *_remote;\n \n-\turl = xstrdup(remote);\n-\tconn = git_connect(fd, url, exec, 0);\n+\t_remote = remote_get(remote);\n+\tif (!_remote->url[0])\n+\t\tdie(\"git archive: Remote with no URL\");\n+\ttransport = transport_get(_remote, _remote->url[0]);\n+\ttransport_connect(transport, \"git-upload-archive\", exec, fd);\n \n \tfor (i = 1; i < argc; i++)\n \t\tpacket_write(fd[1], \"argument %s\\n\", argv[i]);\n@@ -53,9 +58,7 @@ static int run_remote_archiver(int argc, const char **argv,\n \n \t/* Now, start reading from fd[0] and spit it out to stdout */\n \trv = recv_sideband(\"archive\", fd[0], 1);\n-\tclose(fd[0]);\n-\tclose(fd[1]);\n-\trv |= finish_connect(conn);\n+\trv |= transport_disconnect(transport);\n \n \treturn !!rv;\n }\ndiff --git a/transport-helper.c b/transport-helper.c\nindex e30c914..70c798e 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -477,6 +477,24 @@ static int process_connect(struct transport *transport,\n \treturn _process_connect(transport, name, exec);\n }\n \n+static int connect_helper(struct transport *transport, const char *name,\n+\t\t const char *exec, int fd[2])\n+{\n+\tstruct helper_data *data = transport->data;\n+\n+\t/* Get_helper so connect is inited. */\n+\tget_helper(transport);\n+\tif (!data->connect)\n+\t\tdie(\"Operation not supported by protocol.\");\n+\n+\tif (!_process_connect(transport, name, exec))\n+\t\tdie(\"Can't connect to subservice %s.\", name);\n+\n+\tfd[0] = data->helper->out;\n+\tfd[1] = data->helper->in;\n+\treturn 0;\n+}\n+\n static int fetch(struct transport *transport,\n \t\t int nr_heads, struct ref **to_fetch)\n {\n@@ -721,6 +739,7 @@ int transport_helper_init(struct transport *transport, const char *name)\n \ttransport->fetch = fetch;\n \ttransport->push_refs = push_refs;\n \ttransport->disconnect = release_helper;\n+\ttransport->connect = connect_helper;\n \ttransport->smart_options = &(data->gitoptions);\n \treturn 0;\n }\ndiff --git a/transport.c b/transport.c\nindex c4ecec4..64938fd 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -756,6 +756,17 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n \treturn ret;\n }\n \n+static int connect_git(struct transport *transport, const char *name,\n+\t\t const char *executable, int fd[2])\n+{\n+\tstruct git_transport_data *data = transport->data;\n+\tdata->conn = git_connect(data->fd, transport->url,\n+\t\t\t\t executable, 0);\n+\tfd[0] = data->fd[0];\n+\tfd[1] = data->fd[1];\n+\treturn 0;\n+}\n+\n static int disconnect_git(struct transport *transport)\n {\n \tstruct git_transport_data *data = transport->data;\n@@ -915,6 +926,7 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \t\tret->get_refs_list = get_refs_via_connect;\n \t\tret->fetch = fetch_refs_via_pack;\n \t\tret->push_refs = git_transport_push;\n+\t\tret->connect = connect_git;\n \t\tret->disconnect = disconnect_git;\n \t\tret->smart_options = &(data->options);\n \n@@ -1075,6 +1087,15 @@ void transport_unlock_pack(struct transport *transport)\n \t}\n }\n \n+int transport_connect(struct transport *transport, const char *name,\n+\t\t const char *exec, int fd[2])\n+{\n+\tif (transport->connect)\n+\t\treturn transport->connect(transport, name, exec, fd);\n+\telse\n+\t\tdie(\"Operation not supported by protocol\");\n+}\n+\n int transport_disconnect(struct transport *transport)\n {\n \tint ret = 0;\ndiff --git a/transport.h b/transport.h\nindex 781db2e..97ba251 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -64,6 +64,8 @@ struct transport {\n \t **/\n \tint (*push_refs)(struct transport *transport, struct ref *refs, int flags);\n \tint (*push)(struct transport *connection, int refspec_nr, const char **refspec, int flags);\n+\tint (*connect)(struct transport *connection, const char *name,\n+\t\t const char *executable, int fd[2]);\n \n \t/** get_refs_list(), fetch(), and push_refs() can keep\n \t * resources (such as a connection) reserved for futher\n@@ -133,6 +135,9 @@ char *transport_anonymize_url(const char *url);\n void transport_take_over(struct transport *transport,\n \t\t\t struct child_process *child);\n \n+int transport_connect(struct transport *transport, const char *name,\n+\t\t const char *exec, int fd[2]);\n+\n /* Transport methods defined outside transport.c */\n int transport_helper_init(struct transport *transport, const char *name);\n \n-- \n1.6.6.rc1.300.gfbc27\n"},{"id":"129529","messageId":"1260278177-9029-9-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"21901","inReplyTo":"1260278177-9029-1-git-send-email-ilari.liusvaara@elisanet.fi","subject":"[REROLL PATCH 8/8] Remove special casing of http, https and ftp","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-08T13:16:17Z","receivedAt":"2009-12-08T13:16:17Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"HTTP, HTTPS and FTP are no longer special to transport code. Also\nadd support for FTPS (curl supports it so it is easy).\n\nSigned-off-by: Ilari Liusvaara \nSigned-off-by: Junio C Hamano \nSigned-off-by: Ilari Liusvaara \n---\n .gitignore | 4 ++++\n Makefile | 24 ++++++++++++++++++++++--\n transport.c | 8 --------\n 3 files changed, 26 insertions(+), 10 deletions(-)\n\ndiff --git a/.gitignore b/.gitignore\nindex ac02a58..aa7a8ac 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -107,6 +107,10 @@\n /git-relink\n /git-remote\n /git-remote-curl\n+/git-remote-http\n+/git-remote-https\n+/git-remote-ftp\n+/git-remote-ftps\n /git-repack\n /git-replace\n /git-repo-config\ndiff --git a/Makefile b/Makefile\nindex 2ad7e36..546a408 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -424,6 +424,13 @@ BUILT_INS += git-stage$X\n BUILT_INS += git-status$X\n BUILT_INS += git-whatchanged$X\n \n+#ifdef NO_CURL\n+REMOTE_CURL_NAMES =\n+#else\n+# Yes, this is missing git-remote-http intentionally!\n+REMOTE_CURL_NAMES = git-remote-https git-remote-ftp git-remote-ftps\n+#endif\n+\n # what 'all' will build and 'install' will install in gitexecdir,\n # excluding programs for built-in commands\n ALL_PROGRAMS = $(PROGRAMS) $(SCRIPTS)\n@@ -1097,7 +1104,7 @@ else\n \telse\n \t\tCURL_LIBCURL = -lcurl\n \tendif\n-\tPROGRAMS += git-remote-curl$X git-http-fetch$X\n+\tPROGRAMS += git-remote-http$X git-remote-https$X git-remote-ftp$X git-remote-ftps$X git-http-fetch$X\n \tcurl_check := $(shell (echo 070908; curl-config --vernum) | sort -r | sed -ne 2p)\n \tifeq \"$(curl_check)\" \"070908\"\n \t\tifndef NO_EXPAT\n@@ -1676,7 +1683,13 @@ git-http-push$X: revision.o http.o http-push.o $(GITLIBS)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \\\n \t\t$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)\n \n-git-remote-curl$X: remote-curl.o http.o http-walker.o $(GITLIBS)\n+$(REMOTE_CURL_NAMES): git-remote-http$X\n+\t$(QUIET_LNCP)$(RM) $@ && \\\n+\tln $< $@ 2>/dev/null || \\\n+\tln -s $< $@ 2>/dev/null || \\\n+\tcp $< $@\n+\n+git-remote-http$X: remote-curl.o http.o http-walker.o $(GITLIBS)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \\\n \t\t$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)\n \n@@ -1852,6 +1865,7 @@ endif\n ifneq (,$X)\n \t$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_PROGRAMS) $(BUILT_INS) git$X)), test '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p' -ef '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p$X' || $(RM) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p';)\n endif\n+\n \tbindir=$$(cd '$(DESTDIR_SQ)$(bindir_SQ)' && pwd) && \\\n \texecdir=$$(cd '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' && pwd) && \\\n \t{ test \"$$bindir/\" = \"$$execdir/\" || \\\n@@ -1865,6 +1879,12 @@ endif\n \t\tln -s \"git$X\" \"$$execdir/$$p\" 2>/dev/null || \\\n \t\tcp \"$$execdir/git$X\" \"$$execdir/$$p\" || exit; \\\n \t done; } && \\\n+\t{ for p in $(REMOTE_CURL_NAMES); do \\\n+\t\t$(RM) \"$$execdir/$$p\" && \\\n+\t\tln \"$$execdir/git-remote-http$X\" \"$$execdir/$$p\" 2>/dev/null || \\\n+\t\tln -s \"git-remote-http$X\" \"$$execdir/$$p\" 2>/dev/null || \\\n+\t\tcp \"$$execdir/git-remote-http$X\" \"$$execdir/$$p\" || exit; \\\n+\t done; } && \\\n \t./check_bindir \"z$$bindir\" \"z$$execdir\" \"$$bindir/git-add$X\"\n \n install-doc:\ndiff --git a/transport.c b/transport.c\nindex 64938fd..211114e 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -932,14 +932,6 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \n \t\tdata->conn = NULL;\n \t\tdata->got_remote_heads = 0;\n-\t} else if (!prefixcmp(url, \"http://\")\n-\t\t|| !prefixcmp(url, \"https://\")\n-\t\t|| !prefixcmp(url, \"ftp://\")) {\n-\t\t/* These three are just plain special. */\n-\t\ttransport_helper_init(ret, \"curl\");\n-#ifdef NO_CURL\n-\t\terror(\"git was compiled without libcurl support.\");\n-#endif\n \t} else {\n \t\t/* Unknown protocol in URL. Pass to external handler. */\n \t\tint len = external_specification_len(url);\n-- \n1.6.6.rc1.300.gfbc27\n"},{"id":"129585","messageId":"7v7hsx6nwy.fsf@alter.siamese.dyndns.org","threadId":"21901","inReplyTo":"1260278177-9029-2-git-send-email-ilari.liusvaara@elisanet.fi","subject":"Re: [REROLL PATCH 1/8] Add remote helper debug mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-08T23:34:05Z","receivedAt":"2009-12-08T23:34:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ilari Liusvaara writes:\n\n> Remote helpers deadlock easily, so support debug mode which shows the\n> interaction steps.\n\nAlso new helper functions, sendline(), recvline(), write_constant() and\nxchgline() introduces abstraction that makes the whole thing a lot nicer\nto read ;-).\n\nNice.\n"},{"id":"129586","messageId":"7vzl5t59bp.fsf@alter.siamese.dyndns.org","threadId":"21901","inReplyTo":"1260278177-9029-3-git-send-email-ilari.liusvaara@elisanet.fi","subject":"Re: [REROLL PATCH 2/8] Support mandatory capabilities","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-08T23:34:34Z","receivedAt":"2009-12-08T23:34:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ilari Liusvaara writes:\n\n> diff --git a/transport-helper.c b/transport-helper.c\n> index a721dc2..f977d28 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -93,25 +93,39 @@ static struct child_process *get_helper(struct transport *transport)\n> \n> \tdata->out = xfdopen(helper->out, \"r\");\n> \twhile (1) {\n> +\t\tconst char *capname;\n> +\t\tint mandatory = 0;\n> \t\trecvline(data, &buf);\n> ...\n> +\t\t} else if (mandatory) {\n> +\t\t\tfflush(stderr);\n> +\t\t\tdie(\"Unknown madatory capability %s. This remote \"\n> +\t\t\t \"helper probably needs newer version of Git.\\n\",\n> +\t\t\t capname);\n\nWhy fflush() here? Is the reason for needing to flush stderr before\nletting die() to write into it very specific to this codepath, or shared\namong other callers of die()? I am wondering if we should add this\nfflush() to report() in usage.c instead.\n"},{"id":"129587","messageId":"7vskbl59ai.fsf@alter.siamese.dyndns.org","threadId":"21901","inReplyTo":"1260278177-9029-4-git-send-email-ilari.liusvaara@elisanet.fi","subject":"Re: [REROLL PATCH 3/8] Pass unknown protocols to external protocol handlers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-08T23:35:17Z","receivedAt":"2009-12-08T23:35:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ilari Liusvaara writes:\n\n> +const char *remove_ext_force(const char *url)\n> +{\n> +\tconst char *url2 = url;\n> +\tconst char *first_colon = NULL;\n> +\n> +\tif (!url)\n> +\t\treturn NULL;\n> +\n> +\twhile (*url2 && !first_colon)\n> +\t\tif (*url2 == ':')\n> +\t\t\tfirst_colon = url2;\n> +\t\telse\n> +\t\t\turl2++;\n> +\n> +\tif (first_colon && first_colon[1] == ':')\n> +\t\treturn first_colon + 2;\n> +\telse\n> +\t\treturn url;\n> +}\n\nSorry, I am slow today, but is this any different from:\n\n\tif (url) {\n\t\tconst char *colon = strchr(url, ':');\n\t if (colon && colon[1] == ':')\n\t \treturn url2 + 2;\n\t}\n\treturn url;\n\n> diff --git a/transport.c b/transport.c\n> index 3eea836..e42a82b 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -780,6 +780,58 @@ static int is_file(const char *url)\n> \treturn S_ISREG(buf.st_mode);\n> }\n> \n> +static const char *strchrc(const char *str, int c)\n> +{\n> +\twhile (*str)\n> +\t\tif (*str == c)\n> +\t\t\treturn str;\n> +\t\telse\n> +\t\t\tstr++;\n> +\treturn NULL;\n> +}\n\nI cannot spot how this is different from strchr()...\n\n> +static int is_url(const char *url)\n> +{\n> +\tconst char *url2, *first_slash;\n> +\n> +\tif (!url)\n> +\t\treturn 0;\n> +\turl2 = url;\n> +\tfirst_slash = strchrc(url, '/');\n> +\n> +\t/* Input with no slash at all or slash first can't be URL. */\n> +\tif (!first_slash || first_slash == url)\n> +\t\treturn 0;\n> +\t/* Character before must be : and next must be /. */\n> +\tif (first_slash[-1] != ':' || first_slash[1] != '/')\n> +\t\treturn 0;\n> +\t/* There must be something before the :// */\n> +\tif (first_slash == url + 1)\n> +\t\treturn 0;\n> +\t/*\n> +\t * Check all characters up to first slash. Only alpha, num and\n> +\t * colon (\":\") are allowed. \":\" must be followed by \":\" or \"/\".\n> +\t */\n\nHmm, so \"a::::bc:://\" is ok?\n\nIs the reason this does not to check the string up to (first_slash-1) for\na valid syntax for (as in \"://\") because this potentially\nhas \"::\" in front of it?\n\nI cannot tell if you want to allow \"::::://...\"\nby reading this code.\n\n> +static int external_specification_len(const char *url)\n> +{\n> +\treturn strchrc(url, ':') - url;\n> +}\n\nDoes the caller guarantee url has at least one colon in it?\n\n> struct transport *transport_get(struct remote *remote, const char *url)\n> {\n> \tstruct transport *ret = xcalloc(1, sizeof(*ret));\n> @@ -805,30 +857,23 @@ struct transport *transport_get(struct remote *remote, const char *url)\n> \n> \tif (remote && remote->foreign_vcs) {\n> \t\ttransport_helper_init(ret, remote->foreign_vcs);\n> +\t} else if (!prefixcmp(url, \"rsync:\")) {\n> \t\tret->get_refs_list = get_refs_via_rsync;\n> \t\tret->fetch = fetch_objs_via_rsync;\n> \t\tret->push = rsync_transport_push;\n> \t} else if (is_local(url) && is_file(url)) {\n> \t\tstruct bundle_transport_data *data = xcalloc(1, sizeof(*data));\n> \t\tret->data = data;\n> \t\tret->get_refs_list = get_refs_from_bundle;\n> \t\tret->fetch = fetch_refs_from_bundle;\n> \t\tret->disconnect = close_bundle;\n> +\t} else if (!is_url(url)\n> +\t\t|| !prefixcmp(url, \"file://\")\n> +\t\t|| !prefixcmp(url, \"git://\")\n> +\t\t|| !prefixcmp(url, \"ssh://\")\n> +\t\t|| !prefixcmp(url, \"git+ssh://\")\n> +\t\t|| !prefixcmp(url, \"ssh+git://\")) {\n> +\t\t/* These are builtin smart transports. */\n\nHmm, what is !is_url(url) at the beginning for, if this lists \"builtin\"\nsmart transports?\n\n> @@ -845,6 +890,21 @@ struct transport *transport_get(struct remote *remote, const char *url)\n> \t\tdata->receivepack = \"git-receive-pack\";\n> \t\tif (remote->receivepack)\n> \t\t\tdata->receivepack = remote->receivepack;\n> +\t} else if (!prefixcmp(url, \"http://\")\n> +\t\t|| !prefixcmp(url, \"https://\")\n> +\t\t|| !prefixcmp(url, \"ftp://\")) {\n> +\t\t/* These three are just plain special. */\n> +\t\ttransport_helper_init(ret, \"curl\");\n> +#ifdef NO_CURL\n> +\t\terror(\"git was compiled without libcurl support.\");\n> +#endif\n> +\t} else {\n> +\t\t/* Unknown protocol in URL. Pass to external handler. */\n> +\t\tint len = external_specification_len(url);\n> +\t\tchar *handler = xmalloc(len + 1);\n> +\t\thandler[len] = 0;\n> +\t\tstrncpy(handler, url, len);\n> +\t\ttransport_helper_init(ret, handler);\n\nHmph, external_specification_len() may get passed a colon-less string\nafter all, I think.\n"},{"id":"129588","messageId":"7vljhd597h.fsf@alter.siamese.dyndns.org","threadId":"21901","inReplyTo":"1260278177-9029-6-git-send-email-ilari.liusvaara@elisanet.fi","subject":"Re: [REROLL PATCH 5/8] Support taking over transports","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-08T23:37:06Z","receivedAt":"2009-12-08T23:37:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ilari Liusvaara writes:\n\n> diff --git a/transport-helper.c b/transport-helper.c\n> index 0e82553..3b7340c 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -22,6 +22,7 @@ struct helper_data\n> \t/* These go from remote name (as in \"list\") to private name */\n> \tstruct refspec *refspecs;\n> \tint refspec_nr;\n> +\tstruct git_transport_options gitoptions;\n> };\n\nWill we ever have another 'xxxoptions' field in this structure that is not\nso git-ish? 'gitoptions' somehow doesn't feel right, unless you plan to\nlater add scm specific options like 'hgoptions', 'bzroptions' in this\nfield you need to distinguish \"git\" one from them.\n\nI know you needed to name the new field to store the transport option\nunder a different name from the existing 'option' field, but for that\npurpose, 'transport_options' might be a more appropriate name.\n\n> @@ -109,9 +111,19 @@ static struct child_process *get_helper(struct transport *transport)\n> \t\tdie(\"Unable to run helper: git %s\", helper->argv[0]);\n> \tdata->helper = helper;\n> \n> +\t/*\n> +\t * Open the output as FILE* so strbuf_getline() can be used.\n> +\t * Do this with duped fd because fclose() will close the fd,\n> +\t * and stuff like taking over will require the fd to remain.\n> +\t *\n> +\t */\n> +\tduped = dup(helper->out);\n> +\tif (duped < 0)\n> +\t\tdie_errno(\"Can't dup helper output fd\");\n> +\tdata->out = xfdopen(duped, \"r\");\n> +\n\nNeat hack (the kind I often love), but it makes something deep inside me\ncringe. This looks fragile and possibly wrong.\n\nWho guarantees that reading from the (FILE *)data->out by strbuf_getline()\nthat eventually calls into fgetc() does not cause more data be read in the\nbuffer assiciated with the (FILE *) than we will consume? The other FILE*\nyou will make out of helper->out won't be able to read what data->out has\nalready slurped in to its stdio buffer.\n\nThe call sequence, after [6/8] is applied as I understand it is:\n\n - _process_connect()\n - get_helper()\n - start_command() that gives a pipe to read from the helper in\n helper->out;\n - the above dup dance makes (FILE *)data->out out of a copied fd;\n - read from data->out, potentially reading a lot more than\n the loop consumes into the stdio buffer of data->out;\n - dup dance again to make (FILE *)input;\n - read from input, unbuffered.\n\nBut if \"capabilities\" exchange has read past the capability response from\nthe helper into helper->out inside get_helper(), wouldn't it make the dup\ndance to make an extra \"input\" to read the rest unbuffered moot, as\n\"input\" won't be even reading from the beginning of \"the rest\"?\n"},{"id":"129589","messageId":"7vein5594u.fsf@alter.siamese.dyndns.org","threadId":"21901","inReplyTo":"1260278177-9029-7-git-send-email-ilari.liusvaara@elisanet.fi","subject":"Re: [REROLL PATCH 6/8] Support remote helpers implementing smart transports","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-08T23:38:41Z","receivedAt":"2009-12-08T23:38:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ilari Liusvaara writes:\n\n> Signed-off-by: Ilari Liusvaara \n> Signed-off-by: Junio C Hamano \n> Signed-off-by: Ilari Liusvaara \n> ---\n> Documentation/git-remote-helpers.txt | 25 +++++++-\n> transport-helper.c | 126 ++++++++++++++++++++++++++++++++--\n> 2 files changed, 144 insertions(+), 7 deletions(-)\n>\n> diff --git a/Documentation/git-remote-helpers.txt b/Documentation/git-remote-helpers.txt\n> index 20a05fe..b957813 100644\n> --- a/Documentation/git-remote-helpers.txt\n> +++ b/Documentation/git-remote-helpers.txt\n> @@ -93,6 +93,20 @@ Supported if the helper has the \"push\" capability.\n> +\n> Supported if the helper has the \"import\" capability.\n> \n> +'connect' ::\n> +\tConnects to given service. Stdin and stdout of helper are\n\nA minor point, but in prose, unless it explains how to use \"stdin\" and\n\"stdout\" as a variable, keyword, etc. in code, I'd prefer to see these\nspelled out, like so:\n\n\tThe standard input and output of helpers are connected to ...\n\n> @@ -34,12 +36,12 @@ static void sendline(struct helper_data *helper, struct strbuf *buffer)\n> \t\tdie_errno(\"Full write to remote helper failed\");\n> }\n> \n> -static int recvline(struct helper_data *helper, struct strbuf *buffer)\n> +static int _recvline(FILE *helper, struct strbuf *buffer)\n> {\n\nNot a good naming for two reasons.\n\n - Somebody new to the code wouldn't be able to tell which of recvline()\n and _recvline() take helper_data and FILE easily.\n\n - A function name that starts with an underscore, even if it is file\n scoped static, is something we tend to avoid. This applies to\n _process_connect() in the earlier patch as well.\n\nrecvline_fh() vs revline() might be better as most of the interaction in\nthis layer are done on helper_data, which makes the name recvline() pair\nnicely with sendline that also takes helper_data; and the oddball one that\nis _not_ an implementation detail (i.e. you have calls outside recvline()\nimplementation that call _recvline()) hints that it takes filehandle\ninstead.\n"},{"id":"129590","messageId":"7v7hsx5935.fsf@alter.siamese.dyndns.org","threadId":"21901","inReplyTo":"1260278177-9029-8-git-send-email-ilari.liusvaara@elisanet.fi","subject":"Re: [REROLL PATCH 7/8] Support remote archive from external protocol helpers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-08T23:39:42Z","receivedAt":"2009-12-08T23:39:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ilari Liusvaara writes:\n\n> Helpers which support connect also should support remote archive\n> snapshot (or at least there's only one way to attempt it). So support\n> remote snapshotting for protocol helpers.\n\nSorry, but I cannot parse this \"should\", and cannot understand how the\nfirst sentence leads to the conclusion (\"So\") that we must have this\npatch.\n\n\"They should support it when able, because doing so gives them _this nice\nproperty_ (that is unspecified)\"???\n\nOr \"They must support it because we are changing the code not to work\nwithout\"???\n\nOr \"Because the transport layer has been restructured cleanly enough to\nallow passing general payload, there is no reason not to do this change to\npass 'archive' output in addition to the 'git smart fetch/push protocol'\npayload, and this allows the archive command to take advantage of the\nhelper based transports\"???\n"},{"id":"129613","messageId":"36ca99e90912090055s32d5ebc5p917dd7e528218f68@mail.gmail.com","threadId":"21901","inReplyTo":"7vskbl59ai.fsf@alter.siamese.dyndns.org","subject":"Re: [REROLL PATCH 3/8] Pass unknown protocols to external protocol handlers","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2009-12-09T08:55:01Z","receivedAt":"2009-12-09T08:55:01Z","isPatch":true,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"On Wed, Dec 9, 2009 at 00:35, Junio C Hamano wrote:\n> Ilari Liusvaara writes:\n>> +     } else if (!is_url(url)\n>> +             || !prefixcmp(url, \"file://\")\n>> +             || !prefixcmp(url, \"git://\")\n>> +             || !prefixcmp(url, \"ssh://\")\n>> +             || !prefixcmp(url, \"git+ssh://\")\n>> +             || !prefixcmp(url, \"ssh+git://\")) {\n>> +             /* These are builtin smart transports. */\n>\n> Hmm, what is !is_url(url) at the beginning for, if this lists \"builtin\"\n> smart transports?\nI think/hope this should catch theses use cases:\n\n ? [user@]host.xz:/path/to/repo.git/\n ? [user@]host.xz:~user/path/to/repo.git/\n ? [user@]host.xz:path/to/repo.git\n"},{"id":"129638","messageId":"20091209151230.GA15673@Knoppix","threadId":"21901","inReplyTo":"7vzl5t59bp.fsf@alter.siamese.dyndns.org","subject":"Re: [REROLL PATCH 2/8] Support mandatory capabilities","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-09T15:12:30Z","receivedAt":"2009-12-09T15:12:30Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Tue, Dec 08, 2009 at 03:34:34PM -0800, Junio C Hamano wrote:\n> Ilari Liusvaara writes:\n\n> > +\t\t\tfflush(stderr);\n> > +\t\t\tdie(\"Unknown madatory capability %s. This remote \"\n> > +\t\t\t \"helper probably needs newer version of Git.\\n\",\n> > +\t\t\t capname);\n> \n> Why fflush() here? Is the reason for needing to flush stderr before\n> letting die() to write into it very specific to this codepath, or shared\n> among other callers of die()? I am wondering if we should add this\n> fflush() to report() in usage.c instead.\n\nNo idea why its there (anymore). Die will flush stderr anyway via exit.\nRemoved.\n\n-Ilari\n"},{"id":"129639","messageId":"20091209151549.GB15673@Knoppix","threadId":"21901","inReplyTo":"7vskbl59ai.fsf@alter.siamese.dyndns.org","subject":"Re: [REROLL PATCH 3/8] Pass unknown protocols to external protocol handlers","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-09T15:15:50Z","receivedAt":"2009-12-09T15:15:50Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Tue, Dec 08, 2009 at 03:35:17PM -0800, Junio C Hamano wrote:\n> Ilari Liusvaara writes:\n> \n> > +const char *remove_ext_force(const char *url)\n> > +{\n> > +\tconst char *url2 = url;\n> > +\tconst char *first_colon = NULL;\n> > +\n> > +\tif (!url)\n> > +\t\treturn NULL;\n> > +\n> > +\twhile (*url2 && !first_colon)\n> > +\t\tif (*url2 == ':')\n> > +\t\t\tfirst_colon = url2;\n> > +\t\telse\n> > +\t\t\turl2++;\n> > +\n> > +\tif (first_colon && first_colon[1] == ':')\n> > +\t\treturn first_colon + 2;\n> > +\telse\n> > +\t\treturn url;\n> > +}\n> \n> Sorry, I am slow today, but is this any different from:\n> \n> \tif (url) {\n> \t\tconst char *colon = strchr(url, ':');\n> \t if (colon && colon[1] == ':')\n> \t \treturn url2 + 2;\n> \t}\n> \treturn url;\n\nUndefined variable url2 (you mean colon?). Changed.\n \n> > diff --git a/transport.c b/transport.c\n> > index 3eea836..e42a82b 100644\n> > --- a/transport.c\n> > +++ b/transport.c\n> > @@ -780,6 +780,58 @@ static int is_file(const char *url)\n> > \treturn S_ISREG(buf.st_mode);\n> > }\n> > \n> > +static const char *strchrc(const char *str, int c)\n> > +{\n> > +\twhile (*str)\n> > +\t\tif (*str == c)\n> > +\t\t\treturn str;\n> > +\t\telse\n> > +\t\t\tstr++;\n> > +\treturn NULL;\n> > +}\n> \n> I cannot spot how this is different from strchr()...\n\nReturn type. Not that useful. Removed.\n \n> > +static int is_url(const char *url)\n> > +{\n> > +\tconst char *url2, *first_slash;\n> > +\n> > +\tif (!url)\n> > +\t\treturn 0;\n> > +\turl2 = url;\n> > +\tfirst_slash = strchrc(url, '/');\n> > +\n> > +\t/* Input with no slash at all or slash first can't be URL. */\n> > +\tif (!first_slash || first_slash == url)\n> > +\t\treturn 0;\n> > +\t/* Character before must be : and next must be /. */\n> > +\tif (first_slash[-1] != ':' || first_slash[1] != '/')\n> > +\t\treturn 0;\n> > +\t/* There must be something before the :// */\n> > +\tif (first_slash == url + 1)\n> > +\t\treturn 0;\n> > +\t/*\n> > +\t * Check all characters up to first slash. Only alpha, num and\n> > +\t * colon (\":\") are allowed. \":\" must be followed by \":\" or \"/\".\n> > +\t */\n\nI tightened this a bit, only checking (exclusive) character before first\n\"/\" and requiring all-alphanum. The previous rules were apparently\nleftover from times the \"specify remote helper by url\" patch didn't\nexist.\n\nConsidering context at call site, it is equivalent, since any ':' must be\nfollowed by '/' or is_url will never be called, and first ':/' must be\nexactly at last_slash - 1 (by earlier checks).\n \n> Hmm, so \"a::::bc:://\" is ok?\n\nis_url never gets such thing. Due to \"::\" it bypasses URL validation. And\nyes, it is ok.\n\n> Is the reason this does not to check the string up to (first_slash-1) for\n> a valid syntax for (as in \"://\") because this potentially\n> has \"::\" in front of it?\n\nIt doesn't have '::' in front of it.\n\n> I cannot tell if you want to allow \"::::://...\"\n> by reading this code.\n\nAllowing or denying that is not up to this function. And one can't chain\nhelpers that way. It invokes \n \n> > +static int external_specification_len(const char *url)\n> > +{\n> > +\treturn strchrc(url, ':') - url;\n> > +}\n> \n> Does the caller guarantee url has at least one colon in it?\n\nAnything passing is_url() does have ':' in it.\n\n> > +\t} else if (!is_url(url)\n> > +\t\t|| !prefixcmp(url, \"file://\")\n> > +\t\t|| !prefixcmp(url, \"git://\")\n> > +\t\t|| !prefixcmp(url, \"ssh://\")\n> > +\t\t|| !prefixcmp(url, \"git+ssh://\")\n> > +\t\t|| !prefixcmp(url, \"ssh+git://\")) {\n> > +\t\t/* These are builtin smart transports. */\n> \n> Hmm, what is !is_url(url) at the beginning for, if this lists \"builtin\"\n> smart transports?\n\nIt catches the scp and path syntaxes as not URLs.\n \n> > +\t} else {\n> > +\t\t/* Unknown protocol in URL. Pass to external handler. */\n> > +\t\tint len = external_specification_len(url);\n> > +\t\tchar *handler = xmalloc(len + 1);\n> > +\t\thandler[len] = 0;\n> > +\t\tstrncpy(handler, url, len);\n> > +\t\ttransport_helper_init(ret, handler);\n> \n> Hmph, external_specification_len() may get passed a colon-less string\n> after all, I think.\n\nNope, it can't. Else if above snarfs everything that doesn't pass\nis_url, and string can't pass it without having ':' in it. \n\n-Ilari\n"},{"id":"129640","messageId":"20091209151630.GC15673@Knoppix","threadId":"21901","inReplyTo":"7vein5594u.fsf@alter.siamese.dyndns.org","subject":"Re: [REROLL PATCH 6/8] Support remote helpers implementing smart transports","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-09T15:16:30Z","receivedAt":"2009-12-09T15:16:30Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Tue, Dec 08, 2009 at 03:38:41PM -0800, Junio C Hamano wrote:\n> Ilari Liusvaara writes:\n> > \n> > +'connect' ::\n> > +\tConnects to given service. Stdin and stdout of helper are\n> \n> A minor point, but in prose, unless it explains how to use \"stdin\" and\n> \"stdout\" as a variable, keyword, etc. in code, I'd prefer to see these\n> spelled out, like so:\n> \n> \tThe standard input and output of helpers are connected to ...\n\nChanged.\n\n> > -static int recvline(struct helper_data *helper, struct strbuf *buffer)\n> > +static int _recvline(FILE *helper, struct strbuf *buffer)\n> > {\n> \n> recvline_fh() vs revline() might be better as most of the interaction in\n> this layer are done on helper_data, which makes the name recvline() pair\n> nicely with sendline that also takes helper_data; and the oddball one that\n> is _not_ an implementation detail (i.e. you have calls outside recvline()\n> implementation that call _recvline()) hints that it takes filehandle\n> instead.\n\n- _recvline -> recvline_fh\n- _process_connect -> process_connect_service\n\n-Ilari\n"},{"id":"129641","messageId":"20091209151658.GD15673@Knoppix","threadId":"21901","inReplyTo":"7v7hsx5935.fsf@alter.siamese.dyndns.org","subject":"Re: [REROLL PATCH 7/8] Support remote archive from external protocol helpers","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-09T15:16:58Z","receivedAt":"2009-12-09T15:16:58Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Tue, Dec 08, 2009 at 03:39:42PM -0800, Junio C Hamano wrote:\n> Ilari Liusvaara writes:\n> \n> > Helpers which support connect also should support remote archive\n> > snapshot (or at least there's only one way to attempt it). So support\n> > remote snapshotting for protocol helpers.\n> \n> Or \"Because the transport layer has been restructured cleanly enough to\n> allow passing general payload, there is no reason not to do this change to\n> pass 'archive' output in addition to the 'git smart fetch/push protocol'\n> payload, and this allows the archive command to take advantage of the\n> helper based transports\"???\n\nThis one is the correct interpretation.\n\nChanged to:\n-----------\nSupport remote archive from all smart transports\n\nPreviously, remote archive required internal (non remote-helper)\nsmart transport. Extend the remote archive to also support smart\ntransports implemented by remote helpers.\n------------\n\n-Ilari\n"},{"id":"129642","messageId":"20091209151718.GE15673@Knoppix","threadId":"21901","inReplyTo":"7vljhd597h.fsf@alter.siamese.dyndns.org","subject":"Re: [REROLL PATCH 5/8] Support taking over transports","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-09T15:17:18Z","receivedAt":"2009-12-09T15:17:18Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Tue, Dec 08, 2009 at 03:37:06PM -0800, Junio C Hamano wrote:\n> Ilari Liusvaara writes:\n> \n> Will we ever have another 'xxxoptions' field in this structure that is not\n> so git-ish? 'gitoptions' somehow doesn't feel right, unless you plan to\n> later add scm specific options like 'hgoptions', 'bzroptions' in this\n> field you need to distinguish \"git\" one from them.\n> \n> I know you needed to name the new field to store the transport option\n> under a different name from the existing 'option' field, but for that\n> purpose, 'transport_options' might be a more appropriate name.\n\nChanged.\n \n> > @@ -109,9 +111,19 @@ static struct child_process *get_helper(struct transport *transport)\n> > \t\tdie(\"Unable to run helper: git %s\", helper->argv[0]);\n> > \tdata->helper = helper;\n> > \n> > +\t/*\n> > +\t * Open the output as FILE* so strbuf_getline() can be used.\n> > +\t * Do this with duped fd because fclose() will close the fd,\n> > +\t * and stuff like taking over will require the fd to remain.\n> > +\t *\n> > +\t */\n> > +\tduped = dup(helper->out);\n> > +\tif (duped < 0)\n> > +\t\tdie_errno(\"Can't dup helper output fd\");\n> > +\tdata->out = xfdopen(duped, \"r\");\n> > +\n> \n> Neat hack (the kind I often love), but it makes something deep inside me\n> cringe. This looks fragile and possibly wrong.\n> \n> Who guarantees that reading from the (FILE *)data->out by strbuf_getline()\n> that eventually calls into fgetc() does not cause more data be read in the\n> buffer assiciated with the (FILE *) than we will consume? The other FILE*\n> you will make out of helper->out won't be able to read what data->out has\n> already slurped in to its stdio buffer.\n\nCausality impiles this can happen only if buffered version gets its buffer\nfilled after sending connect command. And looking at stdio operations that\ncan occur after sending the command:\n\n- fprintfs on stderr (debug mode only).\n- fgetc()s on unbuffered version.\n\n-Ilari\n"},{"id":"129662","messageId":"7vein3onxf.fsf@alter.siamese.dyndns.org","threadId":"21901","inReplyTo":"20091209151718.GE15673@Knoppix","subject":"Re: [REROLL PATCH 5/8] Support taking over transports","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-09T21:08:44Z","receivedAt":"2009-12-09T21:08:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ilari Liusvaara writes:\n\n>> Who guarantees that reading from the (FILE *)data->out by strbuf_getline()\n>> that eventually calls into fgetc() does not cause more data be read in the\n>> buffer assiciated with the (FILE *) than we will consume? The other FILE*\n>> you will make out of helper->out won't be able to read what data->out has\n>> already slurped in to its stdio buffer.\n>\n> Causality impiles this can happen only if buffered version gets its buffer\n> filled after sending connect command. And looking at stdio operations that\n> can occur after sending the command:\n>\n> - fprintfs on stderr (debug mode only).\n> - fgetc()s on unbuffered version.\n\nI was worried about the \"capabilities\" loop in get_helper() if it reads\ntoo much from data->out (causing helper->out to lose the beginning of what\nit should read), but as you said, by \"Causality\" it cannot happen. The\nprotocol during that phase works with the other end in lock-step, and the\nother end wouldn't have written to the pipe what we shouldn't read in that\nloop (e.g. remote-curl.c responds with \"capabilities\" with lines and ends\nwith fflush(), and after that it waits for us to talk to it --- it would\nnever start talking by itself without first being asked to talk).\n\nIs data->out only used inside get_helper() to read the capabilities\nresponse? I was confused by it being keft open until disconnect time.\n"},{"id":"129663","messageId":"20091209214216.GA17474@Knoppix","threadId":"21901","inReplyTo":"7vein3onxf.fsf@alter.siamese.dyndns.org","subject":"Re: [REROLL PATCH 5/8] Support taking over transports","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-09T21:42:16Z","receivedAt":"2009-12-09T21:42:16Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Wed, Dec 09, 2009 at 01:08:44PM -0800, Junio C Hamano wrote:\n> \n> Is data->out only used inside get_helper() to read the capabilities\n> response? I was confused by it being keft open until disconnect time.\n \nIts used for all incoming communications from remote helper until connect\nrequest (and even after it if falling back to dumb transport).\n\n- In smart transport case, that includes possible servpath option.\n- In non-smart case, it includes a lot more.\n\nIn fact, recvline uses exactly helper->out as stream to read.\n\n-Ilari\n"}]}