{"thread":{"id":"17420","subject":"[PATCH] http-push: refactor request url creation","startedAt":"2009-01-28T22:19:43Z","lastAt":"2009-01-29T11:49:13Z","messageCount":4,"participants":["Tay Ray Chuan","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"102362","messageId":"4980D9FF.7020307@gmail.com","threadId":"17420","inReplyTo":null,"subject":"[PATCH] http-push: refactor request url creation","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-01-28T22:19:43Z","receivedAt":"2009-01-28T22:19:43Z","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 have to allocate and do strcpys to generate the URL.\n\nThis patch saves them this trouble, by providing two functions, \"append_remote_object_url\" and \"get_remote_object_url\".\n\nBoth generate a URL, with either the object's 2-digit hex directory (eg. /objects/a1/), or the complete object location (eg. /objects/a1/b2).\n\nHowever, they differ in that \"append_remote_object_url\" appends this URL to a strbuf, while \"get_remote_object_url\" wraps around the former and returns the URL directly in char*. Users usually would use \"get_remote_object_url\", but may find \"append_remote_object_url\" useful if they require further string operations on the URL.\n\nPS. In \"start_fetch_loose\", the variable \"url\" which is passed to \"curl_easy_setopt\" is removed, and in its place \"request->url\" is used. This is safe, since curl strdup's it.\n\nSigned-off-by: Tay Ray Chuan \nAcked-by: Johannes Schindelin \nAcked-by: Junio C Hamano \n---\n\n* renamed only_two_digit_postfix in original patch to only_two_digit_prefix\n* rebased and generated on master (5dc1308)\n* updated with Junio's changes (if (...) and fix memory leak)\n* updated with Junio's double interface\n* rebased and generated on master (297f6a5)\n\n http-push.c | 64 +++++++++++++++++++++++-----------------------------------\n 1 files changed, 25 insertions(+), 39 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex 59037df..6511d19 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -209,6 +209,20 @@ static struct curl_slist *get_dav_token_headers(struct remote_lock *lock, enum d\n \treturn dav_headers;\n }\n\n+static void append_remote_object_url(struct strbuf *buf, const char *url, const char *hex, int only_two_digit_prefix)\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+\n+static char *get_remote_object_url(const char *url, const char *hex, int only_two_digit_prefix)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tappend_remote_object_url(&buf, url, hex, only_two_digit_prefix);\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@@ -254,8 +268,6 @@ static void start_fetch_loose(struct transfer_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@@ -305,17 +317,7 @@ static void start_fetch_loose(struct transfer_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@@ -359,7 +361,7 @@ static void start_fetch_loose(struct transfer_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@@ -388,16 +390,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@@ -512,7 +506,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 buf = STRBUF_INIT;\n \tenum object_type type;\n \tchar hdr[50];\n \tvoid *unpacked;\n@@ -551,21 +545,13 @@ 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_addstr(&buf, \"Destination: \");\n+\tappend_remote_object_url(&buf, remote->url, hex, 0);\n+\trequest->dest = strbuf_detach(&buf, NULL);\n+\n+\tappend_remote_object_url(&buf, remote->url, hex, 0);\n+\tstrbuf_addstr(&buf, request->lock->token);\n+\trequest->url = strbuf_detach(&buf, NULL);\n\n \tslot = get_active_slot();\n \tslot->callback_func = process_response;\n-- \n1.6.1.1.251.g0dfa1\n"},{"id":"102365","messageId":"7vvdrzjb80.fsf@gitster.siamese.dyndns.org","threadId":"17420","inReplyTo":"4980D9FF.7020307@gmail.com","subject":"Re: [PATCH] http-push: refactor request url creation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-28T22:51:27Z","receivedAt":"2009-01-28T22:51:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tay Ray Chuan writes:\n\n> Currently, functions that deal with objects on the remote repository have to allocate and do strcpys to generate the URL.\n>\n> This patch saves them this trouble, by providing two functions, \"append_remote_object_url\" and \"get_remote_object_url\".\n>\n> Both generate a URL, with either the object's 2-digit hex directory (eg. /objects/a1/), or the complete object location (eg. /objects/a1/b2).\n>\n> However, they differ in that \"append_remote_object_url\" appends this URL to a strbuf, while \"get_remote_object_url\" wraps around the former and returns the URL directly in char*. Users usually would use \"get_remote_object_url\", but may find \"append_remote_object_url\" useful if they require further string operations on the URL.\n>\n> PS. In \"start_fetch_loose\", the variable \"url\" which is passed to \"curl_easy_setopt\" is removed, and in its place \"request->url\" is used. This is safe, since curl strdup's it.\n>\n> Signed-off-by: Tay Ray Chuan \n> Acked-by: Johannes Schindelin \n> Acked-by: Junio C Hamano \n\nWhat's with these loooooooooooooooooooooooooooooong lines?\n\nI thought at least you did not have these overlong lines in your earlier\nattempts, and Dscho may have acked one of those, but I doubt he would give\nhis Ack to this one. I certainly wouldn't Ack it myself.\n\nBy the way, aren't you sending format=\"flowed\"? Please don't. It damages\nwhitespaces.\n\nDaniel Stenberg did a research on the safety of your \"since curl stdrup's\nit\" claim, and found that it unsafe for earlier versions of the library\nbefore 7.17.0.\n\nIt seems that we earlier found out that anything older than 7.16 were not\nusable for git-http-push (see Release Notes for 1.5.4), but 7.16 is still\nolder than 7.17, so either we declare you _must_ have 7.17 or newer to use\nhttp-push, or keep an extra copy around and free it later like the\noriginal code does.\n\nEven Debian is at 7.18.2 these days, so requiring 7.17 or newer may not be\nan issue in practice, but there are people who keep running things on\nolder distros with proven stability (and known features limitation).\n\nThe refactoring looked sane otherwise, but I think we would want to opt\nfor safety by keeping an extra string around.\n\nThanks.\n"},{"id":"102366","messageId":"alpine.DEB.1.00.0901290005200.3586@pacific.mpi-cbg.de","threadId":"17420","inReplyTo":"7vvdrzjb80.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] http-push: refactor request url creation","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-28T23:06:19Z","receivedAt":"2009-01-28T23:06:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 28 Jan 2009, Junio C Hamano wrote:\n\n> I thought at least you did not have these overlong lines in your earlier \n> attempts, and Dscho may have acked one of those, but I doubt he would \n> give his Ack to this one. I certainly wouldn't Ack it myself.\n\nMy understanding is that the ACK is sent to the mailing list by the Acker \nand in response to the particular patch she is Acking.\n\n> The refactoring looked sane otherwise, but I think we would want to opt \n> for safety by keeping an extra string around.\n\nYep. Better safe 'n sorry.\n\nCiao,\nDscho\n"},{"id":"102435","messageId":"be6fef0d0901290349i65d75dfel49dda4ce0becf970@mail.gmail.com","threadId":"17420","inReplyTo":"4980D9FF.7020307@gmail.com","subject":"Re: [PATCH] http-push: refactor request url creation","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-01-29T11:49:13Z","receivedAt":"2009-01-29T11:49:13Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Thu, Jan 29, 2009 at 6:51 AM, Junio C Hamano wrote:\n> What's with these loooooooooooooooooooooooooooooong lines?\n>\n> I thought at least you did not have these overlong lines in your earlier\n> attempts, and Dscho may have acked one of those, but I doubt he would give\n> his Ack to this one. I certainly wouldn't Ack it myself.\n>\n> By the way, aren't you sending format=\"flowed\"? Please don't. It damages\n> whitespaces.\n\nI used Thunderbird since Gmail kept wrapping lines in the patch, guess\ni\"ll have to manually wrap the commit lines.\n\n> Daniel Stenberg did a research on the safety of your \"since curl stdrup's\n> it\" claim, and found that it unsafe for earlier versions of the library\n> before 7.17.0.\n>\n> It seems that we earlier found out that anything older than 7.16 were not\n> usable for git-http-push (see Release Notes for 1.5.4), but 7.16 is still\n> older than 7.17, so either we declare you _must_ have 7.17 or newer to use\n> http-push, or keep an extra copy around and free it later like the\n> original code does.\n>\n> Even Debian is at 7.18.2 these days, so requiring 7.17 or newer may not be\n> an issue in practice, but there are people who keep running things on\n> older distros with proven stability (and known features limitation).\n>\n> The refactoring looked sane otherwise, but I think we would want to opt\n> for safety by keeping an extra string around.\n\nHmm, since that string won't be released in start_fetch_loose, or\nanywhere else, would this be considered a memory leak?\n\n-- \nCheers,\nRay Chuan\n"}]}