threads / patch / 17350

patchhttp-push: refactor request url creation

Subject: [PATCH] http-push: refactor request url creation

## tl;dr

6 messages between Jan 25, 2009 and Jan 27, 2009. Diffs are folded; open one to read it.

replies: 5people: 4as markdown or json

Ray Chuan· Jan 25, 2009, 06:08 UTC · lore

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 <johannes.schindelin@gmx.de>
Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
* 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(-)
Show changes to http-push.c +19 −39
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· Jan 25, 2009, 20:35 UTC · re: Ray Chuan · lore

Re: [PATCH] http-push: refactor request url creation

Ray Chuan <rctay89@gmail.com> writes:
Show 9 quoted lines
> 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 <johannes.schindelin@gmx.de>
> Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>

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 (..."
Show 12 quoted lines
> @@ -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?

Show 9 quoted lines
> @@ -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· Jan 26, 2009, 01:52 UTC · re: Junio C Hamano · lore

Re: [PATCH] http-push: refactor request url creation

Hi,
On Mon, Jan 26, 2009 at 4:35 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 17 quoted lines
>> @@ -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· Jan 26, 2009, 08:02 UTC · re: Ray Chuan · lore

Re: [PATCH] http-push: refactor request url creation

On Mon, 26 Jan 2009, Ray Chuan wrote:
Show 8 quoted lines
>>> -     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· Jan 26, 2009, 10:41 UTC · re: Ray Chuan · lore

Re: [PATCH] http-push: refactor request url creation

Hi,
On Mon, 26 Jan 2009, Ray Chuan wrote:
Show 20 quoted lines
> On Mon, Jan 26, 2009 at 4:35 AM, Junio C Hamano <gitster@pobox.com> 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· Jan 27, 2009, 04:58 UTC · re: Johannes Schindelin · lore

Re: [PATCH] http-push: refactor request url creation

Johannes Schindelin <Johannes.Schindelin@gmx.de> 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.

← back to recent threads