{"thread":{"id":"20257","subject":"[PATCH 2/3 v2] Use an external program to implement fetching with curl","startedAt":"2009-07-28T06:08:51Z","lastAt":"2009-07-29T22:16:44Z","messageCount":6,"participants":["Daniel Barkalow","Johannes Schindelin","Erik Faye-Lund"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"118924","messageId":"alpine.LNX.2.00.0907280207350.2147@iabervon.org","threadId":"20257","inReplyTo":null,"subject":"[PATCH 2/3 v2] Use an external program to implement fetching with curl","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2009-07-28T06:08:51Z","receivedAt":"2009-07-28T06:08:51Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"Use the transport shim mechanism to fetch by http (and ftp, etc).\n\nSigned-off-by: Daniel Barkalow <barkalow@iabervon.org>\n---\n Makefile    |    5 ++\n shim-curl.c |  132 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n transport.c |  136 +----------------------------------------------------------\n 3 files changed, 138 insertions(+), 135 deletions(-)\n create mode 100644 shim-curl.c\n\ndiff --git a/Makefile b/Makefile\nindex 01efc73..d3dd2ed 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -980,6 +980,7 @@ else\n \t\tCURL_LIBCURL = -lcurl\n \tendif\n \tBUILTIN_OBJS += builtin-http-fetch.o\n+\tPROGRAMS += git-shim-curl$X\n \tEXTLIBS += $(CURL_LIBCURL)\n \tLIB_OBJS += http.o http-walker.o\n \tcurl_check := $(shell (echo 070908; curl-config --vernum) | sort -r | sed -ne 2p)\n@@ -1490,6 +1491,10 @@ 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-shim-curl$X: shim-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 $(LIB_OBJS) $(BUILTIN_OBJS): $(LIB_H)\n $(patsubst git-%$X,%.o,$(PROGRAMS)) git.o: $(LIB_H) $(wildcard */*.h)\n builtin-revert.o wt-status.o: wt-status.h\ndiff --git a/shim-curl.c b/shim-curl.c\nnew file mode 100644\nindex 0000000..bde041b\n--- /dev/null\n+++ b/shim-curl.c\n@@ -0,0 +1,132 @@\n+#include \"cache.h\"\n+#include \"remote.h\"\n+#include \"strbuf.h\"\n+#include \"walker.h\"\n+#include \"http.h\"\n+\n+static struct ref *get_refs(struct walker *walker, const char *url)\n+{\n+\tint http_ret;\n+\tchar *refs_url;\n+\tstruct strbuf buffer = STRBUF_INIT;\n+\tchar *data, *start, *mid;\n+\tint i = 0;\n+\tchar *ref_name;\n+\tstruct ref *refs = NULL;\n+\tstruct ref *ref = NULL;\n+\tstruct ref *last_ref = NULL;\n+\n+\trefs_url = xmalloc(strlen(url) + 11);\n+\tsprintf(refs_url, \"%s/info/refs\", url);\n+\n+\thttp_ret = http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE);\n+\tswitch (http_ret) {\n+\tcase HTTP_OK:\n+\t\tbreak;\n+\tcase HTTP_MISSING_TARGET:\n+\t\tdie(\"%s not found: did you run git update-server-info on the\"\n+\t\t    \" server?\", refs_url);\n+\tdefault:\n+\t\thttp_error(refs_url, http_ret);\n+\t\tdie(\"HTTP request failed\");\n+\t}\n+\tfree(refs_url);\n+\n+\tdata = buffer.buf;\n+\tstart = NULL;\n+\tmid = data;\n+\twhile (i < buffer.len) {\n+\t\tif (!start) {\n+\t\t\tstart = &data[i];\n+\t\t}\n+\t\tif (data[i] == '\\t')\n+\t\t\tmid = &data[i];\n+\t\tif (data[i] == '\\n') {\n+\t\t\tdata[i] = 0;\n+\t\t\tref_name = mid + 1;\n+\t\t\tref = xmalloc(sizeof(struct ref) +\n+\t\t\t\t      strlen(ref_name) + 1);\n+\t\t\tmemset(ref, 0, sizeof(struct ref));\n+\t\t\tstrcpy(ref->name, ref_name);\n+\t\t\tget_sha1_hex(start, ref->old_sha1);\n+\t\t\tif (!refs)\n+\t\t\t\trefs = ref;\n+\t\t\tif (last_ref)\n+\t\t\t\tlast_ref->next = ref;\n+\t\t\tlast_ref = ref;\n+\t\t\tstart = NULL;\n+\t\t}\n+\t\ti++;\n+\t}\n+\tstrbuf_release(&buffer);\n+\n+\tref = alloc_ref(\"HEAD\");\n+\tif (!walker->fetch_ref(walker, ref) &&\n+\t    !resolve_remote_symref(ref, refs)) {\n+\t\tref->next = refs;\n+\t\trefs = ref;\n+\t} else {\n+\t\tfree(ref);\n+\t}\n+\n+\treturn refs;\n+}\n+\n+int main(int argc, const char **argv)\n+{\n+\tstruct remote *remote;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tconst char *url;\n+\tstruct walker *walker = NULL;\n+\n+\tsetup_git_directory();\n+\tif (argc < 2) {\n+\t\tfprintf(stderr, \"Remote needed\\n\");\n+\t\treturn 1;\n+\t}\n+\n+\tremote = remote_get(argv[1]);\n+\n+\tif (argc > 2) {\n+\t\turl = argv[2];\n+\t} else {\n+\t\turl = remote->url[0];\n+\t}\n+\n+\tdo {\n+\t\tif (strbuf_getline(&buf, stdin, '\\n') == EOF)\n+\t\t\tbreak;\n+\t\tif (!prefixcmp(buf.buf, \"fetch \")) {\n+\t\t\tchar *obj = buf.buf + strlen(\"fetch \");\n+\t\t\tif (!walker)\n+\t\t\t\twalker = get_http_walker(url, remote);\n+\t\t\twalker->get_all = 1;\n+\t\t\twalker->get_tree = 1;\n+\t\t\twalker->get_history = 1;\n+\t\t\twalker->get_verbosely = 0;\n+\t\t\twalker->get_recover = 0;\n+\t\t\tif (walker_fetch(walker, 1, &obj, NULL, NULL))\n+\t\t\t\tdie(\"Fetch failed.\");\n+\t\t\tprintf(\"\\n\");\n+\t\t\tfflush(stdout);\n+\t\t} else if (!strcmp(buf.buf, \"list\")) {\n+\t\t\tstruct ref *refs;\n+\t\t\tstruct ref *posn;\n+\t\t\tif (!walker)\n+\t\t\t\twalker = get_http_walker(url, remote);\n+\t\t\trefs = get_refs(walker, url);\n+\t\t\tfor (posn = refs; posn; posn = posn->next) {\n+\t\t\t\tif (posn->symref)\n+\t\t\t\t\tprintf(\"@%s %s\\n\", posn->symref, posn->name);\n+\t\t\t\telse\n+\t\t\t\t\tprintf(\"%s %s\\n\", sha1_to_hex(posn->old_sha1), posn->name);\n+\t\t\t}\n+\t\t\tprintf(\"\\n\");\n+\t\t\tfflush(stdout);\n+\t\t} else {\n+\t\t\treturn 1;\n+\t\t}\n+\t\tstrbuf_reset(&buf);\n+\t} while (1);\n+\treturn 0;\n+}\ndiff --git a/transport.c b/transport.c\nindex de0d587..6d13b0e 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1,9 +1,6 @@\n #include \"cache.h\"\n #include \"transport.h\"\n #include \"run-command.h\"\n-#ifndef NO_CURL\n-#include \"http.h\"\n-#endif\n #include \"pkt-line.h\"\n #include \"fetch-pack.h\"\n #include \"send-pack.h\"\n@@ -352,45 +349,6 @@ static int rsync_transport_push(struct transport *transport,\n \treturn result;\n }\n \n-/* Generic functions for using commit walkers */\n-\n-#ifndef NO_CURL /* http fetch is the only user */\n-static int fetch_objs_via_walker(struct transport *transport,\n-\t\t\t\t int nr_objs, const struct ref **to_fetch)\n-{\n-\tchar *dest = xstrdup(transport->url);\n-\tstruct walker *walker = transport->data;\n-\tchar **objs = xmalloc(nr_objs * sizeof(*objs));\n-\tint i;\n-\n-\twalker->get_all = 1;\n-\twalker->get_tree = 1;\n-\twalker->get_history = 1;\n-\twalker->get_verbosely = transport->verbose >= 0;\n-\twalker->get_recover = 0;\n-\n-\tfor (i = 0; i < nr_objs; i++)\n-\t\tobjs[i] = xstrdup(sha1_to_hex(to_fetch[i]->old_sha1));\n-\n-\tif (walker_fetch(walker, nr_objs, objs, NULL, NULL))\n-\t\tdie(\"Fetch failed.\");\n-\n-\tfor (i = 0; i < nr_objs; i++)\n-\t\tfree(objs[i]);\n-\tfree(objs);\n-\tfree(dest);\n-\treturn 0;\n-}\n-#endif /* NO_CURL */\n-\n-static int disconnect_walker(struct transport *transport)\n-{\n-\tstruct walker *walker = transport->data;\n-\tif (walker)\n-\t\twalker_free(walker);\n-\treturn 0;\n-}\n-\n #ifndef NO_CURL\n static int curl_transport_push(struct transport *transport, int refspec_nr, const char **refspec, int flags)\n {\n@@ -432,96 +390,6 @@ static int curl_transport_push(struct transport *transport, int refspec_nr, cons\n \treturn !!err;\n }\n \n-static struct ref *get_refs_via_curl(struct transport *transport, int for_push)\n-{\n-\tstruct strbuf buffer = STRBUF_INIT;\n-\tchar *data, *start, *mid;\n-\tchar *ref_name;\n-\tchar *refs_url;\n-\tint i = 0;\n-\tint http_ret;\n-\n-\tstruct ref *refs = NULL;\n-\tstruct ref *ref = NULL;\n-\tstruct ref *last_ref = NULL;\n-\n-\tstruct walker *walker;\n-\n-\tif (for_push)\n-\t\treturn NULL;\n-\n-\tif (!transport->data)\n-\t\ttransport->data = get_http_walker(transport->url,\n-\t\t\t\t\t\ttransport->remote);\n-\n-\twalker = transport->data;\n-\n-\trefs_url = xmalloc(strlen(transport->url) + 11);\n-\tsprintf(refs_url, \"%s/info/refs\", transport->url);\n-\n-\thttp_ret = http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE);\n-\tswitch (http_ret) {\n-\tcase HTTP_OK:\n-\t\tbreak;\n-\tcase HTTP_MISSING_TARGET:\n-\t\tdie(\"%s not found: did you run git update-server-info on the\"\n-\t\t    \" server?\", refs_url);\n-\tdefault:\n-\t\thttp_error(refs_url, http_ret);\n-\t\tdie(\"HTTP request failed\");\n-\t}\n-\n-\tdata = buffer.buf;\n-\tstart = NULL;\n-\tmid = data;\n-\twhile (i < buffer.len) {\n-\t\tif (!start)\n-\t\t\tstart = &data[i];\n-\t\tif (data[i] == '\\t')\n-\t\t\tmid = &data[i];\n-\t\tif (data[i] == '\\n') {\n-\t\t\tdata[i] = 0;\n-\t\t\tref_name = mid + 1;\n-\t\t\tref = xmalloc(sizeof(struct ref) +\n-\t\t\t\t      strlen(ref_name) + 1);\n-\t\t\tmemset(ref, 0, sizeof(struct ref));\n-\t\t\tstrcpy(ref->name, ref_name);\n-\t\t\tget_sha1_hex(start, ref->old_sha1);\n-\t\t\tif (!refs)\n-\t\t\t\trefs = ref;\n-\t\t\tif (last_ref)\n-\t\t\t\tlast_ref->next = ref;\n-\t\t\tlast_ref = ref;\n-\t\t\tstart = NULL;\n-\t\t}\n-\t\ti++;\n-\t}\n-\n-\tstrbuf_release(&buffer);\n-\n-\tref = alloc_ref(\"HEAD\");\n-\tif (!walker->fetch_ref(walker, ref) &&\n-\t    !resolve_remote_symref(ref, refs)) {\n-\t\tref->next = refs;\n-\t\trefs = ref;\n-\t} else {\n-\t\tfree(ref);\n-\t}\n-\n-\tstrbuf_release(&buffer);\n-\tfree(refs_url);\n-\treturn refs;\n-}\n-\n-static int fetch_objs_via_curl(struct transport *transport,\n-\t\t\t\t int nr_objs, const struct ref **to_fetch)\n-{\n-\tif (!transport->data)\n-\t\ttransport->data = get_http_walker(transport->url,\n-\t\t\t\t\t\ttransport->remote);\n-\treturn fetch_objs_via_walker(transport, nr_objs, to_fetch);\n-}\n-\n #endif\n \n struct bundle_transport_data {\n@@ -950,14 +818,12 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \t} else if (!prefixcmp(url, \"http://\")\n \t        || !prefixcmp(url, \"https://\")\n \t        || !prefixcmp(url, \"ftp://\")) {\n+\t\ttransport_shim_init(ret, \"curl\");\n #ifdef NO_CURL\n \t\terror(\"git was compiled without libcurl support.\");\n #else\n-\t\tret->get_refs_list = get_refs_via_curl;\n-\t\tret->fetch = fetch_objs_via_curl;\n \t\tret->push = curl_transport_push;\n #endif\n-\t\tret->disconnect = disconnect_walker;\n \n \t} else if (is_local(url) && is_file(url)) {\n \t\tstruct bundle_transport_data *data = xcalloc(1, sizeof(*data));\n-- \n1.6.4.rc2.13.ga9762.dirty\n"},{"id":"118957","messageId":"alpine.DEB.1.00.0907281505290.8306@pacific.mpi-cbg.de","threadId":"20257","inReplyTo":"alpine.LNX.2.00.0907280207350.2147@iabervon.org","subject":"Re: [PATCH 2/3 v2] Use an external program to implement fetching with curl","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-07-28T13:19:22Z","receivedAt":"2009-07-28T13:19:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 28 Jul 2009, Daniel Barkalow wrote:\n\n> diff --git a/Makefile b/Makefile\n> index 01efc73..d3dd2ed 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -980,6 +980,7 @@ else\n>  \t\tCURL_LIBCURL = -lcurl\n>  \tendif\n>  \tBUILTIN_OBJS += builtin-http-fetch.o\n> +\tPROGRAMS += git-shim-curl$X\n\nSo now you not only use that disgustingly meaningless name \"shim\", but \nalso abolish the notion that the helper is called by its protocol name?\n\n> diff --git a/shim-curl.c b/shim-curl.c\n> new file mode 100644\n> index 0000000..bde041b\n> --- /dev/null\n> +++ b/shim-curl.c\n> @@ -0,0 +1,132 @@\n> +#include \"cache.h\"\n> +#include \"remote.h\"\n> +#include \"strbuf.h\"\n> +#include \"walker.h\"\n> +#include \"http.h\"\n> +\n> +static struct ref *get_refs(struct walker *walker, const char *url)\n> +{\n> +\tint http_ret;\n> +\tchar *refs_url;\n> +\tstruct strbuf buffer = STRBUF_INIT;\n> +\tchar *data, *start, *mid;\n> +\tint i = 0;\n> +\tchar *ref_name;\n> +\tstruct ref *refs = NULL;\n> +\tstruct ref *ref = NULL;\n> +\tstruct ref *last_ref = NULL;\n> +\n> +\trefs_url = xmalloc(strlen(url) + 11);\n> +\tsprintf(refs_url, \"%s/info/refs\", url);\n\nAnd I thought we had strbufs...\n\n> +\n> +\thttp_ret = http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE);\n> +\tswitch (http_ret) {\n> +\tcase HTTP_OK:\n> +\t\tbreak;\n> +\tcase HTTP_MISSING_TARGET:\n> +\t\tdie(\"%s not found: did you run git update-server-info on the\"\n> +\t\t    \" server?\", refs_url);\n> +\tdefault:\n> +\t\thttp_error(refs_url, http_ret);\n> +\t\tdie(\"HTTP request failed\");\n> +\t}\n> +\tfree(refs_url);\n> +\n> +\tdata = buffer.buf;\n> +\tstart = NULL;\n> +\tmid = data;\n\nWhat does \"mid\" stand for?\n\n> +\twhile (i < buffer.len) {\n> +\t\tif (!start) {\n> +\t\t\tstart = &data[i];\n> +\t\t}\n\nWhy curly brackets around this one, but not this:\n\n> +\t\tif (data[i] == '\\t')\n> +\t\t\tmid = &data[i];\n\n> +\t\tif (data[i] == '\\n') {\n> +\t\t\tdata[i] = 0;\n> +\t\t\tref_name = mid + 1;\n> +\t\t\tref = xmalloc(sizeof(struct ref) +\n> +\t\t\t\t      strlen(ref_name) + 1);\n\nWhat is alloc_ref() for?\n\n> +\t\t\tmemset(ref, 0, sizeof(struct ref));\n> +\t\t\tstrcpy(ref->name, ref_name);\n> +\t\t\tget_sha1_hex(start, ref->old_sha1);\n> +\t\t\tif (!refs)\n> +\t\t\t\trefs = ref;\n> +\t\t\tif (last_ref)\n> +\t\t\t\tlast_ref->next = ref;\n> +\t\t\tlast_ref = ref;\n\nIs this another \"tail\"?\n\n> +\t\t\tstart = NULL;\n> +\t\t}\n> +\t\ti++;\n\nOh, so it _is_ a for() loop.\n\n> +\t}\n> +\tstrbuf_release(&buffer);\n> +\n> +\tref = alloc_ref(\"HEAD\");\n> +\tif (!walker->fetch_ref(walker, ref) &&\n> +\t    !resolve_remote_symref(ref, refs)) {\n> +\t\tref->next = refs;\n> +\t\trefs = ref;\n> +\t} else {\n> +\t\tfree(ref);\n> +\t}\n> +\n> +\treturn refs;\n> +}\n> +\n> +int main(int argc, const char **argv)\n> +{\n> +\tstruct remote *remote;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tconst char *url;\n> +\tstruct walker *walker = NULL;\n> +\n> +\tsetup_git_directory();\n> +\tif (argc < 2) {\n> +\t\tfprintf(stderr, \"Remote needed\\n\");\n> +\t\treturn 1;\n> +\t}\n> +\n> +\tremote = remote_get(argv[1]);\n> +\n> +\tif (argc > 2) {\n> +\t\turl = argv[2];\n> +\t} else {\n> +\t\turl = remote->url[0];\n> +\t}\n> +\n> +\tdo {\n> +\t\tif (strbuf_getline(&buf, stdin, '\\n') == EOF)\n> +\t\t\tbreak;\n> +\t\tif (!prefixcmp(buf.buf, \"fetch \")) {\n> +\t\t\tchar *obj = buf.buf + strlen(\"fetch \");\n> +\t\t\tif (!walker)\n> +\t\t\t\twalker = get_http_walker(url, remote);\n> +\t\t\twalker->get_all = 1;\n> +\t\t\twalker->get_tree = 1;\n> +\t\t\twalker->get_history = 1;\n> +\t\t\twalker->get_verbosely = 0;\n> +\t\t\twalker->get_recover = 0;\n> +\t\t\tif (walker_fetch(walker, 1, &obj, NULL, NULL))\n> +\t\t\t\tdie(\"Fetch failed.\");\n> +\t\t\tprintf(\"\\n\");\n> +\t\t\tfflush(stdout);\n> +\t\t} else if (!strcmp(buf.buf, \"list\")) {\n> +\t\t\tstruct ref *refs;\n> +\t\t\tstruct ref *posn;\n> +\t\t\tif (!walker)\n> +\t\t\t\twalker = get_http_walker(url, remote);\n> +\t\t\trefs = get_refs(walker, url);\n> +\t\t\tfor (posn = refs; posn; posn = posn->next) {\n> +\t\t\t\tif (posn->symref)\n> +\t\t\t\t\tprintf(\"@%s %s\\n\", posn->symref, posn->name);\n> +\t\t\t\telse\n> +\t\t\t\t\tprintf(\"%s %s\\n\", sha1_to_hex(posn->old_sha1), posn->name);\n> +\t\t\t}\n> +\t\t\tprintf(\"\\n\");\n> +\t\t\tfflush(stdout);\n> +\t\t} else {\n> +\t\t\treturn 1;\n> +\t\t}\n> +\t\tstrbuf_reset(&buf);\n> +\t} while (1);\n> +\treturn 0;\n> +}\n\nIf you already start some infrastructure like this, you should go the \nwhole nine yards and make helper functions in remote.c or transport.c that \nhelp implementing \"git-remote-<protocol>\" helpers.\n\n> diff --git a/transport.c b/transport.c\n> index de0d587..6d13b0e 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -950,14 +818,12 @@ struct transport *transport_get(struct remote *remote, const char *url)\n>  \t} else if (!prefixcmp(url, \"http://\")\n>  \t        || !prefixcmp(url, \"https://\")\n>  \t        || !prefixcmp(url, \"ftp://\")) {\n> +\t\ttransport_shim_init(ret, \"curl\");\n\nLike I said, identifying the remote helper by anything else than the \nprotocol is a design catastrophe.\n\n>  #ifdef NO_CURL\n>  \t\terror(\"git was compiled without libcurl support.\");\n\nWhy the heck does this have to be guarded by an #ifdef NO_CURL?  It is the \nhelper's task to say, not transport.c's.\n\n>  #else\n> -\t\tret->get_refs_list = get_refs_via_curl;\n> -\t\tret->fetch = fetch_objs_via_curl;\n>  \t\tret->push = curl_transport_push;\n>  #endif\n> -\t\tret->disconnect = disconnect_walker;\n>  \n>  \t} else if (is_local(url) && is_file(url)) {\n>  \t\tstruct bundle_transport_data *data = xcalloc(1, sizeof(*data));\n\nI think the basic idea of yours is sound, but all I see is some incomplete \nseparation between builtin git-fetch/-push and the helper.  As it stands, \ngit-fetch/-push have to be compiled with the exact knowledge what helper \nis there, git-fetch/-push are _still_ aware if Git was compiled with http \nsupport (so that it is not possible to just install the helper \nafterwards), and the infrastructure forbids any extension of capabilities \nin the future.\n\nI think these issues are serious enough to merit a rework.\n\nThanks,\nDscho\n"},{"id":"118961","messageId":"40aa078e0907280630p7751c3d9m639e63e7700a8819@mail.gmail.com","threadId":"20257","inReplyTo":"alpine.DEB.1.00.0907281505290.8306@pacific.mpi-cbg.de","subject":"Re: [PATCH 2/3 v2] Use an external program to implement fetching with curl","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-07-28T13:30:13Z","receivedAt":"2009-07-28T13:30:13Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, Jul 28, 2009 at 3:19 PM, Johannes\nSchindelin<Johannes.Schindelin@gmx.de> wrote:\n> If you already start some infrastructure like this, you should go the\n> whole nine yards and make helper functions in remote.c or transport.c that\n> help implementing \"git-remote-<protocol>\" helpers.\n\nYou mean helper-helpers? :)\n\n-- \nErik \"kusma\" Faye-Lund\nkusmabite@gmail.com\n(+47) 986 59 656\n"},{"id":"118965","messageId":"alpine.DEB.1.00.0907281658370.14706@intel-tinevez-2-302","threadId":"20257","inReplyTo":"40aa078e0907280630p7751c3d9m639e63e7700a8819@mail.gmail.com","subject":"Re: [PATCH 2/3 v2] Use an external program to implement fetching with curl","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-07-28T15:03:11Z","receivedAt":"2009-07-28T15:03:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 28 Jul 2009, Erik Faye-Lund wrote:\n\n> On Tue, Jul 28, 2009 at 3:19 PM, Johannes\n> Schindelin<Johannes.Schindelin@gmx.de> wrote:\n> > If you already start some infrastructure like this, you should go the \n> > whole nine yards and make helper functions in remote.c or transport.c \n> > that help implementing \"git-remote-<protocol>\" helpers.\n> \n> You mean helper-helpers? :)\n\nNot really... I meant functions that do all the stdin parsing for you, \ntogether with error handling, and only return proper lists of verified \narguments in the appropriate data structure.  This functionality is likely \nto be needed by all remote helpers, that's why I would prefer to have it \nexposed as functions in libgit.a, to be reused by all the helpers.\n\nOf course, this would not help shell scripting such helpers, but you have \nto start somewhere.  And that somewhere happens to be the http transport.\n\nCiao,\nDscho\n"},{"id":"118975","messageId":"alpine.LNX.2.00.0907281339260.2147@iabervon.org","threadId":"20257","inReplyTo":"alpine.DEB.1.00.0907281505290.8306@pacific.mpi-cbg.de","subject":"Re: [PATCH 2/3 v2] Use an external program to implement fetching with curl","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2009-07-28T17:59:44Z","receivedAt":"2009-07-28T17:59:44Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"\n\nOn Tue, 28 Jul 2009, Johannes Schindelin wrote:\n\n> Hi,\n> \n> On Tue, 28 Jul 2009, Daniel Barkalow wrote:\n> \n> > diff --git a/Makefile b/Makefile\n> > index 01efc73..d3dd2ed 100644\n> > --- a/Makefile\n> > +++ b/Makefile\n> > @@ -980,6 +980,7 @@ else\n> >  \t\tCURL_LIBCURL = -lcurl\n> >  \tendif\n> >  \tBUILTIN_OBJS += builtin-http-fetch.o\n> > +\tPROGRAMS += git-shim-curl$X\n> \n> So now you not only use that disgustingly meaningless name \"shim\", but \n> also abolish the notion that the helper is called by its protocol name?\n> \n> > diff --git a/shim-curl.c b/shim-curl.c\n> > new file mode 100644\n> > index 0000000..bde041b\n> > --- /dev/null\n> > +++ b/shim-curl.c\n> > @@ -0,0 +1,132 @@\n> > +#include \"cache.h\"\n> > +#include \"remote.h\"\n> > +#include \"strbuf.h\"\n> > +#include \"walker.h\"\n> > +#include \"http.h\"\n> > +\n> > +static struct ref *get_refs(struct walker *walker, const char *url)\n> > +{\n> > +\tint http_ret;\n> > +\tchar *refs_url;\n> > +\tstruct strbuf buffer = STRBUF_INIT;\n> > +\tchar *data, *start, *mid;\n> > +\tint i = 0;\n> > +\tchar *ref_name;\n> > +\tstruct ref *refs = NULL;\n> > +\tstruct ref *ref = NULL;\n> > +\tstruct ref *last_ref = NULL;\n> > +\n> > +\trefs_url = xmalloc(strlen(url) + 11);\n> > +\tsprintf(refs_url, \"%s/info/refs\", url);\n> \n> And I thought we had strbufs...\n\nThis whole chunk of code is a direct move from transport.c, which is \nunfortunately not obvious in the way we represent diffs. I'd prefer to \nkeep the code the same across the move so that we'd be able to track it if \n\"git blame\" got especially smart.\n\n> > +\n> > +\thttp_ret = http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE);\n> > +\tswitch (http_ret) {\n> > +\tcase HTTP_OK:\n> > +\t\tbreak;\n> > +\tcase HTTP_MISSING_TARGET:\n> > +\t\tdie(\"%s not found: did you run git update-server-info on the\"\n> > +\t\t    \" server?\", refs_url);\n> > +\tdefault:\n> > +\t\thttp_error(refs_url, http_ret);\n> > +\t\tdie(\"HTTP request failed\");\n> > +\t}\n> > +\tfree(refs_url);\n> > +\n> > +\tdata = buffer.buf;\n> > +\tstart = NULL;\n> > +\tmid = data;\n> \n> What does \"mid\" stand for?\n> \n> > +\twhile (i < buffer.len) {\n> > +\t\tif (!start) {\n> > +\t\t\tstart = &data[i];\n> > +\t\t}\n> \n> Why curly brackets around this one, but not this:\n> \n> > +\t\tif (data[i] == '\\t')\n> > +\t\t\tmid = &data[i];\n> \n> > +\t\tif (data[i] == '\\n') {\n> > +\t\t\tdata[i] = 0;\n> > +\t\t\tref_name = mid + 1;\n> > +\t\t\tref = xmalloc(sizeof(struct ref) +\n> > +\t\t\t\t      strlen(ref_name) + 1);\n> \n> What is alloc_ref() for?\n> \n> > +\t\t\tmemset(ref, 0, sizeof(struct ref));\n> > +\t\t\tstrcpy(ref->name, ref_name);\n> > +\t\t\tget_sha1_hex(start, ref->old_sha1);\n> > +\t\t\tif (!refs)\n> > +\t\t\t\trefs = ref;\n> > +\t\t\tif (last_ref)\n> > +\t\t\t\tlast_ref->next = ref;\n> > +\t\t\tlast_ref = ref;\n> \n> Is this another \"tail\"?\n> \n> > +\t\t\tstart = NULL;\n> > +\t\t}\n> > +\t\ti++;\n> \n> Oh, so it _is_ a for() loop.\n> \n> > +\t}\n> > +\tstrbuf_release(&buffer);\n> > +\n> > +\tref = alloc_ref(\"HEAD\");\n> > +\tif (!walker->fetch_ref(walker, ref) &&\n> > +\t    !resolve_remote_symref(ref, refs)) {\n> > +\t\tref->next = refs;\n> > +\t\trefs = ref;\n> > +\t} else {\n> > +\t\tfree(ref);\n> > +\t}\n> > +\n> > +\treturn refs;\n> > +}\n> > +\n> > +int main(int argc, const char **argv)\n> > +{\n> > +\tstruct remote *remote;\n> > +\tstruct strbuf buf = STRBUF_INIT;\n> > +\tconst char *url;\n> > +\tstruct walker *walker = NULL;\n> > +\n> > +\tsetup_git_directory();\n> > +\tif (argc < 2) {\n> > +\t\tfprintf(stderr, \"Remote needed\\n\");\n> > +\t\treturn 1;\n> > +\t}\n> > +\n> > +\tremote = remote_get(argv[1]);\n> > +\n> > +\tif (argc > 2) {\n> > +\t\turl = argv[2];\n> > +\t} else {\n> > +\t\turl = remote->url[0];\n> > +\t}\n> > +\n> > +\tdo {\n> > +\t\tif (strbuf_getline(&buf, stdin, '\\n') == EOF)\n> > +\t\t\tbreak;\n> > +\t\tif (!prefixcmp(buf.buf, \"fetch \")) {\n> > +\t\t\tchar *obj = buf.buf + strlen(\"fetch \");\n> > +\t\t\tif (!walker)\n> > +\t\t\t\twalker = get_http_walker(url, remote);\n> > +\t\t\twalker->get_all = 1;\n> > +\t\t\twalker->get_tree = 1;\n> > +\t\t\twalker->get_history = 1;\n> > +\t\t\twalker->get_verbosely = 0;\n> > +\t\t\twalker->get_recover = 0;\n> > +\t\t\tif (walker_fetch(walker, 1, &obj, NULL, NULL))\n> > +\t\t\t\tdie(\"Fetch failed.\");\n> > +\t\t\tprintf(\"\\n\");\n> > +\t\t\tfflush(stdout);\n> > +\t\t} else if (!strcmp(buf.buf, \"list\")) {\n> > +\t\t\tstruct ref *refs;\n> > +\t\t\tstruct ref *posn;\n> > +\t\t\tif (!walker)\n> > +\t\t\t\twalker = get_http_walker(url, remote);\n> > +\t\t\trefs = get_refs(walker, url);\n> > +\t\t\tfor (posn = refs; posn; posn = posn->next) {\n> > +\t\t\t\tif (posn->symref)\n> > +\t\t\t\t\tprintf(\"@%s %s\\n\", posn->symref, posn->name);\n> > +\t\t\t\telse\n> > +\t\t\t\t\tprintf(\"%s %s\\n\", sha1_to_hex(posn->old_sha1), posn->name);\n> > +\t\t\t}\n> > +\t\t\tprintf(\"\\n\");\n> > +\t\t\tfflush(stdout);\n> > +\t\t} else {\n> > +\t\t\treturn 1;\n> > +\t\t}\n> > +\t\tstrbuf_reset(&buf);\n> > +\t} while (1);\n> > +\treturn 0;\n> > +}\n> \n> If you already start some infrastructure like this, you should go the \n> whole nine yards and make helper functions in remote.c or transport.c that \n> help implementing \"git-remote-<protocol>\" helpers.\n\nI thought of that, but I couldn't think of any helper functions that \nwouldn't be harder to use than to not use. In any case, they should be in \na separate file, since they'd only be used by a set of separate programs.\n\n> > diff --git a/transport.c b/transport.c\n> > index de0d587..6d13b0e 100644\n> > --- a/transport.c\n> > +++ b/transport.c\n> > @@ -950,14 +818,12 @@ struct transport *transport_get(struct remote *remote, const char *url)\n> >  \t} else if (!prefixcmp(url, \"http://\")\n> >  \t        || !prefixcmp(url, \"https://\")\n> >  \t        || !prefixcmp(url, \"ftp://\")) {\n> > +\t\ttransport_shim_init(ret, \"curl\");\n> \n> Like I said, identifying the remote helper by anything else than the \n> protocol is a design catastrophe.\n\nBut this helper supports all of the protocols supported by curl, and may \neventually not be the only available helper that supports some of these \nprotocols. I think it's better to represent this fact by having \ntransport.c explicitly know when to use this helper, rather than linking \nthe same executable to git-remote-https, git-remote-http, git-remote-ftp, \netc.\n\n> >  #ifdef NO_CURL\n> >  \t\terror(\"git was compiled without libcurl support.\");\n> \n> Why the heck does this have to be guarded by an #ifdef NO_CURL?  It is the \n> helper's task to say, not transport.c's.\n\nWhat helper? There is no helper at all if NO_CURL, and:\n\n  git: 'remote-curl' is not a git-command. See 'git --help'.\n\nis not a user-friendly message. For now, I'm keeping the existing \nbehavior.\n\n> >  #else\n> > -\t\tret->get_refs_list = get_refs_via_curl;\n> > -\t\tret->fetch = fetch_objs_via_curl;\n> >  \t\tret->push = curl_transport_push;\n> >  #endif\n> > -\t\tret->disconnect = disconnect_walker;\n> >  \n> >  \t} else if (is_local(url) && is_file(url)) {\n> >  \t\tstruct bundle_transport_data *data = xcalloc(1, sizeof(*data));\n> \n> I think the basic idea of yours is sound, but all I see is some incomplete \n> separation between builtin git-fetch/-push and the helper.  As it stands, \n> git-fetch/-push have to be compiled with the exact knowledge what helper \n> is there, git-fetch/-push are _still_ aware if Git was compiled with http \n> support (so that it is not possible to just install the helper \n> afterwards), and the infrastructure forbids any extension of capabilities \n> in the future.\n\nIn the long run, I want to have two methods for running an external \nhelper: one where the installation in some way knows when to run the \nhelper (either by having code in place or by having helpers with \nparticular names or something), and one where the user specifies something \nexplicit. That is, git shouldn't know that a remote with no url but some \n\"codeline\" settings that start with \"//\" is p4, and the user shouldn't \nhave to know that git repositories whose urls start with http:// are done \nwith an external helper, so no one mechanism is sufficient.\n\nIt would be nice if git supported querying its environment to see whether \na command is available and printing a message suitted to why it was \nlooking for it, but that's a separate topic.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"119107","messageId":"alpine.DEB.1.00.0907300002450.8306@pacific.mpi-cbg.de","threadId":"20257","inReplyTo":"alpine.LNX.2.00.0907281339260.2147@iabervon.org","subject":"Re: [PATCH 2/3 v2] Use an external program to implement fetching with curl","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-07-29T22:16:44Z","receivedAt":"2009-07-29T22:16:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 28 Jul 2009, Daniel Barkalow wrote:\n\n> On Tue, 28 Jul 2009, Johannes Schindelin wrote:\n> \n> > On Tue, 28 Jul 2009, Daniel Barkalow wrote:\n> > \n> > > diff --git a/shim-curl.c b/shim-curl.c\n> > > new file mode 100644\n> > > index 0000000..bde041b\n> > > --- /dev/null\n> > > +++ b/shim-curl.c\n> > > @@ -0,0 +1,132 @@\n> > > +#include \"cache.h\"\n> > > +#include \"remote.h\"\n> > > +#include \"strbuf.h\"\n> > > +#include \"walker.h\"\n> > > +#include \"http.h\"\n> > > +\n> > > +static struct ref *get_refs(struct walker *walker, const char *url)\n> > > +{\n> > > +\tint http_ret;\n> > > +\tchar *refs_url;\n> > > +\tstruct strbuf buffer = STRBUF_INIT;\n> > > +\tchar *data, *start, *mid;\n> > > +\tint i = 0;\n> > > +\tchar *ref_name;\n> > > +\tstruct ref *refs = NULL;\n> > > +\tstruct ref *ref = NULL;\n> > > +\tstruct ref *last_ref = NULL;\n> > > +\n> > > +\trefs_url = xmalloc(strlen(url) + 11);\n> > > +\tsprintf(refs_url, \"%s/info/refs\", url);\n> > \n> > And I thought we had strbufs...\n> \n> This whole chunk of code is a direct move from transport.c, which is \n> unfortunately not obvious in the way we represent diffs. I'd prefer to \n> keep the code the same across the move so that we'd be able to track it \n> if \"git blame\" got especially smart.\n\n... and make review of the current patch -- which is possible right now, \nwithout the need of a super clever git blame -- hard?  Do you really think \nthis is a wise decision?\n\n> > > [a couple of unaddressed comments]\n> > >\n> > > +int main(int argc, const char **argv)\n> > > +{\n> > > +\tstruct remote *remote;\n> > > +\tstruct strbuf buf = STRBUF_INIT;\n> > > +\tconst char *url;\n> > > +\tstruct walker *walker = NULL;\n> > > +\n> > > +\tsetup_git_directory();\n> > > +\tif (argc < 2) {\n> > > +\t\tfprintf(stderr, \"Remote needed\\n\");\n> > > +\t\treturn 1;\n> > > +\t}\n> > > +\n> > > +\tremote = remote_get(argv[1]);\n> > > +\n> > > +\tif (argc > 2) {\n> > > +\t\turl = argv[2];\n> > > +\t} else {\n> > > +\t\turl = remote->url[0];\n> > > +\t}\n> > > +\n> > > +\tdo {\n> > > +\t\tif (strbuf_getline(&buf, stdin, '\\n') == EOF)\n> > > +\t\t\tbreak;\n> > > +\t\tif (!prefixcmp(buf.buf, \"fetch \")) {\n> > > +\t\t\tchar *obj = buf.buf + strlen(\"fetch \");\n> > > +\t\t\tif (!walker)\n> > > +\t\t\t\twalker = get_http_walker(url, remote);\n> > > +\t\t\twalker->get_all = 1;\n> > > +\t\t\twalker->get_tree = 1;\n> > > +\t\t\twalker->get_history = 1;\n> > > +\t\t\twalker->get_verbosely = 0;\n> > > +\t\t\twalker->get_recover = 0;\n> > > +\t\t\tif (walker_fetch(walker, 1, &obj, NULL, NULL))\n> > > +\t\t\t\tdie(\"Fetch failed.\");\n> > > +\t\t\tprintf(\"\\n\");\n> > > +\t\t\tfflush(stdout);\n> > > +\t\t} else if (!strcmp(buf.buf, \"list\")) {\n> > > +\t\t\tstruct ref *refs;\n> > > +\t\t\tstruct ref *posn;\n> > > +\t\t\tif (!walker)\n> > > +\t\t\t\twalker = get_http_walker(url, remote);\n> > > +\t\t\trefs = get_refs(walker, url);\n> > > +\t\t\tfor (posn = refs; posn; posn = posn->next) {\n> > > +\t\t\t\tif (posn->symref)\n> > > +\t\t\t\t\tprintf(\"@%s %s\\n\", posn->symref, posn->name);\n> > > +\t\t\t\telse\n> > > +\t\t\t\t\tprintf(\"%s %s\\n\", sha1_to_hex(posn->old_sha1), posn->name);\n> > > +\t\t\t}\n> > > +\t\t\tprintf(\"\\n\");\n> > > +\t\t\tfflush(stdout);\n> > > +\t\t} else {\n> > > +\t\t\treturn 1;\n> > > +\t\t}\n> > > +\t\tstrbuf_reset(&buf);\n> > > +\t} while (1);\n> > > +\treturn 0;\n> > > +}\n> > \n> > If you already start some infrastructure like this, you should go the \n> > whole nine yards and make helper functions in remote.c or transport.c that \n> > help implementing \"git-remote-<protocol>\" helpers.\n> \n> I thought of that, but I couldn't think of any helper functions that \n> wouldn't be harder to use than to not use. In any case, they should be in \n> a separate file, since they'd only be used by a set of separate programs.\n\nSo every helper should implement the same parsing of the whole protocol?\n\nI mean, the protocol _lends_ itself to an interface that implements \nhandlers for the commands sent over the pipe, but with compile time errors \nwhen, say, a parameter type mistake happened.\n\n> > > diff --git a/transport.c b/transport.c\n> > > index de0d587..6d13b0e 100644\n> > > --- a/transport.c\n> > > +++ b/transport.c\n> > > @@ -950,14 +818,12 @@ struct transport *transport_get(struct remote *remote, const char *url)\n> > >  \t} else if (!prefixcmp(url, \"http://\")\n> > >  \t        || !prefixcmp(url, \"https://\")\n> > >  \t        || !prefixcmp(url, \"ftp://\")) {\n> > > +\t\ttransport_shim_init(ret, \"curl\");\n> > \n> > Like I said, identifying the remote helper by anything else than the \n> > protocol is a design catastrophe.\n> \n> But this helper supports all of the protocols supported by curl, and may \n> eventually not be the only available helper that supports some of these \n> protocols.\n\nYes, and in the near future my computer may understand what I say, and \nguess my thoughts.\n\nLet's stay with the presence, okay?\n\nIt is _all_ too easy to hardlink git-remote-http to git-remote-https.\n\nAnd if you have two different handlers for the same protocol, how should \n_Git_ decide which one to take?\n\nNo, I firmly believe that this design decision is too short-sighted.\n\n> I think it's better to represent this fact by having transport.c \n> explicitly know when to use this helper, rather than linking the same \n> executable to git-remote-https, git-remote-http, git-remote-ftp, etc.\n\nYou really want to recompile Git whenever somebody writes a new protocol \nhelper?  That's wrong.\n\n> > >  #ifdef NO_CURL\n> > >  \t\terror(\"git was compiled without libcurl support.\");\n> > \n> > Why the heck does this have to be guarded by an #ifdef NO_CURL?  It is the \n> > helper's task to say, not transport.c's.\n> \n> What helper? There is no helper at all if NO_CURL,\n\nAgain, you want to _require_ that the helpers and Git are compiled \ntogether.  I stand firmly against this idea.\n\n> and:\n> \n>   git: 'remote-curl' is not a git-command. See 'git --help'.\n> \n> is not a user-friendly message.\n\nOh?  You would want to show the user that message?  I wouldn't.  I would \nwant to show the user:\n\n\tNo helper found to handle protocol 'https' (i.e. no \n\tgit-remote-http was found in the PATH)\n\n> > >  #else\n> > > -\t\tret->get_refs_list = get_refs_via_curl;\n> > > -\t\tret->fetch = fetch_objs_via_curl;\n> > >  \t\tret->push = curl_transport_push;\n> > >  #endif\n> > > -\t\tret->disconnect = disconnect_walker;\n> > >  \n> > >  \t} else if (is_local(url) && is_file(url)) {\n> > >  \t\tstruct bundle_transport_data *data = xcalloc(1, sizeof(*data));\n> > \n> > I think the basic idea of yours is sound, but all I see is some incomplete \n> > separation between builtin git-fetch/-push and the helper.  As it stands, \n> > git-fetch/-push have to be compiled with the exact knowledge what helper \n> > is there, git-fetch/-push are _still_ aware if Git was compiled with http \n> > support (so that it is not possible to just install the helper \n> > afterwards), and the infrastructure forbids any extension of capabilities \n> > in the future.\n> \n> In the long run, I want to have two methods for running an external \n> helper: one where the installation in some way knows when to run the \n> helper (either by having code in place or by having helpers with \n> particular names or something), and one where the user specifies something \n> explicit.\n\nI am not sure I like that.\n\n> That is, git shouldn't know that a remote with no url but some \n> \"codeline\" settings that start with \"//\" is p4,\n\nWhat speaks against a URL \"p4://...\"?  That would be _totally_ intuitive \nand easy to handle.\n\n> and the user shouldn't have to know that git repositories whose urls \n> start with http:// are done with an external helper, so no one mechanism \n> is sufficient.\n\nWhy on earth should the user need to know that http:// is handled by an \nexternal helper?\n\nOf course, if the user wants to support, say, VSS, it is now easy (well, \nthe Git part) to provide a simple git-remote-vss helper _and not having to \nrecompile Git at all_.\n\n> It would be nice if git supported querying its environment to see whether \n> a command is available and printing a message suitted to why it was \n> looking for it, but that's a separate topic.\n\nWe do have such a framework in place already.  You even mentioned it.  Our \ngit-help can look in the PATH what is available.\n\nBut then, we do not have to do that at all.\n\nWe just try to handle the protocol internally.  If that is not possible, \nwe extract the protocol from the URL and try to call a program called \ngit-remote-<protocol>.  We inquire via the capabilities command if that \nhelper can fetch/push and continue accordingly.  If the program could not \nbe launched, with exit status 127, we know there was no helper and can \ntell the user that.\n\nWe do not need to do any discovery of the environment at all.  \nstart_command() is all we ever need.\n\nSimple is beautiful,\nDscho\n"}]}