# [PATCH] http-push: refactor request url creation 6 messages from 2009-01-25 to 2009-01-27. Participants: Ray Chuan, Junio C Hamano, Daniel Stenberg, Johannes Schindelin. Thread: https://gitlist.dev/t/17350 ## Ray Chuan, 2009-01-25 06:08 Subject: [PATCH] http-push: refactor request url creation Message-ID: URL: https://gitlist.dev/e/be6fef0d0901242208p635264e5jc1f95d784cd51450%40mail.gmail.com ``` Currently, functions that deal with objects on the remote repository have to allocate and do strcpys to generate the URL. This patch saves them this trouble, by providing a function that returns a URL: either the object's 2-digit hex directory (eg. /objects/a1/) or the complete object location (eg. /objects/a1/b2). Acked-by: Johannes Schindelin Signed-off-by: Tay Ray Chuan --- * renamed only_two_digit_postfix in original patch to only_two_digit_prefix * rebased and generated on master (5dc1308) http-push.c | 58 +++++++++++++++++++--------------------------------------- 1 files changed, 19 insertions(+), 39 deletions(-) diff --git a/http-push.c b/http-push.c index eca4a8e..27825f2 100644 --- a/http-push.c +++ b/http-push.c @@ -208,6 +208,16 @@ static struct curl_slist *get_dav_token_headers(struct remote_lock *lock, enum d return dav_headers; } +static char *get_remote_object_url(const char *url, const char *hex, int only_two_digit_prefix) { + struct strbuf buf = STRBUF_INIT; + + strbuf_addf(&buf, "%sobjects/%.*s/", url, 2, hex); + if(!only_two_digit_prefix) + strbuf_addf(&buf, "%s", hex+2); + + return strbuf_detach(&buf, NULL); +} + static void finish_request(struct transfer_request *request); static void release_request(struct transfer_request *request); @@ -253,8 +263,6 @@ static void start_fetch_loose(struct transfer_request *request) char *hex = sha1_to_hex(request->obj->sha1); char *filename; char prevfile[PATH_MAX]; - char *url; - char *posn; int prevlocal; unsigned char prev_buf[PREV_BUF_SIZE]; ssize_t prev_read = 0; @@ -304,17 +312,7 @@ static void start_fetch_loose(struct transfer_request *request) git_SHA1_Init(&request->c); - url = xmalloc(strlen(remote->url) + 50); - request->url = xmalloc(strlen(remote->url) + 50); - strcpy(url, remote->url); - posn = url + strlen(remote->url); - strcpy(posn, "objects/"); - posn += 8; - memcpy(posn, hex, 2); - posn += 2; - *(posn++) = '/'; - strcpy(posn, hex + 2); - strcpy(request->url, url); + request->url = get_remote_object_url(remote->url, hex, 0); /* If a previous temp file is present, process what was already fetched. */ @@ -358,7 +356,7 @@ static void start_fetch_loose(struct transfer_request *request) curl_easy_setopt(slot->curl, CURLOPT_FILE, request); curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_sha1_file); curl_easy_setopt(slot->curl, CURLOPT_ERRORBUFFER, request->errorstr); - curl_easy_setopt(slot->curl, CURLOPT_URL, url); + curl_easy_setopt(slot->curl, CURLOPT_URL, request->url); curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, no_pragma_header); /* If we have successfully processed data from a previous fetch @@ -387,16 +385,8 @@ static void start_mkcol(struct transfer_request *request) { char *hex = sha1_to_hex(request->obj->sha1); struct active_request_slot *slot; - char *posn; - request->url = xmalloc(strlen(remote->url) + 13); - strcpy(request->url, remote->url); - posn = request->url + strlen(remote->url); - strcpy(posn, "objects/"); - posn += 8; - memcpy(posn, hex, 2); - posn += 2; - strcpy(posn, "/"); + request->url = get_remote_object_url(remote->url, hex, 1); slot = get_active_slot(); slot->callback_func = process_response; @@ -511,7 +501,7 @@ static void start_put(struct transfer_request *request) { char *hex = sha1_to_hex(request->obj->sha1); struct active_request_slot *slot; - char *posn; + struct strbuf url_buf = STRBUF_INIT; enum object_type type; char hdr[50]; void *unpacked; @@ -550,21 +540,11 @@ static void start_put(struct transfer_request *request) request->buffer.buf.len = stream.total_out; - request->url = xmalloc(strlen(remote->url) + - strlen(request->lock->token) + 51); - strcpy(request->url, remote->url); - posn = request->url + strlen(remote->url); - strcpy(posn, "objects/"); - posn += 8; - memcpy(posn, hex, 2); - posn += 2; - *(posn++) = '/'; - strcpy(posn, hex + 2); - request->dest = xmalloc(strlen(request->url) + 14); - sprintf(request->dest, "Destination: %s", request->url); - posn += 38; - *(posn++) = '_'; - strcpy(posn, request->lock->token); + strbuf_addf(&url_buf, "Destination: %s", get_remote_object_url(remote->url, hex, 0)); + request->dest = strbuf_detach(&url_buf, NULL); + + strbuf_addf(&url_buf, "%s_%s", get_remote_object_url(remote->url, hex, 0), request->lock->token); + request->url = strbuf_detach(&url_buf, NULL); slot = get_active_slot(); slot->callback_func = process_response; -- 1.6.0.4 ``` ## Junio C Hamano, 2009-01-25 20:35 Subject: Re: [PATCH] http-push: refactor request url creation Message-ID: <7vpribdszr.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vpribdszr.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Ray Chuan writes: > Currently, functions that deal with objects on the remote repository > have to allocate and do strcpys to generate the URL. > > This patch saves them this trouble, by providing a function that > returns a URL: either the object's 2-digit hex directory (eg. > /objects/a1/) or the complete object location (eg. /objects/a1/b2). > > Acked-by: Johannes Schindelin > Signed-off-by: Tay Ray Chuan If you wrote it, and then Johannes vetted it, then these two should be in the opposite order. > +static char *get_remote_object_url(const char *url, const char *hex, > int only_two_digit_prefix) { Linewrapped. > + struct strbuf buf = STRBUF_INIT; > + > + strbuf_addf(&buf, "%sobjects/%.*s/", url, 2, hex); > + if(!only_two_digit_prefix) "if (..." > @@ -304,17 +312,7 @@ static void start_fetch_loose(struct > transfer_request *request) > > git_SHA1_Init(&request->c); > > - url = xmalloc(strlen(remote->url) + 50); > ... > - strcpy(request->url, url); > + request->url = get_remote_object_url(remote->url, hex, 0); > ... > - curl_easy_setopt(slot->curl, CURLOPT_URL, url); > + curl_easy_setopt(slot->curl, CURLOPT_URL, request->url); The original code gave a separate "url" to setop() but this gives the same string. Does curl_easy_setop() copies the given string away? IOW is this change safe? > @@ -550,21 +540,11 @@ static void start_put(struct transfer_request *request) > > request->buffer.buf.len = stream.total_out; > > - request->url = xmalloc(strlen(remote->url) + > ... > - strcpy(posn, request->lock->token); > + strbuf_addf(&url_buf, "Destination: %s", > get_remote_object_url(remote->url, hex, 0)); Linewrapped. The return value from get_remote_object_url() leaks here. > + request->dest = strbuf_detach(&url_buf, NULL); > + > + strbuf_addf(&url_buf, "%s_%s", get_remote_object_url(remote->url, > hex, 0), request->lock->token); The return value from get_remote_object_url() leaks here, too. ``` ## Ray Chuan, 2009-01-26 01:52 Subject: Re: [PATCH] http-push: refactor request url creation Message-ID: URL: https://gitlist.dev/e/be6fef0d0901251752p5b34c053pb24dce8a35b06fce%40mail.gmail.com In-Reply-To: <7vpribdszr.fsf@gitster.siamese.dyndns.org> ``` Hi, On Mon, Jan 26, 2009 at 4:35 AM, Junio C Hamano wrote: >> @@ -304,17 +312,7 @@ static void start_fetch_loose(struct >> transfer_request *request) >> >> git_SHA1_Init(&request->c); >> >> - url = xmalloc(strlen(remote->url) + 50); >> ... >> - strcpy(request->url, url); >> + request->url = get_remote_object_url(remote->url, hex, 0); >> ... >> - curl_easy_setopt(slot->curl, CURLOPT_URL, url); >> + curl_easy_setopt(slot->curl, CURLOPT_URL, request->url); > > The original code gave a separate "url" to setop() but this gives the same > string. Does curl_easy_setop() copies the given string away? IOW is this > change safe? > curl strdup's it, so this is safe. The stack is like this: curl_easy_setopt calls Curl_setopt (at 391) calls setstropt (at 1566) calls strdup (at 276). http://cool.haxx.se/cvs.cgi/curl/lib/easy.c?annotate=1.132 (for curl_easy_setopt) http://cool.haxx.se/cvs.cgi/curl/lib/url.c?annotate=1.782 (for Curl_setopt, setstropt) -- Cheers, Ray Chuan ``` ## Daniel Stenberg, 2009-01-26 08:02 Subject: Re: [PATCH] http-push: refactor request url creation Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.10.0901260901320.32450%40yvahk2.pbagnpgbe.fr In-Reply-To: ``` On Mon, 26 Jan 2009, Ray Chuan wrote: >>> - curl_easy_setopt(slot->curl, CURLOPT_URL, url); >>> + curl_easy_setopt(slot->curl, CURLOPT_URL, request->url); >> >> The original code gave a separate "url" to setop() but this gives the same >> string. Does curl_easy_setop() copies the given string away? IOW is this >> change safe? > > curl strdup's it, so this is safe. I'm not sure what the oldest possibly libcurl version git can deal with, but here's a related quote from the curl_easy_setopt man page: NOTE: before 7.17.0 strings were not copied. Instead the user was forced keep them available until libcurl no longer needed them. -- / daniel.haxx.se ``` ## Johannes Schindelin, 2009-01-26 10:41 Subject: Re: [PATCH] http-push: refactor request url creation Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0901261141070.14855%40racer In-Reply-To: ``` Hi, On Mon, 26 Jan 2009, Ray Chuan wrote: > On Mon, Jan 26, 2009 at 4:35 AM, Junio C Hamano wrote: > >> @@ -304,17 +312,7 @@ static void start_fetch_loose(struct > >> transfer_request *request) > >> > >> git_SHA1_Init(&request->c); > >> > >> - url = xmalloc(strlen(remote->url) + 50); > >> ... > >> - strcpy(request->url, url); > >> + request->url = get_remote_object_url(remote->url, hex, 0); > >> ... > >> - curl_easy_setopt(slot->curl, CURLOPT_URL, url); > >> + curl_easy_setopt(slot->curl, CURLOPT_URL, request->url); > > > > The original code gave a separate "url" to setop() but this gives the same > > string. Does curl_easy_setop() copies the given string away? IOW is this > > change safe? > > > > curl strdup's it, so this is safe. I might have mentioned that things like this _need_ to go into the commit message. Ciao, Dscho ``` ## Junio C Hamano, 2009-01-27 04:58 Subject: Re: [PATCH] http-push: refactor request url creation Message-ID: <7vskn51h1l.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vskn51h1l.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Johannes Schindelin writes: >> curl strdup's it, so this is safe. > > I might have mentioned that things like this _need_ to go into the commit > message. Yeah, and let's not forget advice from Daniel Stenberg. I'm not sure what the oldest possibly libcurl version git can deal with, but here's a related quote from the curl_easy_setopt man page: NOTE: before 7.17.0 strings were not copied. Instead the user was forced keep them available until libcurl no longer needed them. ```