{"thread":{"id":"13804","subject":"[PATCH] handle http urls with query string (\"?foo\") correctly","startedAt":"2008-06-04T23:28:31Z","lastAt":"2008-06-05T12:14:59Z","messageCount":8,"participants":["David ‘Bombe’ Roden","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"78678","messageId":"200806050128.33467.bombe@pterodactylus.net","threadId":"13804","inReplyTo":null,"subject":"[PATCH] handle http urls with query string (\"?foo\") correctly","fromName":"David ‘Bombe’ Roden","fromEmail":"bombe@pterodactylus.net","sentAt":"2008-06-04T23:28:31Z","receivedAt":"2008-06-04T23:28:31Z","isPatch":true,"sender":{"key":"bombe@pterodactylus.net","avatar":"https://gravatar.com/avatar/6845b170b5e882edcb27b015cdad0fb918f2d079c2cf77e668dcc71ae645ed87?d=mp&s=160"},"body":"From 1f55b9176248673b78c77ff292fb5c6c988a7f8b Mon Sep 17 00:00:00 2001\nFrom: =?utf-8?q?David=20=E2=80=98Bombe=E2=80=99=20Roden?= <bombe@pterodactylus.net>\nDate: Thu, 5 Jun 2008 00:53:56 +0200\nSubject: [PATCH] handle http urls with query string (\"?foo\") correctly\n\n---\nI'm a developer for the Freenet Project[1] and tried to get git running with Freenet's http-gateway. For several reasons I have to use URLs \nlike \"http://host:8888/USK@<lots-of-stuff>/project.git/4?type=text/plain\". This breaks the way git creates http URLs to retrieve. The \nfollowing patch remedys this situation by inserting the git-generated part of the URL (like \"/info/refs\") before the query string.\n\n http-walker.c |   31 ++++++++++++-------------------\n http.c        |   43 ++++++++++++++++++++++++++++++++++---------\n http.h        |    2 ++\n transport.c   |    5 +++--\n 4 files changed, 51 insertions(+), 30 deletions(-)\n\ndiff --git a/http-walker.c b/http-walker.c\nindex 99f397e..711e55b 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -100,7 +100,6 @@ static void start_object_request(struct walker *walker,\n \tchar *hex = sha1_to_hex(obj_req->sha1);\n \tchar prevfile[PATH_MAX];\n \tchar *url;\n-\tchar *posn;\n \tint prevlocal;\n \tunsigned char prev_buf[PREV_BUF_SIZE];\n \tssize_t prev_read = 0;\n@@ -146,16 +145,11 @@ static void start_object_request(struct walker *walker,\n \n \tSHA1_Init(&obj_req->c);\n \n-\turl = xmalloc(strlen(obj_req->repo->base) + 51);\n+\tchar *suffix = xmalloc(51);\n \tobj_req->url = xmalloc(strlen(obj_req->repo->base) + 51);\n-\tstrcpy(url, obj_req->repo->base);\n-\tposn = url + strlen(obj_req->repo->base);\n-\tstrcpy(posn, \"/objects/\");\n-\tposn += 9;\n-\tmemcpy(posn, hex, 2);\n-\tposn += 2;\n-\t*(posn++) = '/';\n-\tstrcpy(posn, hex + 2);\n+\tstrcpy(suffix, \"/objects/\");\n+\tsprintf(suffix + 9, \"%c%c/%s\", hex[0], hex[1], hex + 2);\n+\turl = transform_url(obj_req->repo->base, suffix);\n \tstrcpy(obj_req->url, url);\n \n \t/* If a previous temp file is present, process what was already\n@@ -384,8 +378,9 @@ static int fetch_index(struct walker *walker, struct alt_base *repo, unsigned ch\n \tif (walker->get_verbosely)\n \t\tfprintf(stderr, \"Getting index for pack %s\\n\", hex);\n \n-\turl = xmalloc(strlen(repo->base) + 64);\n-\tsprintf(url, \"%s/objects/pack/pack-%s.idx\", repo->base, hex);\n+\tchar *index_url = xmalloc(64);\n+\tsprintf(index_url, \"/objects/pack/pack-%s.idx\", hex);\n+\turl = transform_url(repo->base, index_url);\n \n \tfilename = sha1_pack_index_name(sha1);\n \tsnprintf(tmpfile, sizeof(tmpfile), \"%s.temp\", filename);\n@@ -608,8 +603,7 @@ static void fetch_alternates(struct walker *walker, const char *base)\n \tif (walker->get_verbosely)\n \t\tfprintf(stderr, \"Getting alternates list for %s\\n\", base);\n \n-\turl = xmalloc(strlen(base) + 31);\n-\tsprintf(url, \"%s/objects/info/http-alternates\", base);\n+\turl = transform_url(base, \"/objects/info/http-alternates\");\n \n \t/* Use a callback to process the result, since another request\n \t   may fail and need to have alternates loaded before continuing */\n@@ -655,8 +649,7 @@ static int fetch_indices(struct walker *walker, struct alt_base *repo)\n \tif (walker->get_verbosely)\n \t\tfprintf(stderr, \"Getting pack list for %s\\n\", repo->base);\n \n-\turl = xmalloc(strlen(repo->base) + 21);\n-\tsprintf(url, \"%s/objects/info/packs\", repo->base);\n+\turl = transform_url(repo->base, \"/objects/info/packs\");\n \n \tslot = get_active_slot();\n \tslot->results = &results;\n@@ -739,9 +732,9 @@ static int fetch_pack(struct walker *walker, struct alt_base *repo, unsigned cha\n \t\t\tsha1_to_hex(sha1));\n \t}\n \n-\turl = xmalloc(strlen(repo->base) + 65);\n-\tsprintf(url, \"%s/objects/pack/pack-%s.pack\",\n-\t\trepo->base, sha1_to_hex(target->sha1));\n+\tchar *pack_url = xmalloc(65);\n+\tsprintf(pack_url, \"/objects/pack/pack-%s.pack\", sha1_to_hex(target->sha1));\n+\turl = transform_url(repo->base, pack_url);\n \n \tfilename = sha1_pack_name(target->sha1);\n \tsnprintf(tmpfile, sizeof(tmpfile), \"%s.temp\", filename);\ndiff --git a/http.c b/http.c\nindex 2a21ccb..e2b764a 100644\n--- a/http.c\n+++ b/http.c\n@@ -590,15 +590,17 @@ static char *quote_ref_url(const char *base, const char *ref)\n \tqref = xmalloc(len);\n \tmemcpy(qref, base, baselen);\n \tdp = qref + baselen;\n-\t*(dp++) = '/';\n-\tfor (cp = ref; (ch = *cp) != 0; cp++) {\n-\t\tif (needs_quote(ch)) {\n-\t\t\t*dp++ = '%';\n-\t\t\t*dp++ = hex((ch >> 4) & 0xF);\n-\t\t\t*dp++ = hex(ch & 0xF);\n+\tif (*ref) {\n+\t\t*(dp++) = '/';\n+\t\tfor (cp = ref; (ch = *cp) != 0; cp++) {\n+\t\t\tif (needs_quote(ch)) {\n+\t\t\t\t*dp++ = '%';\n+\t\t\t\t*dp++ = hex((ch >> 4) & 0xF);\n+\t\t\t\t*dp++ = hex(ch & 0xF);\n+\t\t\t}\n+\t\t\telse\n+\t\t\t\t*dp++ = ch;\n \t\t}\n-\t\telse\n-\t\t\t*dp++ = ch;\n \t}\n \t*dp = 0;\n \n@@ -613,7 +615,10 @@ int http_fetch_ref(const char *base, struct ref *ref)\n \tstruct slot_results results;\n \tint ret;\n \n-\turl = quote_ref_url(base, ref->name);\n+\tchar *real_ref_name = xmalloc(strlen(ref->name) + 2);\n+\tsprintf(real_ref_name, \"/%s\", ref->name);\n+\tchar *real_url = transform_url(base, real_ref_name);\n+\turl = quote_ref_url(real_url, \"\");\n \tslot = get_active_slot();\n \tslot->results = &results;\n \tcurl_easy_setopt(slot->curl, CURLOPT_FILE, &buffer);\n@@ -643,3 +648,23 @@ int http_fetch_ref(const char *base, struct ref *ref)\n \tfree(url);\n \treturn ret;\n }\n+\n+char *transform_url(const char *url, const char *suffix)\n+{\n+\tchar *new_url;\n+\tchar *question_mark;\n+\tptrdiff_t offset;\n+\n+\tnew_url = xmalloc(strlen(url) + strlen(suffix) + 1);\n+\n+\tif ((question_mark = strchr(url, '?'))) {\n+\t\toffset = (ptrdiff_t) question_mark - (ptrdiff_t) url;\n+\t\tsnprintf(new_url, offset + 1, \"%s\", url);\n+\t\tstrcat(new_url, suffix);\n+\t\tstrcat(new_url, url + offset);\n+\t} else {\n+\t\tsprintf(new_url, \"%s%s\", url, suffix);\n+\t}\n+\treturn new_url;\n+}\n+\ndiff --git a/http.h b/http.h\nindex a04fc6a..58730b8 100644\n--- a/http.h\n+++ b/http.h\n@@ -107,4 +107,6 @@ static inline int missing__target(int code, int result)\n \n extern int http_fetch_ref(const char *base, struct ref *ref);\n \n+extern char *transform_url(const char *url, const char *suffix);\n+\n #endif /* HTTP_H */\ndiff --git a/transport.c b/transport.c\nindex 3ff8519..b1966d8 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1,3 +1,4 @@\n+#include <stddef.h>\n #include \"cache.h\"\n #include \"transport.h\"\n #include \"run-command.h\"\n@@ -449,8 +450,7 @@ static struct ref *get_refs_via_curl(struct transport *transport)\n \n \twalker = transport->data;\n \n-\trefs_url = xmalloc(strlen(transport->url) + 11);\n-\tsprintf(refs_url, \"%s/info/refs\", transport->url);\n+\trefs_url = transform_url(transport->url, \"/info/refs\");\n \n \tslot = get_active_slot();\n \tslot->results = &results;\n@@ -833,3 +833,4 @@ int transport_disconnect(struct transport *transport)\n \tfree(transport);\n \treturn ret;\n }\n+\n-- \n1.5.6.rc1.6.gc8c52\n\n"},{"id":"78681","messageId":"alpine.DEB.1.00.0806050103520.21190@racer","threadId":"13804","inReplyTo":"200806050128.33467.bombe@pterodactylus.net","subject":"Re: [PATCH] handle http urls with query string (\"?foo\") correctly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-06-05T00:12:21Z","receivedAt":"2008-06-05T00:12:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 5 Jun 2008, David ‘Bombe’ Roden wrote:\n\n> From 1f55b9176248673b78c77ff292fb5c6c988a7f8b Mon Sep 17 00:00:00 2001\n> From: =?utf-8?q?David=20=E2=80=98Bombe=E2=80=99=20Roden?= <bombe@pterodactylus.net>\n> Date: Thu, 5 Jun 2008 00:53:56 +0200\n> Subject: [PATCH] handle http urls with query string (\"?foo\") correctly\n\nPlease follow the hints in Documentation/SubmittingPatches, or just \nimitate others' mails with patches.\n\n> I'm a developer for the Freenet Project[1] and tried to get git running \n> with Freenet's http-gateway. For several reasons I have to use URLs like \n> \"http://host:8888/USK@<lots-of-stuff>/project.git/4?type=text/plain\". \n> This breaks the way git creates http URLs to retrieve. The following \n> patch remedys this situation by inserting the git-generated part of the \n> URL (like \"/info/refs\") before the query string.\n\nI think that this paragraph should be rewritten into a less \"I did\" form, \nand be the commit message.\n\n> diff --git a/http-walker.c b/http-walker.c\n> index 99f397e..711e55b 100644\n> --- a/http-walker.c\n> +++ b/http-walker.c\n> @@ -100,7 +100,6 @@ static void start_object_request(struct walker *walker,\n>  \tchar *hex = sha1_to_hex(obj_req->sha1);\n>  \tchar prevfile[PATH_MAX];\n>  \tchar *url;\n> -\tchar *posn;\n>  \tint prevlocal;\n>  \tunsigned char prev_buf[PREV_BUF_SIZE];\n>  \tssize_t prev_read = 0;\n> @@ -146,16 +145,11 @@ static void start_object_request(struct walker *walker,\n>  \n>  \tSHA1_Init(&obj_req->c);\n>  \n> -\turl = xmalloc(strlen(obj_req->repo->base) + 51);\n> +\tchar *suffix = xmalloc(51);\n\nI guess you are a Java programmer?  In C, this would be \"char \nsuffix[51];\".\n\nAlso, we like to have C89 compatibility, especially when it is too simple: \ndeclarations have to come _before_ statements.\n \n>  \tobj_req->url = xmalloc(strlen(obj_req->repo->base) + 51);\n> -\tstrcpy(url, obj_req->repo->base);\n> -\tposn = url + strlen(obj_req->repo->base);\n> -\tstrcpy(posn, \"/objects/\");\n> -\tposn += 9;\n> -\tmemcpy(posn, hex, 2);\n> -\tposn += 2;\n> -\t*(posn++) = '/';\n> -\tstrcpy(posn, hex + 2);\n> +\tstrcpy(suffix, \"/objects/\");\n> +\tsprintf(suffix + 9, \"%c%c/%s\", hex[0], hex[1], hex + 2);\n> +\turl = transform_url(obj_req->repo->base, suffix);\n\nThis part of Git's source code predates the strbuf work, and therefore it \nis understandable that strbufs are not used there.  However, I think that \nyour changes just cry for want of strbufs.\n\nAlso, the strcpy() could be merged into the sprintf().\n\n> @@ -384,8 +378,9 @@ static int fetch_index(struct walker *walker, struct alt_base *repo, unsigned ch\n>  \tif (walker->get_verbosely)\n>  \t\tfprintf(stderr, \"Getting index for pack %s\\n\", hex);\n>  \n> -\turl = xmalloc(strlen(repo->base) + 64);\n> -\tsprintf(url, \"%s/objects/pack/pack-%s.idx\", repo->base, hex);\n> +\tchar *index_url = xmalloc(64);\n> +\tsprintf(index_url, \"/objects/pack/pack-%s.idx\", hex);\n> +\turl = transform_url(repo->base, index_url);\n\nAgain, declaration-after-statement, and again an opportunity for an \nstrbuf.\n\nMaybe you want to rewrite your patch before I keep on repeating these two \ncomments? ;-)\n\nThanks,\nDscho"},{"id":"78707","messageId":"200806050848.43462.bombe@pterodactylus.net","threadId":"13804","inReplyTo":"alpine.DEB.1.00.0806050103520.21190@racer","subject":"Re: [PATCH] handle http urls with query string (\"?foo\") correctly","fromName":"David ‘Bombe’ Roden","fromEmail":"bombe@pterodactylus.net","sentAt":"2008-06-05T06:48:39Z","receivedAt":"2008-06-05T06:48:39Z","isPatch":true,"sender":{"key":"bombe@pterodactylus.net","avatar":"https://gravatar.com/avatar/6845b170b5e882edcb27b015cdad0fb918f2d079c2cf77e668dcc71ae645ed87?d=mp&s=160"},"body":"On Thursday 05 June 2008 02:12:21 Johannes Schindelin wrote:\n\n> I think that this paragraph should be rewritten into a less \"I did\" form,\n> and be the commit message.\n\nOkay.\n\n\n> This part of Git's source code predates the strbuf work, and therefore it\n> is understandable that strbufs are not used there.  However, I think that\n> your changes just cry for want of strbufs.\n\nSounds good. strbuf is indeed a tad more convenient than sprintf and strcat. :)\n\n\n> Maybe you want to rewrite your patch before I keep on repeating these two\n> comments? ;-)\n\nI did. Find the revised version below. :)\n\n---\nhandle http urls with query string (\"?foo\") correctly\ngit breaks when a repository is cloned from an http url that contains a\nquery string. This patch fixes this behaviour be inserting the name of\nthe requested object (like \"/info/refs\") before the query string.\n\n http-walker.c |   31 ++++++++++++-------------------\n http.c        |   42 +++++++++++++++++++++++++++++++++---------\n http.h        |    2 ++\n transport.c   |    5 +++--\n 4 files changed, 50 insertions(+), 30 deletions(-)\n\ndiff --git a/http-walker.c b/http-walker.c\nindex 99f397e..b14497a 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -100,7 +100,6 @@ static void start_object_request(struct walker *walker,\n \tchar *hex = sha1_to_hex(obj_req->sha1);\n \tchar prevfile[PATH_MAX];\n \tchar *url;\n-\tchar *posn;\n \tint prevlocal;\n \tunsigned char prev_buf[PREV_BUF_SIZE];\n \tssize_t prev_read = 0;\n@@ -109,6 +108,7 @@ static void start_object_request(struct walker *walker,\n \tstruct curl_slist *range_header = NULL;\n \tstruct active_request_slot *slot;\n \tstruct walker_data *data = walker->data;\n+\tstruct strbuf suffix_buffer = STRBUF_INIT;\n \n \tsnprintf(prevfile, sizeof(prevfile), \"%s.prev\", obj_req->filename);\n \tunlink(prevfile);\n@@ -146,16 +146,10 @@ static void start_object_request(struct walker *walker,\n \n \tSHA1_Init(&obj_req->c);\n \n-\turl = xmalloc(strlen(obj_req->repo->base) + 51);\n+\tstrbuf_addstr(&suffix_buffer, \"/objects/\");\n+\tstrbuf_addf(&suffix_buffer, \"%c%c/%s\", hex[0], hex[1], hex + 2);\n+\turl = transform_url(obj_req->repo->base, strbuf_detach(&suffix_buffer, NULL));\n \tobj_req->url = xmalloc(strlen(obj_req->repo->base) + 51);\n-\tstrcpy(url, obj_req->repo->base);\n-\tposn = url + strlen(obj_req->repo->base);\n-\tstrcpy(posn, \"/objects/\");\n-\tposn += 9;\n-\tmemcpy(posn, hex, 2);\n-\tposn += 2;\n-\t*(posn++) = '/';\n-\tstrcpy(posn, hex + 2);\n \tstrcpy(obj_req->url, url);\n \n \t/* If a previous temp file is present, process what was already\n@@ -373,6 +367,7 @@ static int fetch_index(struct walker *walker, struct alt_base *repo, unsigned ch\n \tchar range[RANGE_HEADER_SIZE];\n \tstruct curl_slist *range_header = NULL;\n \tstruct walker_data *data = walker->data;\n+\tstruct strbuf suffix_buffer = STRBUF_INIT;\n \n \tFILE *indexfile;\n \tstruct active_request_slot *slot;\n@@ -384,8 +379,8 @@ static int fetch_index(struct walker *walker, struct alt_base *repo, unsigned ch\n \tif (walker->get_verbosely)\n \t\tfprintf(stderr, \"Getting index for pack %s\\n\", hex);\n \n-\turl = xmalloc(strlen(repo->base) + 64);\n-\tsprintf(url, \"%s/objects/pack/pack-%s.idx\", repo->base, hex);\n+\tstrbuf_addf(&suffix_buffer, \"/objects/pack/pack-%s.idx\", hex);\n+\turl = transform_url(repo->base, strbuf_detach(&suffix_buffer, NULL));\n \n \tfilename = sha1_pack_index_name(sha1);\n \tsnprintf(tmpfile, sizeof(tmpfile), \"%s.temp\", filename);\n@@ -608,8 +603,7 @@ static void fetch_alternates(struct walker *walker, const char *base)\n \tif (walker->get_verbosely)\n \t\tfprintf(stderr, \"Getting alternates list for %s\\n\", base);\n \n-\turl = xmalloc(strlen(base) + 31);\n-\tsprintf(url, \"%s/objects/info/http-alternates\", base);\n+\turl = transform_url(base, \"/objects/info/http-alternates\");\n \n \t/* Use a callback to process the result, since another request\n \t   may fail and need to have alternates loaded before continuing */\n@@ -655,8 +649,7 @@ static int fetch_indices(struct walker *walker, struct alt_base *repo)\n \tif (walker->get_verbosely)\n \t\tfprintf(stderr, \"Getting pack list for %s\\n\", repo->base);\n \n-\turl = xmalloc(strlen(repo->base) + 21);\n-\tsprintf(url, \"%s/objects/info/packs\", repo->base);\n+\turl = transform_url(repo->base, \"/objects/info/packs\");\n \n \tslot = get_active_slot();\n \tslot->results = &results;\n@@ -722,6 +715,7 @@ static int fetch_pack(struct walker *walker, struct alt_base *repo, unsigned cha\n \tchar range[RANGE_HEADER_SIZE];\n \tstruct curl_slist *range_header = NULL;\n \tstruct walker_data *data = walker->data;\n+\tstruct strbuf suffix_buffer = STRBUF_INIT;\n \n \tstruct active_request_slot *slot;\n \tstruct slot_results results;\n@@ -739,9 +733,8 @@ static int fetch_pack(struct walker *walker, struct alt_base *repo, unsigned cha\n \t\t\tsha1_to_hex(sha1));\n \t}\n \n-\turl = xmalloc(strlen(repo->base) + 65);\n-\tsprintf(url, \"%s/objects/pack/pack-%s.pack\",\n-\t\trepo->base, sha1_to_hex(target->sha1));\n+\tstrbuf_addf(&suffix_buffer, \"/objects/pack/pack-%s.pack\", sha1_to_hex(target->sha1));\n+\turl = transform_url(repo->base, strbuf_detach(&suffix_buffer, NULL));\n \n \tfilename = sha1_pack_name(target->sha1);\n \tsnprintf(tmpfile, sizeof(tmpfile), \"%s.temp\", filename);\ndiff --git a/http.c b/http.c\nindex 2a21ccb..a60f9ea 100644\n--- a/http.c\n+++ b/http.c\n@@ -590,15 +590,17 @@ static char *quote_ref_url(const char *base, const char *ref)\n \tqref = xmalloc(len);\n \tmemcpy(qref, base, baselen);\n \tdp = qref + baselen;\n-\t*(dp++) = '/';\n-\tfor (cp = ref; (ch = *cp) != 0; cp++) {\n-\t\tif (needs_quote(ch)) {\n-\t\t\t*dp++ = '%';\n-\t\t\t*dp++ = hex((ch >> 4) & 0xF);\n-\t\t\t*dp++ = hex(ch & 0xF);\n+\tif (*ref) {\n+\t\t*(dp++) = '/';\n+\t\tfor (cp = ref; (ch = *cp) != 0; cp++) {\n+\t\t\tif (needs_quote(ch)) {\n+\t\t\t\t*dp++ = '%';\n+\t\t\t\t*dp++ = hex((ch >> 4) & 0xF);\n+\t\t\t\t*dp++ = hex(ch & 0xF);\n+\t\t\t}\n+\t\t\telse\n+\t\t\t\t*dp++ = ch;\n \t\t}\n-\t\telse\n-\t\t\t*dp++ = ch;\n \t}\n \t*dp = 0;\n \n@@ -611,9 +613,12 @@ int http_fetch_ref(const char *base, struct ref *ref)\n \tstruct strbuf buffer = STRBUF_INIT;\n \tstruct active_request_slot *slot;\n \tstruct slot_results results;\n+\tstruct strbuf suffix_buffer = STRBUF_INIT;\n \tint ret;\n \n-\turl = quote_ref_url(base, ref->name);\n+\tstrbuf_addf(&suffix_buffer, \"/%s\", ref->name);\n+\turl = transform_url(base, strbuf_detach(&suffix_buffer, NULL));\n+\turl = quote_ref_url(url, \"\");\n \tslot = get_active_slot();\n \tslot->results = &results;\n \tcurl_easy_setopt(slot->curl, CURLOPT_FILE, &buffer);\n@@ -643,3 +648,22 @@ int http_fetch_ref(const char *base, struct ref *ref)\n \tfree(url);\n \treturn ret;\n }\n+\n+char *transform_url(const char *url, const char *suffix)\n+{\n+\tstruct strbuf new_url = STRBUF_INIT;\n+\tchar *question_mark;\n+\tptrdiff_t offset;\n+\n+\tif ((question_mark = strchr(url, '?'))) {\n+\t\toffset = (ptrdiff_t) question_mark - (ptrdiff_t) url;\n+\t\tstrbuf_add(&new_url, url, offset);\n+\t\tstrbuf_addstr(&new_url, suffix);\n+\t\tstrbuf_addstr(&new_url, url + offset);\n+\t} else {\n+\t\tstrbuf_addstr(&new_url, url);\n+\t\tstrbuf_addstr(&new_url, suffix);\n+\t}\n+\treturn strbuf_detach(&new_url, NULL);\n+}\n+\ndiff --git a/http.h b/http.h\nindex a04fc6a..58730b8 100644\n--- a/http.h\n+++ b/http.h\n@@ -107,4 +107,6 @@ static inline int missing__target(int code, int result)\n \n extern int http_fetch_ref(const char *base, struct ref *ref);\n \n+extern char *transform_url(const char *url, const char *suffix);\n+\n #endif /* HTTP_H */\ndiff --git a/transport.c b/transport.c\nindex 3ff8519..b1966d8 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1,3 +1,4 @@\n+#include <stddef.h>\n #include \"cache.h\"\n #include \"transport.h\"\n #include \"run-command.h\"\n@@ -449,8 +450,7 @@ static struct ref *get_refs_via_curl(struct transport *transport)\n \n \twalker = transport->data;\n \n-\trefs_url = xmalloc(strlen(transport->url) + 11);\n-\tsprintf(refs_url, \"%s/info/refs\", transport->url);\n+\trefs_url = transform_url(transport->url, \"/info/refs\");\n \n \tslot = get_active_slot();\n \tslot->results = &results;\n@@ -833,3 +833,4 @@ int transport_disconnect(struct transport *transport)\n \tfree(transport);\n \treturn ret;\n }\n+\n-- \n1.5.5.3\n\n\nThanks,\n\n\tDavid\n"},{"id":"78711","messageId":"alpine.DEB.1.00.0806050758210.21190@racer","threadId":"13804","inReplyTo":"200806050848.43462.bombe@pterodactylus.net","subject":"Re: [PATCH] handle http urls with query string (\"?foo\") correctly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-06-05T07:15:01Z","receivedAt":"2008-06-05T07:15:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 5 Jun 2008, David ‘Bombe’ Roden wrote:\n\n> \n> ---\n> handle http urls with query string (\"?foo\") correctly\n> git breaks when a repository is cloned from an http url that contains a\n> query string. This patch fixes this behaviour be inserting the name of\n> the requested object (like \"/info/refs\") before the query string.\n\nUsually, this comes before the \"---\", and your comments/answers after it.  \nAnd the first line would be the subject:\n\n-- snip --\nHandle http urls with query string (\"?foo\") correctly\n\nGit breaks when a repository is cloned from an http url that contains a\nquery string. This patch fixes this behaviour be inserting the name of\nthe requested object (like \"/info/refs\") before the query string.\n-- snap --\n\nAnd of course, you usually sign off your patches.\n\n> diff --git a/http-walker.c b/http-walker.c\n> index 99f397e..b14497a 100644\n> --- a/http-walker.c\n> +++ b/http-walker.c\n> @@ -100,7 +100,6 @@ static void start_object_request(struct walker *walker,\n>  \tchar *hex = sha1_to_hex(obj_req->sha1);\n>  \tchar prevfile[PATH_MAX];\n>  \tchar *url;\n> -\tchar *posn;\n>  \tint prevlocal;\n>  \tunsigned char prev_buf[PREV_BUF_SIZE];\n>  \tssize_t prev_read = 0;\n> @@ -109,6 +108,7 @@ static void start_object_request(struct walker *walker,\n>  \tstruct curl_slist *range_header = NULL;\n>  \tstruct active_request_slot *slot;\n>  \tstruct walker_data *data = walker->data;\n> +\tstruct strbuf suffix_buffer = STRBUF_INIT;\n>  \n>  \tsnprintf(prevfile, sizeof(prevfile), \"%s.prev\", obj_req->filename);\n>  \tunlink(prevfile);\n> @@ -146,16 +146,10 @@ static void start_object_request(struct walker *walker,\n>  \n>  \tSHA1_Init(&obj_req->c);\n>  \n> -\turl = xmalloc(strlen(obj_req->repo->base) + 51);\n> +\tstrbuf_addstr(&suffix_buffer, \"/objects/\");\n> +\tstrbuf_addf(&suffix_buffer, \"%c%c/%s\", hex[0], hex[1], hex + 2);\n\nHow about\n\n\tstrbuf_addf(&suffix_buffer, \"/objects/%c%c/%s\",\n\t\thex[0], hex[1], hex + 2);\n\n?\n\n> +\turl = transform_url(obj_req->repo->base, strbuf_detach(&suffix_buffer, NULL));\n\nAnd this line wrapped, like so?\n\n\turl = transform_url(obj_req->repo->base,\n\t\tstrbuf_detach(&suffix_buffer, NULL));\n\nOh, but maybe it makes more sense to let transform_url() work on the \nstrbuf itself?\n\n> @@ -608,8 +603,7 @@ static void fetch_alternates(struct walker *walker, const char *base)\n>  \tif (walker->get_verbosely)\n>  \t\tfprintf(stderr, \"Getting alternates list for %s\\n\", base);\n>  \n> -\turl = xmalloc(strlen(base) + 31);\n> -\tsprintf(url, \"%s/objects/info/http-alternates\", base);\n> +\turl = transform_url(base, \"/objects/info/http-alternates\");\n>  \n>  \t/* Use a callback to process the result, since another request\n>  \t   may fail and need to have alternates loaded before continuing */\n> @@ -655,8 +649,7 @@ static int fetch_indices(struct walker *walker, struct alt_base *repo)\n>  \tif (walker->get_verbosely)\n>  \t\tfprintf(stderr, \"Getting pack list for %s\\n\", repo->base);\n>  \n> -\turl = xmalloc(strlen(repo->base) + 21);\n> -\tsprintf(url, \"%s/objects/info/packs\", repo->base);\n> +\turl = transform_url(repo->base, \"/objects/info/packs\");\n>  \n>  \tslot = get_active_slot();\n>  \tslot->results = &results;\n\nOf course, these two would have to initialise their strbuf, then.\n\n> diff --git a/http.c b/http.c\n> index 2a21ccb..a60f9ea 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -590,15 +590,17 @@ static char *quote_ref_url(const char *base, const char *ref)\n>  \tqref = xmalloc(len);\n>  \tmemcpy(qref, base, baselen);\n>  \tdp = qref + baselen;\n> -\t*(dp++) = '/';\n> -\tfor (cp = ref; (ch = *cp) != 0; cp++) {\n> -\t\tif (needs_quote(ch)) {\n> -\t\t\t*dp++ = '%';\n> -\t\t\t*dp++ = hex((ch >> 4) & 0xF);\n> -\t\t\t*dp++ = hex(ch & 0xF);\n> +\tif (*ref) {\n> +\t\t*(dp++) = '/';\n> +\t\tfor (cp = ref; (ch = *cp) != 0; cp++) {\n> +\t\t\tif (needs_quote(ch)) {\n> +\t\t\t\t*dp++ = '%';\n> +\t\t\t\t*dp++ = hex((ch >> 4) & 0xF);\n> +\t\t\t\t*dp++ = hex(ch & 0xF);\n> +\t\t\t}\n> +\t\t\telse\n> +\t\t\t\t*dp++ = ch;\n>  \t\t}\n> -\t\telse\n> -\t\t\t*dp++ = ch;\n>  \t}\n>  \t*dp = 0;\n>  \n\nIs this correct?  Either it should work the old way, or you will run into \nproblems when ref is not empty...\n\n> @@ -611,9 +613,12 @@ int http_fetch_ref(const char *base, struct ref *ref)\n>  \tstruct strbuf buffer = STRBUF_INIT;\n>  \tstruct active_request_slot *slot;\n>  \tstruct slot_results results;\n> +\tstruct strbuf suffix_buffer = STRBUF_INIT;\n>  \tint ret;\n>  \n> -\turl = quote_ref_url(base, ref->name);\n> +\tstrbuf_addf(&suffix_buffer, \"/%s\", ref->name);\n> +\turl = transform_url(base, strbuf_detach(&suffix_buffer, NULL));\n> +\turl = quote_ref_url(url, \"\");\n\n... and indeed, it seems that by now, you would not want to add the ref in \nquote_ref_url() anymore... which you do not, since you pass \"\" as ref \n(and this is the only caller of quote_ref_url).\n\n> @@ -643,3 +648,22 @@ int http_fetch_ref(const char *base, struct ref *ref)\n>  \tfree(url);\n>  \treturn ret;\n>  }\n> +\n> +char *transform_url(const char *url, const char *suffix)\n> +{\n> +\tstruct strbuf new_url = STRBUF_INIT;\n> +\tchar *question_mark;\n> +\tptrdiff_t offset;\n> +\n> +\tif ((question_mark = strchr(url, '?'))) {\n\nJunio usually prefers assignments outside of the if() statement.\n\n> +\t\toffset = (ptrdiff_t) question_mark - (ptrdiff_t) url;\n> +\t\tstrbuf_add(&new_url, url, offset);\n> +\t\tstrbuf_addstr(&new_url, suffix);\n> +\t\tstrbuf_addstr(&new_url, url + offset);\n> +\t} else {\n> +\t\tstrbuf_addstr(&new_url, url);\n> +\t\tstrbuf_addstr(&new_url, suffix);\n> +\t}\n> +\treturn strbuf_detach(&new_url, NULL);\n> +}\n> +\n\nThere is an extra empty line at the end.\n\nBut another thing is more pressing: obviously, there were reasons to quote \nthe ref names with this funny URL encoding (%45%75%6E%6E%79), and you \neffectively undid that change.\n\n> diff --git a/transport.c b/transport.c\n> index 3ff8519..b1966d8 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -1,3 +1,4 @@\n> +#include <stddef.h>\n\nWhy?\n\nCiao,\nDscho\n"},{"id":"78716","messageId":"7vtzg82u18.fsf@gitster.siamese.dyndns.org","threadId":"13804","inReplyTo":"alpine.DEB.1.00.0806050758210.21190@racer","subject":"Re: [PATCH] handle http urls with query string (\"?foo\") correctly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-05T07:57:39Z","receivedAt":"2008-06-05T07:57:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Usually, this comes before the \"---\", and your comments/answers after it.  \n> And the first line would be the subject:\n>\n> -- snip --\n> Handle http urls with query string (\"?foo\") correctly\n>\n> Git breaks when a repository is cloned from an http url that contains a\n> query string. This patch fixes this behaviour be inserting the name of\n> the requested object (like \"/info/refs\") before the query string.\n> -- snap --\n>\n> And of course, you usually sign off your patches.\n\nPlease wait a minute and step back.\n\nBefore going into the presentation, I have a strong doubt about what this\nis trying to solve.\n\nWithout reading the patch at all (and the lazyness is only half the reason\nfor not reading the patch before thinking about the issue --- it is also a\ngood lithmus test to make sure that the commit log explains what is done\nwell), my understanding is that this peculiar http-hosted git repository\ntakes:\n\n\thttp://foo.bar.xz/serve.cgi?repo=foo.git/\n\nas the base URL, and the patch author wants us to ask for (for example)\n\"info/alternates\" as\n\n\thttp://foo.bar.xz/serve.cgi/info/alternates?repo=foo.git/\n\nor something like that, not the usual:\n\n\thttp://foo.bar.xz/serve.cgi?repo=foo.git/info/alternates\n\nTwo comments.\n\n (1) If that is not the problem being tackled, the commit log needs to\n     explain the issue much better.  \"git breaks\" is obviously not good\n     enough to convey the issue to me, and if the description was not\n     clear for me to understand what is being fixed, it has no hope to\n     explain the fix to other people.\n\n (2) If that is indeed the issue being tackled, sorry, it is not how \"dumb\n     protocol\" http server is expected to behave.  Your server needs\n     fixing.\n\nIf the protocol being used is still the \"dumb commit walker\" protocol,\nthen, given the base URL of the repository $URL, \"info/refs\" must exist at\n\"$URL/info/refs\", and a loose object deadbeef... must exist at\n\"$URL/objects/de/adbeef...\".  That's how the protocol is defined.\n\nIf we want to have a CGI on the server side, the client _could_ even talk\n\"git native\" protocol or something similar to it.  But that is not what is\nattempted with this patch as far as I can tell.\n"},{"id":"78722","messageId":"200806051029.22580.bombe@pterodactylus.net","threadId":"13804","inReplyTo":"alpine.DEB.1.00.0806050758210.21190@racer","subject":"Re: [PATCH] handle http urls with query string (\"?foo\") correctly","fromName":"David ‘Bombe’ Roden","fromEmail":"bombe@pterodactylus.net","sentAt":"2008-06-05T08:29:16Z","receivedAt":"2008-06-05T08:29:16Z","isPatch":true,"sender":{"key":"bombe@pterodactylus.net","avatar":"https://gravatar.com/avatar/6845b170b5e882edcb27b015cdad0fb918f2d079c2cf77e668dcc71ae645ed87?d=mp&s=160"},"body":"On Thursday 05 June 2008 09:15:01 Johannes Schindelin wrote:\n\n> And of course, you usually sign off your patches.\n\nAs soon as there are no more objections, I will. :)\n\n\n> How about\n>\n> \tstrbuf_addf(&suffix_buffer, \"/objects/%c%c/%s\",\n> \t\thex[0], hex[1], hex + 2);\n>\n> ?\n\nNow that you mention it... ;) Changed.\n\n\n> And this line wrapped, like so?\n>\n> \turl = transform_url(obj_req->repo->base,\n> \t\tstrbuf_detach(&suffix_buffer, NULL));\n\nI usually don't do line-wrapping because all my terminals are wide enough but \nI might just make an exception here. :)\n\n\n> Oh, but maybe it makes more sense to let transform_url() work on the\n> strbuf itself?\n\nThen I would have to rewrite more unrelated stuff because everything else in \nthose files still relies on char*. I'd rather not meddle with that for now; I \nagree that at some point it should all be rewritten to use strbuf (more or \nless) exclusively. :)\n\n\n> Junio usually prefers assignments outside of the if() statement.\n\nCan do.\n\n\n> But another thing is more pressing: obviously, there were reasons to quote\n> the ref names with this funny URL encoding (%45%75%6E%6E%79), and you\n> effectively undid that change.\n\nHmm, yes. Ref names should probably be url-escaped.\n\n\n> > +#include <stddef.h>\n> Why?\n\nFor ptrdiff_t. Yes, I know that the offset between the start of a URL and the \nquestion mark will never really exceed the 32-bit range and it's also highly \nunlikely that the two pointers in comparison differ in the upper 32 bits but \nas I'm actually calculating the difference between two pointers ptrdiff_t was \nthe logical choice over int.\n\n\n> Dscho\n\n\tDavid\n"},{"id":"78723","messageId":"200806051037.58675.bombe@pterodactylus.net","threadId":"13804","inReplyTo":"7vtzg82u18.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] handle http urls with query string (\"?foo\") correctly","fromName":"David ‘Bombe’ Roden","fromEmail":"bombe@pterodactylus.net","sentAt":"2008-06-05T08:37:58Z","receivedAt":"2008-06-05T08:37:58Z","isPatch":true,"sender":{"key":"bombe@pterodactylus.net","avatar":"https://gravatar.com/avatar/6845b170b5e882edcb27b015cdad0fb918f2d079c2cf77e668dcc71ae645ed87?d=mp&s=160"},"body":"On Thursday 05 June 2008 09:57:39 Junio C Hamano wrote:\n\n> my understanding is that this peculiar http-hosted git repository\n> takes:\n> \thttp://foo.bar.xz/serve.cgi?repo=foo.git/\n> as the base URL, and the patch author wants us to ask for (for example)\n> \"info/alternates\" as\n> \thttp://foo.bar.xz/serve.cgi/info/alternates?repo=foo.git/\n\nActually the base URL is more like\n\n\thttp://foo.bar.xz/foo.git/0?type=text/plain\n\nand I want to retrieve objects with\n\n\thttp://foo.bar.xz/foo.git/0/info/refs?type=text/plain\n\nbut the gist is basically the same. My patch will indeed break git for URLs \nlike the one you gave.\n\n\n>  (2) If that is indeed the issue being tackled, sorry, it is not how \"dumb\n>      protocol\" http server is expected to behave.  Your server needs\n>      fixing.\n> If the protocol being used is still the \"dumb commit walker\" protocol,\n> then, given the base URL of the repository $URL, \"info/refs\" must exist at\n> \"$URL/info/refs\", and a loose object deadbeef... must exist at\n> \"$URL/objects/de/adbeef...\".  That's how the protocol is defined.\n\nIf that is indeed the case, you're right. That could probably be tackled by \nsome small fixes in Freenet's http-gateway. I'll talk to the other developers \nabout that.\n\n\nThanks,\n\n\tDavid\n"},{"id":"78754","messageId":"200806051415.02809.bombe@pterodactylus.net","threadId":"13804","inReplyTo":"7vtzg82u18.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] handle http urls with query string (\"?foo\") correctly","fromName":"David ‘Bombe’ Roden","fromEmail":"bombe@pterodactylus.net","sentAt":"2008-06-05T12:14:59Z","receivedAt":"2008-06-05T12:14:59Z","isPatch":true,"sender":{"key":"bombe@pterodactylus.net","avatar":"https://gravatar.com/avatar/6845b170b5e882edcb27b015cdad0fb918f2d079c2cf77e668dcc71ae645ed87?d=mp&s=160"},"body":"On Thursday 05 June 2008 09:57:39 Junio C Hamano wrote:\n\n> If the protocol being used is still the \"dumb commit walker\" protocol,\n> then, given the base URL of the repository $URL, \"info/refs\" must exist at\n> \"$URL/info/refs\", and a loose object deadbeef... must exist at\n> \"$URL/objects/de/adbeef...\".  That's how the protocol is defined.\n\nThrough a small patch in Freenet it is now possible to use the existing http \ntransport, without any modifications in git.\n\n\nThanks for all your help, anyway,\n\n\tDavid\n"}]}