{"thread":{"id":"9860","subject":"[PATCH 5/5] Remove unnecessary 'fetch' argument from transport_get API","startedAt":"2007-09-15T07:23:14Z","lastAt":"2007-09-16T08:11:29Z","messageCount":2,"participants":["Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"53096","messageId":"20070915072314.GE20346@spearce.org","threadId":"9860","inReplyTo":null,"subject":"[PATCH 5/5] Remove unnecessary 'fetch' argument from transport_get API","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-09-15T07:23:14Z","receivedAt":"2007-09-15T07:23:14Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"We don't actually need to know at the time of transport_get if the\ncaller wants to fetch, push, or do both on the returned object.\nIt is easier to just delay the initialization of the HTTP walker\nuntil we know we will need it by providing a CURL specific fetch\nfunction in the curl_transport that makes sure the walker instance\nis initialized before use.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n builtin-fetch.c |    2 +-\n builtin-push.c  |    2 +-\n transport.c     |   23 +++++++++++++++++------\n transport.h     |    4 +---\n 4 files changed, 20 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin-fetch.c b/builtin-fetch.c\nindex 8b0fdbe..300d563 100644\n--- a/builtin-fetch.c\n+++ b/builtin-fetch.c\n@@ -496,7 +496,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \telse\n \t\tremote = remote_get(argv[i++]);\n \n-\ttransport = transport_get(remote, remote->uri[0], 1);\n+\ttransport = transport_get(remote, remote->uri[0]);\n \tif (verbose >= 2)\n \t\ttransport->verbose = 1;\n \tif (quiet)\ndiff --git a/builtin-push.c b/builtin-push.c\nindex f496b46..7d7e826 100644\n--- a/builtin-push.c\n+++ b/builtin-push.c\n@@ -59,7 +59,7 @@ static int do_push(const char *repo, int flags)\n \terrs = 0;\n \tfor (i = 0; i < remote->uri_nr; i++) {\n \t\tstruct transport *transport =\n-\t\t\ttransport_get(remote, remote->uri[i], 0);\n+\t\t\ttransport_get(remote, remote->uri[i]);\n \t\tint err;\n \t\tif (receivepack)\n \t\t\ttransport_set_option(transport,\ndiff --git a/transport.c b/transport.c\nindex 5eabe8d..7f94d30 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -174,6 +174,14 @@ static struct ref *get_refs_via_curl(const struct transport *transport)\n \treturn refs;\n }\n \n+static int fetch_objs_via_curl(struct transport *transport,\n+\t\t\t\t int nr_objs, struct ref **to_fetch)\n+{\n+\tif (!transport->data)\n+\t\ttransport->data = get_http_walker(transport->url);\n+\treturn fetch_objs_via_walker(transport, nr_objs, to_fetch);\n+}\n+\n #else\n \n static struct ref *get_refs_via_curl(const struct transport *transport)\n@@ -182,12 +190,19 @@ static struct ref *get_refs_via_curl(const struct transport *transport)\n \treturn NULL;\n }\n \n+static int fetch_objs_via_curl(struct transport *transport,\n+\t\t\t\t int nr_objs, struct ref **to_fetch)\n+{\n+\tdie(\"Cannot fetch from '%s' without curl ...\", transport->url);\n+\treturn -1;\n+}\n+\n #endif\n \n static const struct transport_ops curl_transport = {\n \t/* set_option */\tNULL,\n \t/* get_refs_list */\tget_refs_via_curl,\n-\t/* fetch */\t\tfetch_objs_via_walker,\n+\t/* fetch */\t\tfetch_objs_via_curl,\n \t/* push */\t\tcurl_transport_push,\n \t/* disconnect */\tdisconnect_walker\n };\n@@ -408,14 +423,12 @@ static int is_file(const char *url)\n \treturn S_ISREG(buf.st_mode);\n }\n \n-struct transport *transport_get(struct remote *remote, const char *url,\n-\t\t\t\tint fetch)\n+struct transport *transport_get(struct remote *remote, const char *url)\n {\n \tstruct transport *ret = xcalloc(1, sizeof(*ret));\n \n \tret->remote = remote;\n \tret->url = url;\n-\tret->fetch = !!fetch;\n \n \tif (!prefixcmp(url, \"rsync://\")) {\n \t\tret->ops = &rsync_transport;\n@@ -423,8 +436,6 @@ struct transport *transport_get(struct remote *remote, const char *url,\n \t        || !prefixcmp(url, \"https://\")\n \t        || !prefixcmp(url, \"ftp://\")) {\n \t\tret->ops = &curl_transport;\n-\t\tif (fetch)\n-\t\t\tret->data = get_http_walker(url);\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;\ndiff --git a/transport.h b/transport.h\nindex f2bbdf7..6a95d66 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -6,7 +6,6 @@\n \n struct transport {\n \tunsigned verbose : 1;\n-\tunsigned fetch : 1;\n \tstruct remote *remote;\n \tconst char *url;\n \n@@ -38,8 +37,7 @@ struct transport_ops {\n };\n \n /* Returns a transport suitable for the url */\n-struct transport *transport_get(struct remote *remote, const char *url,\n-\t\t\t\tint fetch);\n+struct transport *transport_get(struct remote *, const char *);\n \n /* Transport options which apply to git:// and scp-style URLs */\n \n-- \n1.5.3.1.84.gaf82-dirty\n"},{"id":"53175","messageId":"20070916081129.GE3099@spearce.org","threadId":"9860","inReplyTo":"20070915072314.GE20346@spearce.org","subject":"Re: [PATCH 5/5] Remove unnecessary 'fetch' argument from transport_get API","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-09-16T08:11:29Z","receivedAt":"2007-09-16T08:11:29Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> wrote:\n> We don't actually need to know at the time of transport_get if the\n> caller wants to fetch, push, or do both on the returned object.\n> It is easier to just delay the initialization of the HTTP walker\n> until we know we will need it by providing a CURL specific fetch\n> function in the curl_transport that makes sure the walker instance\n> is initialized before use.\n\nDaniel privately emailed me his rationale for why this fetch argument\nwas here in the first place and I mostly agree with him, but will\nbe working up a more clear API replacement in the near future.\nThe \"1/0\",\"fetch/push\" thing is not the clearest way that we could\ndefine the API.\n\nRight now this patch is required to go along with my 1/5 bug fix,\nas without this patch we get the sequence:\n\n\thttp_init()\n\thttp_init()\n\t... use http ...\n\thttp_cleanup()\n\t... try to use http again and barf ...\n\nThis 5/5 makes the sequence be in proper order by delaying creation\nof the HTTP walker object (and thus one of the http_init() calls)\nto after the first http_cleanup(), so we get the nice neat order of:\n\n\thttp_init()\n\t... use http ...\n\thttp_cleanup()\n\thttp_init()\n\t... use http just fine ...\n\thttp_cleanup()\n\nI'll be honest here; I did not test 1/5 or 5/5 on their own.\nI only tested the combined result of them, and that creates a\nworking HTTP fetch.  But I'm pretty sure that one of these patches\nalone will still cause SIGSEGV/SIGBUS errors during HTTP fetch due\nto either the bad cleanup (1/5 fixes) or the bad init ordering I'm\ntalking about above (5/5 fixes).\n\nReinstating a replacement for the fetch parameter that I removed\nin this patch isn't critical for functionality, but will be\nnecessary to do performance optimization in the form of reusing\nthe connection between ref discovery and pack transfer in the\nnative protocol.  Right now I'm focusing on making builtin-fetch\nstable and implementing prior behavior.  Once its solid enough to\ngraduate to `next` we can start doing some of the optimization work.\n \n-- \nShawn.\n"}]}