Re: [PATCH] handle http urls with query string ("?foo") correctly

["David ‘Bombe’ Roden" <bombe@pterodactylus.net>]

[-- Attachment #1: Type: text/plain, Size: 8137 bytes --]

On Thursday 05 June 2008 02:12:21 Johannes Schindelin wrote:

> I think that this paragraph should be rewritten into a less "I did" form,
> and be the commit message.

Okay.


> This part of Git's source code predates the strbuf work, and therefore it
> is understandable that strbufs are not used there.  However, I think that
> your changes just cry for want of strbufs.

Sounds good. strbuf is indeed a tad more convenient than sprintf and strcat. :)


> Maybe you want to rewrite your patch before I keep on repeating these two
> comments? ;-)

I did. Find the revised version below. :)

---
handle http urls with query string ("?foo") correctly
git breaks when a repository is cloned from an http url that contains a
query string. This patch fixes this behaviour be inserting the name of
the requested object (like "/info/refs") before the query string.

 http-walker.c |   31 ++++++++++++-------------------
 http.c        |   42 +++++++++++++++++++++++++++++++++---------
 http.h        |    2 ++
 transport.c   |    5 +++--
 4 files changed, 50 insertions(+), 30 deletions(-)

diff --git a/http-walker.c b/http-walker.c
index 99f397e..b14497a 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -100,7 +100,6 @@ static void start_object_request(struct walker *walker,
 	char *hex = sha1_to_hex(obj_req->sha1);
 	char prevfile[PATH_MAX];
 	char *url;
-	char *posn;
 	int prevlocal;
 	unsigned char prev_buf[PREV_BUF_SIZE];
 	ssize_t prev_read = 0;
@@ -109,6 +108,7 @@ static void start_object_request(struct walker *walker,
 	struct curl_slist *range_header = NULL;
 	struct active_request_slot *slot;
 	struct walker_data *data = walker->data;
+	struct strbuf suffix_buffer = STRBUF_INIT;
 
 	snprintf(prevfile, sizeof(prevfile), "%s.prev", obj_req->filename);
 	unlink(prevfile);
@@ -146,16 +146,10 @@ static void start_object_request(struct walker *walker,
 
 	SHA1_Init(&obj_req->c);
 
-	url = xmalloc(strlen(obj_req->repo->base) + 51);
+	strbuf_addstr(&suffix_buffer, "/objects/");
+	strbuf_addf(&suffix_buffer, "%c%c/%s", hex[0], hex[1], hex + 2);
+	url = transform_url(obj_req->repo->base, strbuf_detach(&suffix_buffer, NULL));
 	obj_req->url = xmalloc(strlen(obj_req->repo->base) + 51);
-	strcpy(url, obj_req->repo->base);
-	posn = url + strlen(obj_req->repo->base);
-	strcpy(posn, "/objects/");
-	posn += 9;
-	memcpy(posn, hex, 2);
-	posn += 2;
-	*(posn++) = '/';
-	strcpy(posn, hex + 2);
 	strcpy(obj_req->url, url);
 
 	/* If a previous temp file is present, process what was already
@@ -373,6 +367,7 @@ static int fetch_index(struct walker *walker, struct alt_base *repo, unsigned ch
 	char range[RANGE_HEADER_SIZE];
 	struct curl_slist *range_header = NULL;
 	struct walker_data *data = walker->data;
+	struct strbuf suffix_buffer = STRBUF_INIT;
 
 	FILE *indexfile;
 	struct active_request_slot *slot;
@@ -384,8 +379,8 @@ static int fetch_index(struct walker *walker, struct alt_base *repo, unsigned ch
 	if (walker->get_verbosely)
 		fprintf(stderr, "Getting index for pack %s\n", hex);
 
-	url = xmalloc(strlen(repo->base) + 64);
-	sprintf(url, "%s/objects/pack/pack-%s.idx", repo->base, hex);
+	strbuf_addf(&suffix_buffer, "/objects/pack/pack-%s.idx", hex);
+	url = transform_url(repo->base, strbuf_detach(&suffix_buffer, NULL));
 
 	filename = sha1_pack_index_name(sha1);
 	snprintf(tmpfile, sizeof(tmpfile), "%s.temp", filename);
@@ -608,8 +603,7 @@ static void fetch_alternates(struct walker *walker, const char *base)
 	if (walker->get_verbosely)
 		fprintf(stderr, "Getting alternates list for %s\n", base);
 
-	url = xmalloc(strlen(base) + 31);
-	sprintf(url, "%s/objects/info/http-alternates", base);
+	url = transform_url(base, "/objects/info/http-alternates");
 
 	/* Use a callback to process the result, since another request
 	   may fail and need to have alternates loaded before continuing */
@@ -655,8 +649,7 @@ static int fetch_indices(struct walker *walker, struct alt_base *repo)
 	if (walker->get_verbosely)
 		fprintf(stderr, "Getting pack list for %s\n", repo->base);
 
-	url = xmalloc(strlen(repo->base) + 21);
-	sprintf(url, "%s/objects/info/packs", repo->base);
+	url = transform_url(repo->base, "/objects/info/packs");
 
 	slot = get_active_slot();
 	slot->results = &results;
@@ -722,6 +715,7 @@ static int fetch_pack(struct walker *walker, struct alt_base *repo, unsigned cha
 	char range[RANGE_HEADER_SIZE];
 	struct curl_slist *range_header = NULL;
 	struct walker_data *data = walker->data;
+	struct strbuf suffix_buffer = STRBUF_INIT;
 
 	struct active_request_slot *slot;
 	struct slot_results results;
@@ -739,9 +733,8 @@ static int fetch_pack(struct walker *walker, struct alt_base *repo, unsigned cha
 			sha1_to_hex(sha1));
 	}
 
-	url = xmalloc(strlen(repo->base) + 65);
-	sprintf(url, "%s/objects/pack/pack-%s.pack",
-		repo->base, sha1_to_hex(target->sha1));
+	strbuf_addf(&suffix_buffer, "/objects/pack/pack-%s.pack", sha1_to_hex(target->sha1));
+	url = transform_url(repo->base, strbuf_detach(&suffix_buffer, NULL));
 
 	filename = sha1_pack_name(target->sha1);
 	snprintf(tmpfile, sizeof(tmpfile), "%s.temp", filename);
diff --git a/http.c b/http.c
index 2a21ccb..a60f9ea 100644
--- a/http.c
+++ b/http.c
@@ -590,15 +590,17 @@ static char *quote_ref_url(const char *base, const char *ref)
 	qref = xmalloc(len);
 	memcpy(qref, base, baselen);
 	dp = qref + baselen;
-	*(dp++) = '/';
-	for (cp = ref; (ch = *cp) != 0; cp++) {
-		if (needs_quote(ch)) {
-			*dp++ = '%';
-			*dp++ = hex((ch >> 4) & 0xF);
-			*dp++ = hex(ch & 0xF);
+	if (*ref) {
+		*(dp++) = '/';
+		for (cp = ref; (ch = *cp) != 0; cp++) {
+			if (needs_quote(ch)) {
+				*dp++ = '%';
+				*dp++ = hex((ch >> 4) & 0xF);
+				*dp++ = hex(ch & 0xF);
+			}
+			else
+				*dp++ = ch;
 		}
-		else
-			*dp++ = ch;
 	}
 	*dp = 0;
 
@@ -611,9 +613,12 @@ int http_fetch_ref(const char *base, struct ref *ref)
 	struct strbuf buffer = STRBUF_INIT;
 	struct active_request_slot *slot;
 	struct slot_results results;
+	struct strbuf suffix_buffer = STRBUF_INIT;
 	int ret;
 
-	url = quote_ref_url(base, ref->name);
+	strbuf_addf(&suffix_buffer, "/%s", ref->name);
+	url = transform_url(base, strbuf_detach(&suffix_buffer, NULL));
+	url = quote_ref_url(url, "");
 	slot = get_active_slot();
 	slot->results = &results;
 	curl_easy_setopt(slot->curl, CURLOPT_FILE, &buffer);
@@ -643,3 +648,22 @@ int http_fetch_ref(const char *base, struct ref *ref)
 	free(url);
 	return ret;
 }
+
+char *transform_url(const char *url, const char *suffix)
+{
+	struct strbuf new_url = STRBUF_INIT;
+	char *question_mark;
+	ptrdiff_t offset;
+
+	if ((question_mark = strchr(url, '?'))) {
+		offset = (ptrdiff_t) question_mark - (ptrdiff_t) url;
+		strbuf_add(&new_url, url, offset);
+		strbuf_addstr(&new_url, suffix);
+		strbuf_addstr(&new_url, url + offset);
+	} else {
+		strbuf_addstr(&new_url, url);
+		strbuf_addstr(&new_url, suffix);
+	}
+	return strbuf_detach(&new_url, NULL);
+}
+
diff --git a/http.h b/http.h
index a04fc6a..58730b8 100644
--- a/http.h
+++ b/http.h
@@ -107,4 +107,6 @@ static inline int missing__target(int code, int result)
 
 extern int http_fetch_ref(const char *base, struct ref *ref);
 
+extern char *transform_url(const char *url, const char *suffix);
+
 #endif /* HTTP_H */
diff --git a/transport.c b/transport.c
index 3ff8519..b1966d8 100644
--- a/transport.c
+++ b/transport.c
@@ -1,3 +1,4 @@
+#include <stddef.h>
 #include "cache.h"
 #include "transport.h"
 #include "run-command.h"
@@ -449,8 +450,7 @@ static struct ref *get_refs_via_curl(struct transport *transport)
 
 	walker = transport->data;
 
-	refs_url = xmalloc(strlen(transport->url) + 11);
-	sprintf(refs_url, "%s/info/refs", transport->url);
+	refs_url = transform_url(transport->url, "/info/refs");
 
 	slot = get_active_slot();
 	slot->results = &results;
@@ -833,3 +833,4 @@ int transport_disconnect(struct transport *transport)
 	free(transport);
 	return ret;
 }
+
-- 
1.5.5.3


Thanks,

	David

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]