{"thread":{"id":"17350","subject":"[PATCH] http-push: refactor request url creation","startedAt":"2009-01-25T06:08:57Z","lastAt":"2009-01-27T04:58:30Z","messageCount":6,"participants":["Ray Chuan","Junio C Hamano","Daniel Stenberg","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"101818","messageId":"be6fef0d0901242208p635264e5jc1f95d784cd51450@mail.gmail.com","threadId":"17350","inReplyTo":null,"subject":"[PATCH] http-push: refactor request url creation","fromName":"Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-01-25T06:08:57Z","receivedAt":"2009-01-25T06:08:57Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Currently, functions that deal with objects on the remote repository\nhave to allocate and do strcpys to generate the URL.\n\nThis patch saves them this trouble, by providing a function that\nreturns a URL: either the object's 2-digit hex directory (eg.\n/objects/a1/) or the complete object location (eg. /objects/a1/b2).\n\nAcked-by: Johannes Schindelin <johannes.schindelin@gmx.de>\nSigned-off-by: Tay Ray Chuan <rctay89@gmail.com>\n---\n\n* renamed only_two_digit_postfix in original patch to only_two_digit_prefix\n* rebased and generated on master (5dc1308)\n\n http-push.c |   58 +++++++++++++++++++---------------------------------------\n 1 files changed, 19 insertions(+), 39 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex eca4a8e..27825f2 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -208,6 +208,16 @@ static struct curl_slist\n*get_dav_token_headers(struct remote_lock *lock, enum d\n \treturn dav_headers;\n }\n\n+static char *get_remote_object_url(const char *url, const char *hex,\nint only_two_digit_prefix) {\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tstrbuf_addf(&buf, \"%sobjects/%.*s/\", url, 2, hex);\n+\tif(!only_two_digit_prefix)\n+\t\tstrbuf_addf(&buf, \"%s\", hex+2);\n+\n+\treturn strbuf_detach(&buf, NULL);\n+}\n+\n static void finish_request(struct transfer_request *request);\n static void release_request(struct transfer_request *request);\n\n@@ -253,8 +263,6 @@ static void start_fetch_loose(struct\ntransfer_request *request)\n \tchar *hex = sha1_to_hex(request->obj->sha1);\n \tchar *filename;\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@@ -304,17 +312,7 @@ static void start_fetch_loose(struct\ntransfer_request *request)\n\n \tgit_SHA1_Init(&request->c);\n\n-\turl = xmalloc(strlen(remote->url) + 50);\n-\trequest->url = xmalloc(strlen(remote->url) + 50);\n-\tstrcpy(url, remote->url);\n-\tposn = url + strlen(remote->url);\n-\tstrcpy(posn, \"objects/\");\n-\tposn += 8;\n-\tmemcpy(posn, hex, 2);\n-\tposn += 2;\n-\t*(posn++) = '/';\n-\tstrcpy(posn, hex + 2);\n-\tstrcpy(request->url, url);\n+\trequest->url = get_remote_object_url(remote->url, hex, 0);\n\n \t/* If a previous temp file is present, process what was already\n \t   fetched. */\n@@ -358,7 +356,7 @@ static void start_fetch_loose(struct\ntransfer_request *request)\n \tcurl_easy_setopt(slot->curl, CURLOPT_FILE, request);\n \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_sha1_file);\n \tcurl_easy_setopt(slot->curl, CURLOPT_ERRORBUFFER, request->errorstr);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_URL, url);\n+\tcurl_easy_setopt(slot->curl, CURLOPT_URL, request->url);\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, no_pragma_header);\n\n \t/* If we have successfully processed data from a previous fetch\n@@ -387,16 +385,8 @@ static void start_mkcol(struct transfer_request *request)\n {\n \tchar *hex = sha1_to_hex(request->obj->sha1);\n \tstruct active_request_slot *slot;\n-\tchar *posn;\n\n-\trequest->url = xmalloc(strlen(remote->url) + 13);\n-\tstrcpy(request->url, remote->url);\n-\tposn = request->url + strlen(remote->url);\n-\tstrcpy(posn, \"objects/\");\n-\tposn += 8;\n-\tmemcpy(posn, hex, 2);\n-\tposn += 2;\n-\tstrcpy(posn, \"/\");\n+\trequest->url = get_remote_object_url(remote->url, hex, 1);\n\n \tslot = get_active_slot();\n \tslot->callback_func = process_response;\n@@ -511,7 +501,7 @@ static void start_put(struct transfer_request *request)\n {\n \tchar *hex = sha1_to_hex(request->obj->sha1);\n \tstruct active_request_slot *slot;\n-\tchar *posn;\n+\tstruct strbuf url_buf = STRBUF_INIT;\n \tenum object_type type;\n \tchar hdr[50];\n \tvoid *unpacked;\n@@ -550,21 +540,11 @@ static void start_put(struct transfer_request *request)\n\n \trequest->buffer.buf.len = stream.total_out;\n\n-\trequest->url = xmalloc(strlen(remote->url) +\n-\t\t\t       strlen(request->lock->token) + 51);\n-\tstrcpy(request->url, remote->url);\n-\tposn = request->url + strlen(remote->url);\n-\tstrcpy(posn, \"objects/\");\n-\tposn += 8;\n-\tmemcpy(posn, hex, 2);\n-\tposn += 2;\n-\t*(posn++) = '/';\n-\tstrcpy(posn, hex + 2);\n-\trequest->dest = xmalloc(strlen(request->url) + 14);\n-\tsprintf(request->dest, \"Destination: %s\", request->url);\n-\tposn += 38;\n-\t*(posn++) = '_';\n-\tstrcpy(posn, request->lock->token);\n+\tstrbuf_addf(&url_buf, \"Destination: %s\",\nget_remote_object_url(remote->url, hex, 0));\n+\trequest->dest = strbuf_detach(&url_buf, NULL);\n+\n+\tstrbuf_addf(&url_buf, \"%s_%s\", get_remote_object_url(remote->url,\nhex, 0), request->lock->token);\n+\trequest->url = strbuf_detach(&url_buf, NULL);\n\n \tslot = get_active_slot();\n \tslot->callback_func = process_response;\n-- \n1.6.0.4\n"},{"id":"101868","messageId":"7vpribdszr.fsf@gitster.siamese.dyndns.org","threadId":"17350","inReplyTo":"be6fef0d0901242208p635264e5jc1f95d784cd51450@mail.gmail.com","subject":"Re: [PATCH] http-push: refactor request url creation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-25T20:35:04Z","receivedAt":"2009-01-25T20:35:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ray Chuan <rctay89@gmail.com> writes:\n\n> Currently, functions that deal with objects on the remote repository\n> have to allocate and do strcpys to generate the URL.\n>\n> This patch saves them this trouble, by providing a function that\n> returns a URL: either the object's 2-digit hex directory (eg.\n> /objects/a1/) or the complete object location (eg. /objects/a1/b2).\n>\n> Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>\n\nIf you wrote it, and then Johannes vetted it, then these two should be in\nthe opposite order.\n\n> +static char *get_remote_object_url(const char *url, const char *hex,\n> int only_two_digit_prefix) {\n\nLinewrapped.\n\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\n> +\tstrbuf_addf(&buf, \"%sobjects/%.*s/\", url, 2, hex);\n> +\tif(!only_two_digit_prefix)\n\n\"if (...\"\n\n> @@ -304,17 +312,7 @@ static void start_fetch_loose(struct\n> transfer_request *request)\n>\n>  \tgit_SHA1_Init(&request->c);\n>\n> -\turl = xmalloc(strlen(remote->url) + 50);\n> ...\n> -\tstrcpy(request->url, url);\n> +\trequest->url = get_remote_object_url(remote->url, hex, 0);\n> ...\n> -\tcurl_easy_setopt(slot->curl, CURLOPT_URL, url);\n> +\tcurl_easy_setopt(slot->curl, CURLOPT_URL, request->url);\n\nThe original code gave a separate \"url\" to setop() but this gives the same\nstring.  Does curl_easy_setop() copies the given string away?  IOW is this\nchange safe?\n\n> @@ -550,21 +540,11 @@ static void start_put(struct transfer_request *request)\n>\n>  \trequest->buffer.buf.len = stream.total_out;\n>\n> -\trequest->url = xmalloc(strlen(remote->url) +\n> ...\n> -\tstrcpy(posn, request->lock->token);\n> +\tstrbuf_addf(&url_buf, \"Destination: %s\",\n> get_remote_object_url(remote->url, hex, 0));\n\nLinewrapped.\n\nThe return value from get_remote_object_url() leaks here.\n\n> +\trequest->dest = strbuf_detach(&url_buf, NULL);\n> +\n> +\tstrbuf_addf(&url_buf, \"%s_%s\", get_remote_object_url(remote->url,\n> hex, 0), request->lock->token);\n\nThe return value from get_remote_object_url() leaks here, too.\n"},{"id":"101930","messageId":"be6fef0d0901251752p5b34c053pb24dce8a35b06fce@mail.gmail.com","threadId":"17350","inReplyTo":"7vpribdszr.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] http-push: refactor request url creation","fromName":"Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-01-26T01:52:22Z","receivedAt":"2009-01-26T01:52:22Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Mon, Jan 26, 2009 at 4:35 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> @@ -304,17 +312,7 @@ static void start_fetch_loose(struct\n>> transfer_request *request)\n>>\n>>       git_SHA1_Init(&request->c);\n>>\n>> -     url = xmalloc(strlen(remote->url) + 50);\n>> ...\n>> -     strcpy(request->url, url);\n>> +     request->url = get_remote_object_url(remote->url, hex, 0);\n>> ...\n>> -     curl_easy_setopt(slot->curl, CURLOPT_URL, url);\n>> +     curl_easy_setopt(slot->curl, CURLOPT_URL, request->url);\n>\n> The original code gave a separate \"url\" to setop() but this gives the same\n> string.  Does curl_easy_setop() copies the given string away?  IOW is this\n> change safe?\n>\n\ncurl strdup's it, so this is safe.\n\nThe stack is like this:\n\ncurl_easy_setopt\ncalls Curl_setopt (at 391)\ncalls setstropt (at 1566)\ncalls strdup (at 276).\n\nhttp://cool.haxx.se/cvs.cgi/curl/lib/easy.c?annotate=1.132 (for\ncurl_easy_setopt)\nhttp://cool.haxx.se/cvs.cgi/curl/lib/url.c?annotate=1.782 (for\nCurl_setopt, setstropt)\n\n-- \nCheers,\nRay Chuan\n"},{"id":"101949","messageId":"alpine.DEB.1.10.0901260901320.32450@yvahk2.pbagnpgbe.fr","threadId":"17350","inReplyTo":"be6fef0d0901251752p5b34c053pb24dce8a35b06fce@mail.gmail.com","subject":"Re: [PATCH] http-push: refactor request url creation","fromName":"Daniel Stenberg","fromEmail":"daniel@haxx.se","sentAt":"2009-01-26T08:02:57Z","receivedAt":"2009-01-26T08:02:57Z","isPatch":true,"sender":{"key":"daniel@haxx.se","avatar":"https://gravatar.com/avatar/69fdca87edd17cee21ca2e79fc2ff671d644603c3dc27167430f3cd3dbab7ba8?d=mp&s=160"},"body":"On Mon, 26 Jan 2009, Ray Chuan wrote:\n\n>>> -     curl_easy_setopt(slot->curl, CURLOPT_URL, url);\n>>> +     curl_easy_setopt(slot->curl, CURLOPT_URL, request->url);\n>>\n>> The original code gave a separate \"url\" to setop() but this gives the same\n>> string.  Does curl_easy_setop() copies the given string away?  IOW is this\n>> change safe?\n>\n> curl strdup's it, so this is safe.\n\nI'm not sure what the oldest possibly libcurl version git can deal with, but \nhere's a related quote from the curl_easy_setopt man page:\n\n        NOTE: before 7.17.0 strings were  not  copied.  Instead  the  user  was\n        forced keep them available until libcurl no longer needed them.\n\n-- \n\n  / daniel.haxx.se\n"},{"id":"101958","messageId":"alpine.DEB.1.00.0901261141070.14855@racer","threadId":"17350","inReplyTo":"be6fef0d0901251752p5b34c053pb24dce8a35b06fce@mail.gmail.com","subject":"Re: [PATCH] http-push: refactor request url creation","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-26T10:41:30Z","receivedAt":"2009-01-26T10:41:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 26 Jan 2009, Ray Chuan wrote:\n\n> On Mon, Jan 26, 2009 at 4:35 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> >> @@ -304,17 +312,7 @@ static void start_fetch_loose(struct\n> >> transfer_request *request)\n> >>\n> >>       git_SHA1_Init(&request->c);\n> >>\n> >> -     url = xmalloc(strlen(remote->url) + 50);\n> >> ...\n> >> -     strcpy(request->url, url);\n> >> +     request->url = get_remote_object_url(remote->url, hex, 0);\n> >> ...\n> >> -     curl_easy_setopt(slot->curl, CURLOPT_URL, url);\n> >> +     curl_easy_setopt(slot->curl, CURLOPT_URL, request->url);\n> >\n> > The original code gave a separate \"url\" to setop() but this gives the same\n> > string.  Does curl_easy_setop() copies the given string away?  IOW is this\n> > change safe?\n> >\n> \n> curl strdup's it, so this is safe.\n\nI might have mentioned that things like this _need_ to go into the commit \nmessage.\n\nCiao,\nDscho\n"},{"id":"102084","messageId":"7vskn51h1l.fsf@gitster.siamese.dyndns.org","threadId":"17350","inReplyTo":"alpine.DEB.1.00.0901261141070.14855@racer","subject":"Re: [PATCH] http-push: refactor request url creation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-27T04:58:30Z","receivedAt":"2009-01-27T04:58:30Z","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>> curl strdup's it, so this is safe.\n>\n> I might have mentioned that things like this _need_ to go into the commit \n> message.\n\nYeah, and let's not forget advice from Daniel Stenberg.\n\n    I'm not sure what the oldest possibly libcurl version git can deal\n    with, but here's a related quote from the curl_easy_setopt man page:\n\n           NOTE: before 7.17.0 strings were  not  copied.  Instead  the  user  was\n           forced keep them available until libcurl no longer needed them.\n"}]}