[PATCH] http-push: refactor request url creation

[PATCH] http-push: refactor request url creation

[Ray Chuan]

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(-)

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

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

[Junio C Hamano]

Ray Chuan <rctay89@gmail.com> 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 <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 (..."

> @@ -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.

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

[Ray Chuan]

Hi,

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.

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

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

[Daniel Stenberg]

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

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

[Johannes Schindelin]

Hi,

On Mon, 26 Jan 2009, Ray Chuan wrote:

> 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

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

[Junio C Hamano]

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.

[PATCH] http-push: refactor request url creation

[Tay Ray Chuan]

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 two functions, "append_remote_object_url" and "get_remote_object_url".

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).

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.

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.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Acked-by: Junio C Hamano <gitster@pobox.com>
---

* renamed only_two_digit_postfix in original patch to only_two_digit_prefix
* rebased and generated on master (5dc1308)
* updated with Junio's changes (if (...) and fix memory leak)
* updated with Junio's double interface
* rebased and generated on master (297f6a5)

  http-push.c |   64 +++++++++++++++++++++++-----------------------------------
  1 files changed, 25 insertions(+), 39 deletions(-)

diff --git a/http-push.c b/http-push.c
index 59037df..6511d19 100644
--- a/http-push.c
+++ b/http-push.c
@@ -209,6 +209,20 @@ static struct curl_slist *get_dav_token_headers(struct remote_lock *lock, enum d
  	return dav_headers;
  }

+static void append_remote_object_url(struct strbuf *buf, const char *url, const char *hex, int only_two_digit_prefix)
+{
+	strbuf_addf(buf, "%sobjects/%.*s/", url, 2, hex);
+	if (!only_two_digit_prefix)
+		strbuf_addf(buf, "%s", hex+2);
+}
+
+static char *get_remote_object_url(const char *url, const char *hex, int only_two_digit_prefix)
+{
+	struct strbuf buf = STRBUF_INIT;
+	append_remote_object_url(&buf, url, hex, only_two_digit_prefix);
+	return strbuf_detach(&buf, NULL);
+}
+
  static void finish_request(struct transfer_request *request);
  static void release_request(struct transfer_request *request);

@@ -254,8 +268,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;
@@ -305,17 +317,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. */
@@ -359,7 +361,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
@@ -388,16 +390,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;
@@ -512,7 +506,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 buf = STRBUF_INIT;
  	enum object_type type;
  	char hdr[50];
  	void *unpacked;
@@ -551,21 +545,13 @@ 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_addstr(&buf, "Destination: ");
+	append_remote_object_url(&buf, remote->url, hex, 0);
+	request->dest = strbuf_detach(&buf, NULL);
+
+	append_remote_object_url(&buf, remote->url, hex, 0);
+	strbuf_addstr(&buf, request->lock->token);
+	request->url = strbuf_detach(&buf, NULL);

  	slot = get_active_slot();
  	slot->callback_func = process_response;
-- 
1.6.1.1.251.g0dfa1

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

[Junio C Hamano]

Tay Ray Chuan <rctay89@gmail.com> 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 two functions, "append_remote_object_url" and "get_remote_object_url".
>
> 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).
>
> 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.
>
> 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.
>
> Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
> Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Acked-by: Junio C Hamano <gitster@pobox.com>

What's with these loooooooooooooooooooooooooooooong lines?

I thought at least you did not have these overlong lines in your earlier
attempts, and Dscho may have acked one of those, but I doubt he would give
his Ack to this one.  I certainly wouldn't Ack it myself.

By the way, aren't you sending format="flowed"?  Please don't.  It damages
whitespaces.

Daniel Stenberg did a research on the safety of your "since curl stdrup's
it" claim, and found that it unsafe for earlier versions of the library
before 7.17.0.

It seems that we earlier found out that anything older than 7.16 were not
usable for git-http-push (see Release Notes for 1.5.4), but 7.16 is still
older than 7.17, so either we declare you _must_ have 7.17 or newer to use
http-push, or keep an extra copy around and free it later like the
original code does.

Even Debian is at 7.18.2 these days, so requiring 7.17 or newer may not be
an issue in practice, but there are people who keep running things on
older distros with proven stability (and known features limitation).

The refactoring looked sane otherwise, but I think we would want to opt
for safety by keeping an extra string around.

Thanks.

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

[Johannes Schindelin]

Hi,

On Wed, 28 Jan 2009, Junio C Hamano wrote:

> I thought at least you did not have these overlong lines in your earlier 
> attempts, and Dscho may have acked one of those, but I doubt he would 
> give his Ack to this one.  I certainly wouldn't Ack it myself.

My understanding is that the ACK is sent to the mailing list by the Acker 
and in response to the particular patch she is Acking.

> The refactoring looked sane otherwise, but I think we would want to opt 
> for safety by keeping an extra string around.

Yep.  Better safe 'n sorry.

Ciao,
Dscho

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

[Tay Ray Chuan]

Hi,

On Thu, Jan 29, 2009 at 6:51 AM, Junio C Hamano <gitster@pobox.com> wrote:
> What's with these loooooooooooooooooooooooooooooong lines?
>
> I thought at least you did not have these overlong lines in your earlier
> attempts, and Dscho may have acked one of those, but I doubt he would give
> his Ack to this one.  I certainly wouldn't Ack it myself.
>
> By the way, aren't you sending format="flowed"?  Please don't.  It damages
> whitespaces.

I used Thunderbird since Gmail kept wrapping lines in the patch, guess
i"ll have to manually wrap the commit lines.

> Daniel Stenberg did a research on the safety of your "since curl stdrup's
> it" claim, and found that it unsafe for earlier versions of the library
> before 7.17.0.
>
> It seems that we earlier found out that anything older than 7.16 were not
> usable for git-http-push (see Release Notes for 1.5.4), but 7.16 is still
> older than 7.17, so either we declare you _must_ have 7.17 or newer to use
> http-push, or keep an extra copy around and free it later like the
> original code does.
>
> Even Debian is at 7.18.2 these days, so requiring 7.17 or newer may not be
> an issue in practice, but there are people who keep running things on
> older distros with proven stability (and known features limitation).
>
> The refactoring looked sane otherwise, but I think we would want to opt
> for safety by keeping an extra string around.

Hmm, since that string won't be released in start_fetch_loose, or
anywhere else, would this be considered a memory leak?

-- 
Cheers,
Ray Chuan

[PATCH] http-push: refactor request url creation

[Tay Ray Chuan]

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 two functions,
"append_remote_object_url" and "get_remote_object_url".

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).

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.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
Acked-by: Junio C Hamano <gitster@pobox.com>
---

* renamed only_two_digit_postfix in original patch to only_two_digit_prefix
* updated with Junio's changes (if (...) and fix memory leak)
* updated with Junio's double interface
* added back use of temporary string "url" in "start_fetch_loose"
* rebased and generated on master (a34a9db)
* split "append_remote_object_url" signature across 3 lines at Dscho's suggestion

 http-push.c |   62 +++++++++++++++++++++++-----------------------------------
 1 files changed, 25 insertions(+), 37 deletions(-)

diff --git a/http-push.c b/http-push.c
index 59037df..ba217fc 100644
--- a/http-push.c
+++ b/http-push.c
@@ -209,6 +209,22 @@ static struct curl_slist *get_dav_token_headers(struct remote_lock *lock, enum d
 	return dav_headers;
 }

+static void append_remote_object_url(struct strbuf *buf, const char *url,
+				     const char *hex,
+				     int only_two_digit_prefix)
+{
+	strbuf_addf(buf, "%sobjects/%.*s/", url, 2, hex);
+	if (!only_two_digit_prefix)
+		strbuf_addf(buf, "%s", hex+2);
+}
+
+static char *get_remote_object_url(const char *url, const char *hex, int only_two_digit_prefix)
+{
+	struct strbuf buf = STRBUF_INIT;
+	append_remote_object_url(&buf, url, hex, only_two_digit_prefix);
+	return strbuf_detach(&buf, NULL);
+}
+
 static void finish_request(struct transfer_request *request);
 static void release_request(struct transfer_request *request);

@@ -255,7 +271,6 @@ static void start_fetch_loose(struct transfer_request *request)
 	char *filename;
 	char prevfile[PATH_MAX];
 	char *url;
-	char *posn;
 	int prevlocal;
 	unsigned char prev_buf[PREV_BUF_SIZE];
 	ssize_t prev_read = 0;
@@ -305,17 +320,8 @@ 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);
+	url = get_remote_object_url(remote->url, hex, 0);
+	request->url = xstrdup(url);

 	/* If a previous temp file is present, process what was already
 	   fetched. */
@@ -388,16 +394,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;
@@ -512,7 +510,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 buf = STRBUF_INIT;
 	enum object_type type;
 	char hdr[50];
 	void *unpacked;
@@ -551,21 +549,13 @@ 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_addstr(&buf, "Destination: ");
+	append_remote_object_url(&buf, remote->url, hex, 0);
+	request->dest = strbuf_detach(&buf, NULL);
+
+	append_remote_object_url(&buf, remote->url, hex, 0);
+	strbuf_addstr(&buf, request->lock->token);
+	request->url = strbuf_detach(&buf, NULL);

 	slot = get_active_slot();
 	slot->callback_func = process_response;
-- 
1.6.1.2.254.g2f9c7

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

[Junio C Hamano]

Tay Ray Chuan <rctay89@gmail.com> 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 two functions,
> "append_remote_object_url" and "get_remote_object_url".
>
> 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).
>
> 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.
>
> Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
> Acked-by: Junio C Hamano <gitster@pobox.com>

Thanks, I'll queue this to 'pu' for now, but please stop randomly adding
"Acked-by" from other people, unless you conferred with them on the
exact version of your patch you are submitting.

If somebody said "The version that I reviewed looked sensible", it becomes
irrelevant after you changed your patch in response to people's comments,
exactly because what you are sending is different from what they reviewed.
It is up to them, not you, to see if the issues raised in their comments
are addressed in the new patch to their satisfaction.

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

[Johannes Schindelin]

Hi,

On Sat, 31 Jan 2009, Tay Ray Chuan wrote:

> * split "append_remote_object_url" signature across 3 lines at Dscho's suggestion

This line is longer than 80 characters (and I seem to remember that the 
recommended maximum for emails is even less than that).

> 
>  http-push.c |   62 +++++++++++++++++++++++-----------------------------------
>  1 files changed, 25 insertions(+), 37 deletions(-)
> 
> diff --git a/http-push.c b/http-push.c
> index 59037df..ba217fc 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -209,6 +209,22 @@ static struct curl_slist *get_dav_token_headers(struct remote_lock *lock, enum d
>  	return dav_headers;
>  }
> 
> +static void append_remote_object_url(struct strbuf *buf, const char *url,
> +				     const char *hex,
> +				     int only_two_digit_prefix)
> +{

Fine.  You changed that, although I find the indentation rather funny, 
too.  I would have expected one tab and then the rest of the signature.

> +	strbuf_addf(buf, "%sobjects/%.*s/", url, 2, hex);
> +	if (!only_two_digit_prefix)
> +		strbuf_addf(buf, "%s", hex+2);
> +}
> +
> +static char *get_remote_object_url(const char *url, const char *hex, int only_two_digit_prefix)

But this is still too long.  And no, I will not go through your patch and 
point out every too-long line; I'll expect you to do that yourself...

Ciao,
Dscho