threads / patch / 17226

patchhttp-push: fix off-by-path_len

Subject: [PATCH] http-push: fix off-by-path_len

## tl;dr

33 messages between Jan 17, 2009 and Jan 18, 2009. Diffs are folded; open one to read it.

replies: 32people: 4as markdown or json

Johannes Schindelin· Jan 17, 2009, 15:36 UTC · lore

When getting the result of remote_ls(), we were advancing the variable "path" to the relative path inside the repository.

However, then we went on to malloc a bogus amount of memory: we were subtracting the prefix length _again_, quite possibly getting something negative, which xmalloc() interprets as really, really much.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	Note that the push in t5540 is still broken, as http-push does
	not handle packed-refs (when looking what branches are on the 
	remote side).

	It should not even try to access the directory structure under
	refs/ to begin with, but read info/refs instead.

	However, that is just one example of the ugliness that is 
	http-push.c; it also seems to be a perfect example of a copy-pasting 
	hell; just look at the output of "git grep
	curl_easy_setopt http-push.c".

	There _has_ to be lot of room for improvement.

 http-push.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

Show changes to http-push.c +7 −3

diff --git a/http-push.c b/http-push.c
index 9fcccee..cb5bf95 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1435,10 +1435,8 @@ static void handle_remote_ls_ctx(struct xml_ctx *ctx, int tag_closed)
 			}
 			if (path) {
 				path += remote->path_len;
+				ls->dentry_name = xstrdup(path);
 			}
-			ls->dentry_name = xmalloc(strlen(path) -
-						  remote->path_len + 1);
-			strcpy(ls->dentry_name, path + remote->path_len);
 		} else if (!strcmp(ctx->name, DAV_PROPFIND_COLLECTION)) {
 			ls->dentry_flags |= IS_DIR;
 		}
@@ -1449,6 +1447,12 @@ static void handle_remote_ls_ctx(struct xml_ctx *ctx, int tag_closed)
 	}
 }
 
+/*
+ * NEEDSWORK: remote_ls() ignores info/refs on the remote side.  But it
+ * should _only_ heed the information from that file, instead of trying to
+ * determine the refs from the remote file system (badly: it does not even
+ * know about packed-refs).
+ */
 static void remote_ls(const char *path, int flags,
 		      void (*userFunc)(struct remote_ls_ctx *ls),
 		      void *userData)

-- 
1.6.1.325.g062d4

Johannes Schindelin· Jan 17, 2009, 15:40 UTC · re: Johannes Schindelin · lore

Where's Nick?, was Re: [PATCH] http-push: fix off-by-path_len

Hi,

Nick's email address seems to be no longer functional. Any idea where he's hanging about these days?

Ciao, Dscho

Johannes Schindelin· Jan 17, 2009, 15:41 UTC · re: Johannes Schindelin · lore

[PATCH] t5540: clarify that http-push does not handle packed-refs on the remote

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	On Sat, 17 Jan 2009, Johannes Schindelin wrote:

	> 	Note that the push in t5540 is still broken, as http-push does
	> 	not handle packed-refs (when looking what branches are on the 
	> 	remote side).

	... and this clarifies the issue.

 t/t5540-http-push.sh |   18 +++++++++++++++---
 1 files changed, 15 insertions(+), 3 deletions(-)

Show changes to t/t5540-http-push.sh +15 −3

diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
index 22cfbb6..c236b5e 100755
--- a/t/t5540-http-push.sh
+++ b/t/t5540-http-push.sh
@@ -51,17 +51,29 @@ test_expect_success 'clone remote repository' '
 	git clone $HTTPD_URL/test_repo.git test_repo_clone
 '
 
-test_expect_failure 'push to remote repository' '
+test_expect_failure 'push to remote repository with packed refs' '
 	cd "$ROOT_PATH"/test_repo_clone &&
 	: >path2 &&
 	git add path2 &&
 	test_tick &&
 	git commit -m path2 &&
+	HEAD=$(git rev-parse --verify HEAD) &&
 	git push &&
-	[ -f "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/refs/heads/master" ]
+	(cd "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git &&
+	 test $HEAD = $(git rev-parse --verify HEAD))
 '
 
-test_expect_failure 'create and delete remote branch' '
+test_expect_success ' push to remote repository with unpacked refs' '
+	(cd "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git &&
+	 rm packed-refs &&
+	 git update-ref refs/heads/master \
+		0c973ae9bd51902a28466f3850b543fa66a6aaf4) &&
+	git push &&
+	(cd "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git &&
+	 test $HEAD = $(git rev-parse --verify HEAD))
+'
+
+test_expect_success 'create and delete remote branch' '
 	cd "$ROOT_PATH"/test_repo_clone &&
 	git checkout -b dev &&
 	: >path3 &&

-- 
1.6.1.325.g062d4

Mike Hommey· Jan 18, 2009, 07:49 UTC · re: Johannes Schindelin · lore

Re: [PATCH] http-push: fix off-by-path_len

On Sat, Jan 17, 2009 at 04:36:26PM +0100, Johannes Schindelin wrote:

Show 17 quoted lines

> 
> When getting the result of remote_ls(), we were advancing the variable
> "path" to the relative path inside the repository.
> 
> However, then we went on to malloc a bogus amount of memory: we were
> subtracting the prefix length _again_, quite possibly getting something
> negative, which xmalloc() interprets as really, really much.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> 
> 	Note that the push in t5540 is still broken, as http-push does
> 	not handle packed-refs (when looking what branches are on the 
> 	remote side).
> 
> 	It should not even try to access the directory structure under
> 	refs/ to begin with, but read info/refs instead.

It would actually need to do both, because nothing guarantees info/refs is up-to-date.

> 	However, that is just one example of the ugliness that is 
> 	http-push.c; it also seems to be a perfect example of a copy-pasting 
> 	hell; just look at the output of "git grep
> 	curl_easy_setopt http-push.c".

Likewise for http.c.

> 	There _has_ to be lot of room for improvement.

And I realize I have had a partial improvement on that sitting on my harddrive, without me having time (nor motivation) to go further.

Maybe it's time I let it go and post the work in progress for someone else to take over.

Mike

Mike Hommey· Jan 18, 2009, 08:04 UTC · re: Mike Hommey · lore

[WIP Patch 00/12] Refactoring the http API

Show 5 quoted lines

> And I realize I have had a partial improvement on that sitting on my
> harddrive, without me having time (nor motivation) to go further.
>
> Maybe it's time I let it go and post the work in progress for someone
> else to take over.

Here it is.

Note I already sent the first patch a year ago:
 http://markmail.org/message/s4w4pmla4tzjkpsf

This set of patches only deals with HTTP GET requests, so all of http-push is not taken care of.

As it is work in progress, some error handling might have regressions, but the original error handling is not necessarily much better.

Also note I only rebased my one-year-old work on current master, and haven't actually tested it, though, as the code hasn't changed much, I guess it should be fine.

I hope someone will have an itch to scratch to improve the whole thing.

Mike Hommey (12):
  Don't expect verify_pack() callers to set pack_size
  Some cleanup in get_refs_via_curl()
  Two new functions for the http API
  Use the new http API in http_fetch_ref()
  Use the new http API in get_refs_via_curl()
  Use the new http API in http-walker.c:fetch_indices()
  Use the new http API in http-push.c:fetch_indices()
  Use the new http API in update_remote_info_refs()
  Use the new http API in fetch_symref()
  Use the new http API in http-walker.c:fetch_index()
  Use the new http API in http-push.c:fetch_index()
  Use the new http API in http-walker.c:fetch_pack()

 http-push.c   |  159 ++++++++++-----------------------------------------------
 http-walker.c |  141 ++++++--------------------------------------------
 http.c        |  112 ++++++++++++++++++++++++++++++++--------
 http.h        |   17 ++++++
 pack-check.c  |    8 ++-
 transport.c   |   29 ++--------
 6 files changed, 162 insertions(+), 304 deletions(-)

Mike Hommey· Jan 18, 2009, 08:04 UTC · re: Mike Hommey · lore

[WIP Patch 01/12] Don't expect verify_pack() callers to set pack_size

Since use_pack() will end up populating pack_size if it is not already set, we can just adapt the code in verify_packfile() such that it doesn't require pack_size to be set beforehand.

This allows callers not to have to set pack_size themselves, and we can thus revert changes from 1c23d794 (Don't die in git-http-fetch when fetching packs).

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 http-push.c   |    3 ---
 http-walker.c |    1 -
 pack-check.c  |    8 +++++---
 3 files changed, 5 insertions(+), 7 deletions(-)

Show changes to 3 files +5 −7

http-push.c, http-walker.c, pack-check.c

diff --git a/http-push.c b/http-push.c
index a4b7d08..e69179b 100644
--- a/http-push.c
+++ b/http-push.c
@@ -771,14 +771,11 @@ static void finish_request(struct transfer_request *request)
 				request->url, curl_errorstr);
 			remote->can_update_info_refs = 0;
 		} else {
-			off_t pack_size = ftell(request->local_stream);
-
 			fclose(request->local_stream);
 			request->local_stream = NULL;
 			if (!move_temp_to_file(request->tmpfile,
 					       request->filename)) {
 				target = (struct packed_git *)request->userData;
-				target->pack_size = pack_size;
 				lst = &remote->packs;
 				while (*lst != target)
 					lst = &((*lst)->next);
diff --git a/http-walker.c b/http-walker.c
index 7271c7d..0139d1e 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -785,7 +785,6 @@ static int fetch_pack(struct walker *walker, struct alt_base *repo, unsigned cha
 		return error("Unable to start request");
 	}
 
-	target->pack_size = ftell(packfile);
 	fclose(packfile);
 
 	ret = move_temp_to_file(tmpfile, filename);
diff --git a/pack-check.c b/pack-check.c
index 90c33b1..166ca70 100644
--- a/pack-check.c
+++ b/pack-check.c
@@ -49,7 +49,7 @@ static int verify_packfile(struct packed_git *p,
 	const unsigned char *index_base = p->index_data;
 	git_SHA_CTX ctx;
 	unsigned char sha1[20], *pack_sig;
-	off_t offset = 0, pack_sig_ofs = p->pack_size - 20;
+	off_t offset = 0, pack_sig_ofs = 0;
 	uint32_t nr_objects, i;
 	int err = 0;
 	struct idx_entry *entries;
@@ -61,14 +61,16 @@ static int verify_packfile(struct packed_git *p,
 	 */
 
 	git_SHA1_Init(&ctx);
-	while (offset < pack_sig_ofs) {
+	do {
 		unsigned int remaining;
 		unsigned char *in = use_pack(p, w_curs, offset, &remaining);
 		offset += remaining;
+		if (!pack_sig_ofs)
+			pack_sig_ofs = p->pack_size - 20;
 		if (offset > pack_sig_ofs)
 			remaining -= (unsigned int)(offset - pack_sig_ofs);
 		git_SHA1_Update(&ctx, in, remaining);
-	}
+	} while (offset < pack_sig_ofs);
 	git_SHA1_Final(sha1, &ctx);
 	pack_sig = use_pack(p, w_curs, pack_sig_ofs, NULL);
 	if (hashcmp(sha1, pack_sig))

-- 
1.6.1.141.gb32a

Mike Hommey· Jan 18, 2009, 08:04 UTC · re: Mike Hommey · lore

[WIP Patch 02/12] Some cleanup in get_refs_via_curl()

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 transport.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

Show changes to transport.c +2 −0

diff --git a/transport.c b/transport.c
index 56831c5..6919ff1 100644
--- a/transport.c
+++ b/transport.c
@@ -508,6 +508,8 @@ static struct ref *get_refs_via_curl(struct transport *transport)
 		free(ref);
 	}
 
+	http_cleanup();
+	free(refs_url);
 	return refs;
 }

-- 
1.6.1.141.gb32a

Mike Hommey· Jan 18, 2009, 08:04 UTC · re: Mike Hommey · lore

[WIP Patch 03/12] Two new functions for the http API

http_get_strbuf and http_get_file allow respectively to retrieve contents of an URL to a strbuf or an opened file handle.

Both these functions are the beginning of the http code refactoring.

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 http.c |   85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 http.h |   17 +++++++++++++
 2 files changed, 102 insertions(+), 0 deletions(-)

Show changes to 2 files +102 −0

http.c, http.h

diff --git a/http.c b/http.c
index ee58799..82534cf 100644
--- a/http.c
+++ b/http.c
@@ -638,3 +638,88 @@ int http_fetch_ref(const char *base, struct ref *ref)
 	free(url);
 	return ret;
 }
+
+/* http_request() targets */
+#define HTTP_REQUEST_STRBUF	0
+#define HTTP_REQUEST_FILE	1
+
+static int http_request(const char *url, void *result, int target, int options)
+{
+	struct active_request_slot *slot;
+	struct slot_results results;
+	struct curl_slist *headers = NULL;
+	struct strbuf buf = STRBUF_INIT;
+
+	slot = get_active_slot();
+	slot->results = &results;
+	curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
+
+	if (result == NULL) {
+		curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1);
+	} else {
+		curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
+		curl_easy_setopt(slot->curl, CURLOPT_FILE, result);
+
+		if (target == HTTP_REQUEST_FILE) {
+			long posn = ftell(result);
+			curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,
+					 fwrite);
+			if (posn > 0) {
+				strbuf_addf(&buf, "Range: bytes=%ld-", posn);
+				headers = curl_slist_append(headers, buf.buf);
+				strbuf_reset(&buf);
+			}
+			slot->local = result;
+		} else
+			curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,
+					 fwrite_buffer);
+	}
+
+	strbuf_addstr(&buf, "Pragma:");
+	if (options & HTTP_NO_CACHE)
+		strbuf_addstr(&buf, " no-cache");
+
+	headers = curl_slist_append(headers, buf.buf);
+	strbuf_release(&buf);
+
+	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
+	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
+
+	if (start_active_slot(slot)) {
+		run_active_slot(slot);
+		if (results.curl_result == CURLE_OK)
+			return HTTP_OK;
+		else if (missing_target(&results))
+			return HTTP_MISSING_TARGET;
+	}
+	return HTTP_ERROR;
+}
+
+int http_get_strbuf(const char *url, struct strbuf *result, int options)
+{
+	return http_request(url, result, HTTP_REQUEST_STRBUF, options);
+}
+
+int http_get_file(const char *url, const char *filename, int options)
+{
+	int ret;
+	struct strbuf tmpfile = STRBUF_INIT;
+	FILE *result;
+
+	strbuf_addf(&tmpfile, "%s.temp", filename);
+	result = fopen(tmpfile.buf, "a");
+	if (! result) {
+		error("Unable to open local file %s", tmpfile.buf);
+		ret = HTTP_ERROR;
+		goto cleanup;
+	}
+
+	ret = http_request(url, result, HTTP_REQUEST_FILE, options);
+	fclose(result);
+
+	if ((ret == HTTP_OK) && move_temp_to_file(tmpfile.buf, filename))
+		ret = HTTP_ERROR;
+cleanup:
+	strbuf_release(&tmpfile);
+	return ret;
+}
diff --git a/http.h b/http.h
index 905b462..323c780 100644
--- a/http.h
+++ b/http.h
@@ -104,4 +104,21 @@ static inline int missing__target(int code, int result)
 
 extern int http_fetch_ref(const char *base, struct ref *ref);
 
+/* Options for http_request_*() */
+#define HTTP_NO_CACHE		1
+
+/* Return values for http_request_*() */
+#define HTTP_OK			0
+#define HTTP_MISSING_TARGET	1
+#define HTTP_ERROR		2
+
+/* Requests an url and stores the result in a strbuf.
+ * If the result pointer is NULL, a HTTP HEAD request is made instead of GET. */
+int http_get_strbuf(const char *url, struct strbuf *result, int options);
+
+/* Downloads an url and stores the result in the given file.
+ * If a previous interrupted download is detected (i.e. a previous temporary
+ * file is still around) the download is resumed. */
+int http_get_file(const char *url, const char *filename, int options);
+
 #endif /* HTTP_H */

-- 
1.6.1.141.gb32a

Mike Hommey· Jan 18, 2009, 08:04 UTC · re: Mike Hommey · lore

[WIP Patch 04/12] Use the new http API in http_fetch_ref()

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 http.c |   33 ++++++++-------------------------
 1 files changed, 8 insertions(+), 25 deletions(-)

Show changes to http.c +8 −25

diff --git a/http.c b/http.c
index 82534cf..0c9504b 100644
--- a/http.c
+++ b/http.c
@@ -604,34 +604,17 @@ int http_fetch_ref(const char *base, struct ref *ref)
 {
 	char *url;
 	struct strbuf buffer = STRBUF_INIT;
-	struct active_request_slot *slot;
-	struct slot_results results;
-	int ret;
+	int ret = -1;
 
 	url = quote_ref_url(base, ref->name);
-	slot = get_active_slot();
-	slot->results = &results;
-	curl_easy_setopt(slot->curl, CURLOPT_FILE, &buffer);
-	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
-	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, NULL);
-	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
-	if (start_active_slot(slot)) {
-		run_active_slot(slot);
-		if (results.curl_result == CURLE_OK) {
-			strbuf_rtrim(&buffer);
-			if (buffer.len == 40)
-				ret = get_sha1_hex(buffer.buf, ref->old_sha1);
-			else if (!prefixcmp(buffer.buf, "ref: ")) {
-				ref->symref = xstrdup(buffer.buf + 5);
-				ret = 0;
-			} else
-				ret = 1;
-		} else {
-			ret = error("Couldn't get %s for %s\n%s",
-				    url, ref->name, curl_errorstr);
+	if (http_get_strbuf(url, &buffer, HTTP_NO_CACHE) == HTTP_OK) {
+		strbuf_rtrim(&buffer);
+		if (buffer.len == 40)
+			ret = get_sha1_hex(buffer.buf, ref->old_sha1);
+		else if (!prefixcmp(buffer.buf, "ref: ")) {
+			ref->symref = xstrdup(buffer.buf + 5);
+			ret = 0;
 		}
-	} else {
-		ret = error("Unable to start request");
 	}
 
 	strbuf_release(&buffer);

-- 
1.6.1.141.gb32a

Mike Hommey· Jan 18, 2009, 08:04 UTC · re: Mike Hommey · lore

[WIP Patch 05/12] Use the new http API in get_refs_via_curl()

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 transport.c |   27 ++++-----------------------
 1 files changed, 4 insertions(+), 23 deletions(-)

Show changes to transport.c +4 −23

diff --git a/transport.c b/transport.c
index 6919ff1..55bf274 100644
--- a/transport.c
+++ b/transport.c
@@ -432,9 +432,6 @@ static struct ref *get_refs_via_curl(struct transport *transport)
 	char *refs_url;
 	int i = 0;
 
-	struct active_request_slot *slot;
-	struct slot_results results;
-
 	struct ref *refs = NULL;
 	struct ref *ref = NULL;
 	struct ref *last_ref = NULL;
@@ -450,26 +447,8 @@ static struct ref *get_refs_via_curl(struct transport *transport)
 	refs_url = xmalloc(strlen(transport->url) + 11);
 	sprintf(refs_url, "%s/info/refs", transport->url);
 
-	slot = get_active_slot();
-	slot->results = &results;
-	curl_easy_setopt(slot->curl, CURLOPT_FILE, &buffer);
-	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
-	curl_easy_setopt(slot->curl, CURLOPT_URL, refs_url);
-	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, NULL);
-
-	if (start_active_slot(slot)) {
-		run_active_slot(slot);
-		if (results.curl_result != CURLE_OK) {
-			strbuf_release(&buffer);
-			if (missing_target(&results))
-				die("%s not found: did you run git update-server-info on the server?", refs_url);
-			else
-				die("%s download error - %s", refs_url, curl_errorstr);
-		}
-	} else {
-		strbuf_release(&buffer);
-		die("Unable to start HTTP request");
-	}
+	if (http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE) != HTTP_OK)
+		goto cleanup;
 
 	data = buffer.buf;
 	start = NULL;
@@ -508,6 +487,8 @@ static struct ref *get_refs_via_curl(struct transport *transport)
 		free(ref);
 	}
 
+cleanup:
+	strbuf_release(&buffer);
 	http_cleanup();
 	free(refs_url);
 	return refs;

-- 
1.6.1.141.gb32a

Mike Hommey· Jan 18, 2009, 08:04 UTC · re: Mike Hommey · lore

[WIP Patch 06/12] Use the new http API in http-walker.c:fetch_indices()

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 http-walker.c |   31 ++++++++-----------------------
 1 files changed, 8 insertions(+), 23 deletions(-)

Show changes to http-walker.c +8 −23

diff --git a/http-walker.c b/http-walker.c
index 0139d1e..edcb779 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -648,9 +648,6 @@ static int fetch_indices(struct walker *walker, struct alt_base *repo)
 	int i = 0;
 	int ret = 0;
 
-	struct active_request_slot *slot;
-	struct slot_results results;
-
 	if (repo->got_indices)
 		return 0;
 
@@ -660,27 +657,15 @@ static int fetch_indices(struct walker *walker, struct alt_base *repo)
 	url = xmalloc(strlen(repo->base) + 21);
 	sprintf(url, "%s/objects/info/packs", repo->base);
 
-	slot = get_active_slot();
-	slot->results = &results;
-	curl_easy_setopt(slot->curl, CURLOPT_FILE, &buffer);
-	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
-	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
-	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, NULL);
-	if (start_active_slot(slot)) {
-		run_active_slot(slot);
-		if (results.curl_result != CURLE_OK) {
-			if (missing_target(&results)) {
-				repo->got_indices = 1;
-				goto cleanup;
-			} else {
-				repo->got_indices = 0;
-				ret = error("%s", curl_errorstr);
-				goto cleanup;
-			}
-		}
-	} else {
+	switch (http_get_strbuf(url, &buffer, HTTP_NO_CACHE)) {
+	case HTTP_OK:
+		break;
+	case HTTP_MISSING_TARGET:
+		repo->got_indices = 1;
+		goto cleanup;
+	default:
 		repo->got_indices = 0;
-		ret = error("Unable to start request");
+		ret = -1;
 		goto cleanup;
 	}

-- 
1.6.1.141.gb32a

Mike Hommey· Jan 18, 2009, 08:04 UTC · re: Mike Hommey · lore

[WIP Patch 07/12] Use the new http API in http-push.c:fetch_indices()

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 http-push.c |   32 +++++++-------------------------
 1 files changed, 7 insertions(+), 25 deletions(-)

Show changes to http-push.c +7 −25

diff --git a/http-push.c b/http-push.c
index e69179b..e0b4f5a 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1008,9 +1008,7 @@ static int fetch_indices(void)
 	struct strbuf buffer = STRBUF_INIT;
 	char *data;
 	int i = 0;
-
-	struct active_request_slot *slot;
-	struct slot_results results;
+	int ret = 0;
 
 	if (push_verbosely)
 		fprintf(stderr, "Getting pack list\n");
@@ -1018,28 +1016,10 @@ static int fetch_indices(void)
 	url = xmalloc(strlen(remote->url) + 20);
 	sprintf(url, "%sobjects/info/packs", remote->url);
 
-	slot = get_active_slot();
-	slot->results = &results;
-	curl_easy_setopt(slot->curl, CURLOPT_FILE, &buffer);
-	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
-	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
-	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, NULL);
-	if (start_active_slot(slot)) {
-		run_active_slot(slot);
-		if (results.curl_result != CURLE_OK) {
-			strbuf_release(&buffer);
-			free(url);
-			if (results.http_code == 404)
-				return 0;
-			else
-				return error("%s", curl_errorstr);
-		}
-	} else {
-		strbuf_release(&buffer);
-		free(url);
-		return error("Unable to start request");
+	if (http_get_strbuf(url, &buffer, 0) != HTTP_OK) {
+		ret = -1;
+		goto cleanup;
 	}
-	free(url);
 
 	data = buffer.buf;
 	while (i < buffer.len) {
@@ -1061,8 +1041,10 @@ static int fetch_indices(void)
 		i++;
 	}
 
+cleanup:
 	strbuf_release(&buffer);
-	return 0;
+	free(url);
+	return ret;
 }
 
 static void one_remote_object(const char *hex)

-- 
1.6.1.141.gb32a

Mike Hommey· Jan 18, 2009, 08:04 UTC · re: Mike Hommey · lore

[WIP Patch 08/12] Use the new http API in update_remote_info_refs()

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 http-push.c |   29 ++++++++++-------------------
 1 files changed, 10 insertions(+), 19 deletions(-)

Show changes to http-push.c +10 −19

diff --git a/http-push.c b/http-push.c
index e0b4f5a..7627860 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1960,29 +1960,20 @@ static void update_remote_info_refs(struct remote_lock *lock)
 static int remote_exists(const char *path)
 {
 	char *url = xmalloc(strlen(remote->url) + strlen(path) + 1);
-	struct active_request_slot *slot;
-	struct slot_results results;
-	int ret = -1;
+	int ret;
 
 	sprintf(url, "%s%s", remote->url, path);
 
-	slot = get_active_slot();
-	slot->results = &results;
-	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
-	curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1);
-
-	if (start_active_slot(slot)) {
-		run_active_slot(slot);
-		if (results.http_code == 404)
-			ret = 0;
-		else if (results.curl_result == CURLE_OK)
-			ret = 1;
-		else
-			fprintf(stderr, "HEAD HTTP error %ld\n", results.http_code);
-	} else {
-		fprintf(stderr, "Unable to start HEAD request\n");
+	switch (http_get_strbuf(url, NULL, 0)) {
+	case HTTP_OK:
+		ret = 1;
+		break;
+	case HTTP_MISSING_TARGET:
+		ret = 0;
+		break;
+	default:
+		ret = -1;
 	}
-
 	free(url);
 	return ret;
 }

-- 
1.6.1.141.gb32a

Mike Hommey· Jan 18, 2009, 08:04 UTC · re: Mike Hommey · lore

[WIP Patch 09/12] Use the new http API in fetch_symref()

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 http-push.c |   19 ++-----------------
 1 files changed, 2 insertions(+), 17 deletions(-)

Show changes to http-push.c +2 −17

diff --git a/http-push.c b/http-push.c
index 7627860..b32def4 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1982,27 +1982,12 @@ static void fetch_symref(const char *path, char **symref, unsigned char *sha1)
 {
 	char *url;
 	struct strbuf buffer = STRBUF_INIT;
-	struct active_request_slot *slot;
-	struct slot_results results;
 
 	url = xmalloc(strlen(remote->url) + strlen(path) + 1);
 	sprintf(url, "%s%s", remote->url, path);
 
-	slot = get_active_slot();
-	slot->results = &results;
-	curl_easy_setopt(slot->curl, CURLOPT_FILE, &buffer);
-	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
-	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, NULL);
-	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
-	if (start_active_slot(slot)) {
-		run_active_slot(slot);
-		if (results.curl_result != CURLE_OK) {
-			die("Couldn't get %s for remote symref\n%s",
-			    url, curl_errorstr);
-		}
-	} else {
-		die("Unable to start remote symref request");
-	}
+	if (http_get_strbuf(url, &buffer, 0) != HTTP_OK)
+		die("Couldn't get %s for remote symref\n", url);
 	free(url);
 
 	free(*symref);

-- 
1.6.1.141.gb32a

Mike Hommey· Jan 18, 2009, 08:04 UTC · re: Mike Hommey · lore

[WIP Patch 10/12] Use the new http API in http-walker.c:fetch_index()

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 http-walker.c |   55 +++++--------------------------------------------------
 1 files changed, 5 insertions(+), 50 deletions(-)

Show changes to http-walker.c +5 −50

diff --git a/http-walker.c b/http-walker.c
index edcb779..bc1db2b 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -368,15 +368,7 @@ static int fetch_index(struct walker *walker, struct alt_base *repo, unsigned ch
 	char *hex = sha1_to_hex(sha1);
 	char *filename;
 	char *url;
-	char tmpfile[PATH_MAX];
-	long prev_posn = 0;
-	char range[RANGE_HEADER_SIZE];
-	struct curl_slist *range_header = NULL;
-	struct walker_data *data = walker->data;
-
-	FILE *indexfile;
-	struct active_request_slot *slot;
-	struct slot_results results;
+	int ret = 0;
 
 	if (has_pack_index(sha1))
 		return 0;
@@ -388,48 +380,11 @@ static int fetch_index(struct walker *walker, struct alt_base *repo, unsigned ch
 	sprintf(url, "%s/objects/pack/pack-%s.idx", repo->base, hex);
 
 	filename = sha1_pack_index_name(sha1);
-	snprintf(tmpfile, sizeof(tmpfile), "%s.temp", filename);
-	indexfile = fopen(tmpfile, "a");
-	if (!indexfile)
-		return error("Unable to open local file %s for pack index",
-			     tmpfile);
-
-	slot = get_active_slot();
-	slot->results = &results;
-	curl_easy_setopt(slot->curl, CURLOPT_FILE, indexfile);
-	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite);
-	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
-	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, data->no_pragma_header);
-	slot->local = indexfile;
-
-	/* If there is data present from a previous transfer attempt,
-	   resume where it left off */
-	prev_posn = ftell(indexfile);
-	if (prev_posn>0) {
-		if (walker->get_verbosely)
-			fprintf(stderr,
-				"Resuming fetch of index for pack %s at byte %ld\n",
-				hex, prev_posn);
-		sprintf(range, "Range: bytes=%ld-", prev_posn);
-		range_header = curl_slist_append(range_header, range);
-		curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, range_header);
-	}
-
-	if (start_active_slot(slot)) {
-		run_active_slot(slot);
-		if (results.curl_result != CURLE_OK) {
-			fclose(indexfile);
-			return error("Unable to get pack index %s\n%s", url,
-				     curl_errorstr);
-		}
-	} else {
-		fclose(indexfile);
-		return error("Unable to start request");
-	}
-
-	fclose(indexfile);
 
-	return move_temp_to_file(tmpfile, filename);
+	if (http_get_file(url, filename, 0) != HTTP_OK)
+		ret = error("Unable to get pack index %s\n", url);
+	free(url);
+	return ret;
 }
 
 static int setup_index(struct walker *walker, struct alt_base *repo, unsigned char *sha1)

-- 
1.6.1.141.gb32a

Mike Hommey· Jan 18, 2009, 08:04 UTC · re: Mike Hommey · lore

[WIP Patch 11/12] Use the new http API in http-push.c:fetch_index()

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 http-push.c |   76 ++++++----------------------------------------------------
 1 files changed, 8 insertions(+), 68 deletions(-)

Show changes to http-push.c +8 −68

diff --git a/http-push.c b/http-push.c
index b32def4..0812f58 100644
--- a/http-push.c
+++ b/http-push.c
@@ -898,32 +898,15 @@ static int fetch_index(unsigned char *sha1)
 	char *hex = sha1_to_hex(sha1);
 	char *filename;
 	char *url;
-	char tmpfile[PATH_MAX];
-	long prev_posn = 0;
-	char range[RANGE_HEADER_SIZE];
-	struct curl_slist *range_header = NULL;
-
-	FILE *indexfile;
-	struct active_request_slot *slot;
-	struct slot_results results;
+	int ret = 0;
 
 	/* Don't use the index if the pack isn't there */
 	url = xmalloc(strlen(remote->url) + 64);
 	sprintf(url, "%sobjects/pack/pack-%s.pack", remote->url, hex);
-	slot = get_active_slot();
-	slot->results = &results;
-	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
-	curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1);
-	if (start_active_slot(slot)) {
-		run_active_slot(slot);
-		if (results.curl_result != CURLE_OK) {
-			free(url);
-			return error("Unable to verify pack %s is available",
-				     hex);
-		}
-	} else {
+	if (http_get_strbuf(url, NULL, 0)) {
 		free(url);
-		return error("Unable to start request");
+		return error("Unable to verify pack %s is available",
+			     hex);
 	}
 
 	if (has_pack_index(sha1)) {
@@ -937,55 +920,12 @@ static int fetch_index(unsigned char *sha1)
 	sprintf(url, "%sobjects/pack/pack-%s.idx", remote->url, hex);
 
 	filename = sha1_pack_index_name(sha1);
-	snprintf(tmpfile, sizeof(tmpfile), "%s.temp", filename);
-	indexfile = fopen(tmpfile, "a");
-	if (!indexfile) {
-		free(url);
-		return error("Unable to open local file %s for pack index",
-			     tmpfile);
-	}
-
-	slot = get_active_slot();
-	slot->results = &results;
-	curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
-	curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
-	curl_easy_setopt(slot->curl, CURLOPT_FILE, indexfile);
-	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite);
-	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
-	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, no_pragma_header);
-	slot->local = indexfile;
-
-	/* If there is data present from a previous transfer attempt,
-	   resume where it left off */
-	prev_posn = ftell(indexfile);
-	if (prev_posn>0) {
-		if (push_verbosely)
-			fprintf(stderr,
-				"Resuming fetch of index for pack %s at byte %ld\n",
-				hex, prev_posn);
-		sprintf(range, "Range: bytes=%ld-", prev_posn);
-		range_header = curl_slist_append(range_header, range);
-		curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, range_header);
-	}
-
-	if (start_active_slot(slot)) {
-		run_active_slot(slot);
-		if (results.curl_result != CURLE_OK) {
-			free(url);
-			fclose(indexfile);
-			return error("Unable to get pack index %s\n%s", url,
-				     curl_errorstr);
-		}
-	} else {
-		free(url);
-		fclose(indexfile);
-		return error("Unable to start request");
-	}
 
+	if (http_get_file(url, filename, 0) != HTTP_OK)
+		ret = error("Unable to get pack index %s\n", url);
+	free(filename);
 	free(url);
-	fclose(indexfile);
-
-	return move_temp_to_file(tmpfile, filename);
+	return ret;
 }
 
 static int setup_index(unsigned char *sha1)

-- 
1.6.1.141.gb32a

Mike Hommey· Jan 18, 2009, 08:04 UTC · re: Mike Hommey · lore

[WIP Patch 12/12] Use the new http API in http-walker.c:fetch_pack()

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 http-walker.c |   54 ++++--------------------------------------------------
 1 files changed, 4 insertions(+), 50 deletions(-)

Show changes to http-walker.c +4 −50

diff --git a/http-walker.c b/http-walker.c
index bc1db2b..c201463 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -656,17 +656,8 @@ static int fetch_pack(struct walker *walker, struct alt_base *repo, unsigned cha
 	char *url;
 	struct packed_git *target;
 	struct packed_git **lst;
-	FILE *packfile;
 	char *filename;
-	char tmpfile[PATH_MAX];
-	int ret;
-	long prev_posn = 0;
-	char range[RANGE_HEADER_SIZE];
-	struct curl_slist *range_header = NULL;
-	struct walker_data *data = walker->data;
-
-	struct active_request_slot *slot;
-	struct slot_results results;
+	int ret = 0;
 
 	if (fetch_indices(walker, repo))
 		return -1;
@@ -686,48 +677,11 @@ static int fetch_pack(struct walker *walker, struct alt_base *repo, unsigned cha
 		repo->base, sha1_to_hex(target->sha1));
 
 	filename = sha1_pack_name(target->sha1);
-	snprintf(tmpfile, sizeof(tmpfile), "%s.temp", filename);
-	packfile = fopen(tmpfile, "a");
-	if (!packfile)
-		return error("Unable to open local file %s for pack",
-			     tmpfile);
-
-	slot = get_active_slot();
-	slot->results = &results;
-	curl_easy_setopt(slot->curl, CURLOPT_FILE, packfile);
-	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite);
-	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
-	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, data->no_pragma_header);
-	slot->local = packfile;
-
-	/* If there is data present from a previous transfer attempt,
-	   resume where it left off */
-	prev_posn = ftell(packfile);
-	if (prev_posn>0) {
-		if (walker->get_verbosely)
-			fprintf(stderr,
-				"Resuming fetch of pack %s at byte %ld\n",
-				sha1_to_hex(target->sha1), prev_posn);
-		sprintf(range, "Range: bytes=%ld-", prev_posn);
-		range_header = curl_slist_append(range_header, range);
-		curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, range_header);
-	}
 
-	if (start_active_slot(slot)) {
-		run_active_slot(slot);
-		if (results.curl_result != CURLE_OK) {
-			fclose(packfile);
-			return error("Unable to get pack file %s\n%s", url,
-				     curl_errorstr);
-		}
-	} else {
-		fclose(packfile);
-		return error("Unable to start request");
-	}
-
-	fclose(packfile);
+	if (http_get_file(url, filename, 0) != HTTP_OK)
+		ret = error("Unable to get pack file %s\n", url);
+	free(url);
 
-	ret = move_temp_to_file(tmpfile, filename);
 	if (ret)
 		return ret;

-- 
1.6.1.141.gb32a

Johannes Schindelin· Jan 18, 2009, 15:18 UTC · re: Mike Hommey · lore

Re: [WIP Patch 08/12] Use the new http API in update_remote_info_refs()

Hi,

On Sun, 18 Jan 2009, Mike Hommey wrote:

Show 13 quoted lines

> 
> Signed-off-by: Mike Hommey <mh@glandium.org>
> ---
>  http-push.c |   29 ++++++++++-------------------
>  1 files changed, 10 insertions(+), 19 deletions(-)
> 
> diff --git a/http-push.c b/http-push.c
> index e0b4f5a..7627860 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -1960,29 +1960,20 @@ static void update_remote_info_refs(struct remote_lock *lock)
>  static int remote_exists(const char *path)
>  {

Heh, I see where your commit subject comes from, but it should rather mention the function "remote_exists()"...

Show 33 quoted lines

>  	char *url = xmalloc(strlen(remote->url) + strlen(path) + 1);
> -	struct active_request_slot *slot;
> -	struct slot_results results;
> -	int ret = -1;
> +	int ret;
>  
>  	sprintf(url, "%s%s", remote->url, path);
>  
> -	slot = get_active_slot();
> -	slot->results = &results;
> -	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
> -	curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1);
> -
> -	if (start_active_slot(slot)) {
> -		run_active_slot(slot);
> -		if (results.http_code == 404)
> -			ret = 0;
> -		else if (results.curl_result == CURLE_OK)
> -			ret = 1;
> -		else
> -			fprintf(stderr, "HEAD HTTP error %ld\n", results.http_code);
> -	} else {
> -		fprintf(stderr, "Unable to start HEAD request\n");
> +	switch (http_get_strbuf(url, NULL, 0)) {
> +	case HTTP_OK:
> +		ret = 1;
> +		break;
> +	case HTTP_MISSING_TARGET:
> +		ret = 0;
> +		break;
> +	default:
> +		ret = -1;
>  	}

Does http_get_strbuf() already show the error? Not as far as I can see, even if it would make sense, no? At least you'll have to "return error(...)".

Ciao, Dscho

Mike Hommey· Jan 18, 2009, 19:23 UTC · re: Johannes Schindelin · lore

Re: [WIP Patch 08/12] Use the new http API in update_remote_info_refs()

On Sun, Jan 18, 2009 at 04:18:16PM +0100, Johannes Schindelin wrote:

Show 58 quoted lines

> Hi,
> 
> On Sun, 18 Jan 2009, Mike Hommey wrote:
> 
> > 
> > Signed-off-by: Mike Hommey <mh@glandium.org>
> > ---
> >  http-push.c |   29 ++++++++++-------------------
> >  1 files changed, 10 insertions(+), 19 deletions(-)
> > 
> > diff --git a/http-push.c b/http-push.c
> > index e0b4f5a..7627860 100644
> > --- a/http-push.c
> > +++ b/http-push.c
> > @@ -1960,29 +1960,20 @@ static void update_remote_info_refs(struct remote_lock *lock)
> >  static int remote_exists(const char *path)
> >  {
> 
> Heh, I see where your commit subject comes from, but it should rather 
> mention the function "remote_exists()"...
> 
> >  	char *url = xmalloc(strlen(remote->url) + strlen(path) + 1);
> > -	struct active_request_slot *slot;
> > -	struct slot_results results;
> > -	int ret = -1;
> > +	int ret;
> >  
> >  	sprintf(url, "%s%s", remote->url, path);
> >  
> > -	slot = get_active_slot();
> > -	slot->results = &results;
> > -	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
> > -	curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1);
> > -
> > -	if (start_active_slot(slot)) {
> > -		run_active_slot(slot);
> > -		if (results.http_code == 404)
> > -			ret = 0;
> > -		else if (results.curl_result == CURLE_OK)
> > -			ret = 1;
> > -		else
> > -			fprintf(stderr, "HEAD HTTP error %ld\n", results.http_code);
> > -	} else {
> > -		fprintf(stderr, "Unable to start HEAD request\n");
> > +	switch (http_get_strbuf(url, NULL, 0)) {
> > +	case HTTP_OK:
> > +		ret = 1;
> > +		break;
> > +	case HTTP_MISSING_TARGET:
> > +		ret = 0;
> > +		break;
> > +	default:
> > +		ret = -1;
> >  	}
> 
> Does http_get_strbuf() already show the error?  Not as far as I can see, 
> even if it would make sense, no?  At least you'll have to "return 
> error(...)".

As I said, it has some error handling regressions ;)

Mike

Johannes Schindelin· Jan 18, 2009, 15:14 UTC · re: Mike Hommey · lore

Re: [WIP Patch 07/12] Use the new http API in http-push.c:fetch_indices()

Hi,

On Sun, 18 Jan 2009, Mike Hommey wrote:

Show 28 quoted lines

> @@ -1018,28 +1016,10 @@ static int fetch_indices(void)
>  	url = xmalloc(strlen(remote->url) + 20);
>  	sprintf(url, "%sobjects/info/packs", remote->url);
>  
> -	slot = get_active_slot();
> -	slot->results = &results;
> -	curl_easy_setopt(slot->curl, CURLOPT_FILE, &buffer);
> -	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
> -	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
> -	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, NULL);
> -	if (start_active_slot(slot)) {
> -		run_active_slot(slot);
> -		if (results.curl_result != CURLE_OK) {
> -			strbuf_release(&buffer);
> -			free(url);
> -			if (results.http_code == 404)
> -				return 0;
> -			else
> -				return error("%s", curl_errorstr);
> -		}
> -	} else {
> -		strbuf_release(&buffer);
> -		free(url);
> -		return error("Unable to start request");
> +	if (http_get_strbuf(url, &buffer, 0) != HTTP_OK) {
> +		ret = -1;
> +		goto cleanup;
>  	}

Let's make that

		ret = error("%s", curl_errorstr);

okay?

Ciao, Dscho

Johannes Schindelin· Jan 18, 2009, 15:12 UTC · re: Mike Hommey · lore

Re: [WIP Patch 05/12] Use the new http API in get_refs_via_curl()

Hi,

On Sun, 18 Jan 2009, Mike Hommey wrote:

Show 26 quoted lines

> @@ -450,26 +447,8 @@ static struct ref *get_refs_via_curl(struct transport *transport)
>  	refs_url = xmalloc(strlen(transport->url) + 11);
>  	sprintf(refs_url, "%s/info/refs", transport->url);
>  
> -	slot = get_active_slot();
> -	slot->results = &results;
> -	curl_easy_setopt(slot->curl, CURLOPT_FILE, &buffer);
> -	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
> -	curl_easy_setopt(slot->curl, CURLOPT_URL, refs_url);
> -	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, NULL);
> -
> -	if (start_active_slot(slot)) {
> -		run_active_slot(slot);
> -		if (results.curl_result != CURLE_OK) {
> -			strbuf_release(&buffer);
> -			if (missing_target(&results))
> -				die("%s not found: did you run git update-server-info on the server?", refs_url);
> -			else
> -				die("%s download error - %s", refs_url, curl_errorstr);
> -		}
> -	} else {
> -		strbuf_release(&buffer);
> -		die("Unable to start HTTP request");
> -	}
> +	if (http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE) != HTTP_OK)
> +		goto cleanup;

Why not die() as the original code?

Ciao, Dscho

Johannes Schindelin· Jan 18, 2009, 15:10 UTC · re: Mike Hommey · lore

Re: [WIP Patch 04/12] Use the new http API in http_fetch_ref()

Hi,

On Sun, 18 Jan 2009, Mike Hommey wrote:

Show 45 quoted lines

> diff --git a/http.c b/http.c
> index 82534cf..0c9504b 100644
> --- a/http.c
> +++ b/http.c
> @@ -604,34 +604,17 @@ int http_fetch_ref(const char *base, struct ref *ref)
>  {
>  	char *url;
>  	struct strbuf buffer = STRBUF_INIT;
> -	struct active_request_slot *slot;
> -	struct slot_results results;
> -	int ret;
> +	int ret = -1;
>  
>  	url = quote_ref_url(base, ref->name);
> -	slot = get_active_slot();
> -	slot->results = &results;
> -	curl_easy_setopt(slot->curl, CURLOPT_FILE, &buffer);
> -	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
> -	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, NULL);
> -	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
> -	if (start_active_slot(slot)) {
> -		run_active_slot(slot);
> -		if (results.curl_result == CURLE_OK) {
> -			strbuf_rtrim(&buffer);
> -			if (buffer.len == 40)
> -				ret = get_sha1_hex(buffer.buf, ref->old_sha1);
> -			else if (!prefixcmp(buffer.buf, "ref: ")) {
> -				ref->symref = xstrdup(buffer.buf + 5);
> -				ret = 0;
> -			} else
> -				ret = 1;
> -		} else {
> -			ret = error("Couldn't get %s for %s\n%s",
> -				    url, ref->name, curl_errorstr);
> +	if (http_get_strbuf(url, &buffer, HTTP_NO_CACHE) == HTTP_OK) {
> +		strbuf_rtrim(&buffer);
> +		if (buffer.len == 40)
> +			ret = get_sha1_hex(buffer.buf, ref->old_sha1);
> +		else if (!prefixcmp(buffer.buf, "ref: ")) {
> +			ref->symref = xstrdup(buffer.buf + 5);
> +			ret = 0;
>  		}
> -	} else {
> -		ret = error("Unable to start request");
>  	}

Why not keep that error?

BTW I had to scratch my head for a few seconds why you do not need to set "else ret=1;" for the HTTP_OK case; you set ret = 1 in the beginning. I'd rather put that back for clarity.

Ciao, Dscho

Mike Hommey· Jan 18, 2009, 19:21 UTC · re: Johannes Schindelin · lore

Re: [WIP Patch 04/12] Use the new http API in http_fetch_ref()

On Sun, Jan 18, 2009 at 04:10:38PM +0100, Johannes Schindelin wrote:

Show 51 quoted lines

> Hi,
> 
> On Sun, 18 Jan 2009, Mike Hommey wrote:
> 
> > diff --git a/http.c b/http.c
> > index 82534cf..0c9504b 100644
> > --- a/http.c
> > +++ b/http.c
> > @@ -604,34 +604,17 @@ int http_fetch_ref(const char *base, struct ref *ref)
> >  {
> >  	char *url;
> >  	struct strbuf buffer = STRBUF_INIT;
> > -	struct active_request_slot *slot;
> > -	struct slot_results results;
> > -	int ret;
> > +	int ret = -1;
> >  
> >  	url = quote_ref_url(base, ref->name);
> > -	slot = get_active_slot();
> > -	slot->results = &results;
> > -	curl_easy_setopt(slot->curl, CURLOPT_FILE, &buffer);
> > -	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
> > -	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, NULL);
> > -	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
> > -	if (start_active_slot(slot)) {
> > -		run_active_slot(slot);
> > -		if (results.curl_result == CURLE_OK) {
> > -			strbuf_rtrim(&buffer);
> > -			if (buffer.len == 40)
> > -				ret = get_sha1_hex(buffer.buf, ref->old_sha1);
> > -			else if (!prefixcmp(buffer.buf, "ref: ")) {
> > -				ref->symref = xstrdup(buffer.buf + 5);
> > -				ret = 0;
> > -			} else
> > -				ret = 1;
> > -		} else {
> > -			ret = error("Couldn't get %s for %s\n%s",
> > -				    url, ref->name, curl_errorstr);
> > +	if (http_get_strbuf(url, &buffer, HTTP_NO_CACHE) == HTTP_OK) {
> > +		strbuf_rtrim(&buffer);
> > +		if (buffer.len == 40)
> > +			ret = get_sha1_hex(buffer.buf, ref->old_sha1);
> > +		else if (!prefixcmp(buffer.buf, "ref: ")) {
> > +			ref->symref = xstrdup(buffer.buf + 5);
> > +			ret = 0;
> >  		}
> > -	} else {
> > -		ret = error("Unable to start request");
> >  	}
> 
> Why not keep that error?

It should be handled in http_request, I'd say...

Mike

Johannes Schindelin· Jan 18, 2009, 15:03 UTC · re: Mike Hommey · lore

Re: [WIP Patch 03/12] Two new functions for the http API

Hi,

On Sun, 18 Jan 2009, Mike Hommey wrote:

> http_get_strbuf and http_get_file allow respectively to retrieve contents of
> an URL to a strbuf or an opened file handle.

Yesss!  Why have you hidden them that long?

Ciao, Dscho

Johannes Schindelin· Jan 18, 2009, 19:06 UTC · re: Mike Hommey · lore

Re: [WIP Patch 02/12] Some cleanup in get_refs_via_curl()

Hi,

On Sun, 18 Jan 2009, Mike Hommey wrote:

Show 12 quoted lines

> diff --git a/transport.c b/transport.c
> index 56831c5..6919ff1 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -508,6 +508,8 @@ static struct ref *get_refs_via_curl(struct transport *transport)
>  		free(ref);
>  	}
>  
> +	http_cleanup();
> +	free(refs_url);
>  	return refs;
>  }

You cannot http_cleanup() here, as http-push calls that function, but continues to want to use curl.

Ciao, Dscho

Johannes Schindelin· Jan 18, 2009, 19:11 UTC · re: Johannes Schindelin · lore

Re: [WIP Patch 02/12] Some cleanup in get_refs_via_curl()

Hi,

On Sun, 18 Jan 2009, Johannes Schindelin wrote:

Show 17 quoted lines

> On Sun, 18 Jan 2009, Mike Hommey wrote:
> 
> > diff --git a/transport.c b/transport.c
> > index 56831c5..6919ff1 100644
> > --- a/transport.c
> > +++ b/transport.c
> > @@ -508,6 +508,8 @@ static struct ref *get_refs_via_curl(struct transport *transport)
> >  		free(ref);
> >  	}
> >  
> > +	http_cleanup();
> > +	free(refs_url);
> >  	return refs;
> >  }
> 
> You cannot http_cleanup() here, as http-push calls that function, but 
> continues to want to use curl.

Worse, a http clone will hit the same issue.

Ciao, Dscho

Mike Hommey· Jan 18, 2009, 19:30 UTC · re: Johannes Schindelin · lore

Re: [WIP Patch 02/12] Some cleanup in get_refs_via_curl()

On Sun, Jan 18, 2009 at 08:11:07PM +0100, Johannes Schindelin wrote:

Show 23 quoted lines

> Hi,
> 
> On Sun, 18 Jan 2009, Johannes Schindelin wrote:
> 
> > On Sun, 18 Jan 2009, Mike Hommey wrote:
> > 
> > > diff --git a/transport.c b/transport.c
> > > index 56831c5..6919ff1 100644
> > > --- a/transport.c
> > > +++ b/transport.c
> > > @@ -508,6 +508,8 @@ static struct ref *get_refs_via_curl(struct transport *transport)
> > >  		free(ref);
> > >  	}
> > >  
> > > +	http_cleanup();
> > > +	free(refs_url);
> > >  	return refs;
> > >  }
> > 
> > You cannot http_cleanup() here, as http-push calls that function, but 
> > continues to want to use curl.
> 
> Worse, a http clone will hit the same issue.

IIRC, it doesn't break anything because getting refs is going to happen at the beginning, and the following http request is going to reinitialize the whole thing.

It is suboptimal, I agree.

Mike

Johannes Schindelin· Jan 18, 2009, 21:09 UTC · re: Mike Hommey · lore

Re: [WIP Patch 02/12] Some cleanup in get_refs_via_curl()

Hi,

On Sun, 18 Jan 2009, Mike Hommey wrote:

Show 30 quoted lines

> On Sun, Jan 18, 2009 at 08:11:07PM +0100, Johannes Schindelin wrote:
> > Hi,
> > 
> > On Sun, 18 Jan 2009, Johannes Schindelin wrote:
> > 
> > > On Sun, 18 Jan 2009, Mike Hommey wrote:
> > > 
> > > > diff --git a/transport.c b/transport.c
> > > > index 56831c5..6919ff1 100644
> > > > --- a/transport.c
> > > > +++ b/transport.c
> > > > @@ -508,6 +508,8 @@ static struct ref *get_refs_via_curl(struct transport *transport)
> > > >  		free(ref);
> > > >  	}
> > > >  
> > > > +	http_cleanup();
> > > > +	free(refs_url);
> > > >  	return refs;
> > > >  }
> > > 
> > > You cannot http_cleanup() here, as http-push calls that function, but 
> > > continues to want to use curl.
> > 
> > Worse, a http clone will hit the same issue.
> 
> IIRC, it doesn't break anything because getting refs is going to happen
> at the beginning, and the following http request is going to
> reinitialize the whole thing.
> 
> It is suboptimal, I agree.

>From the output of t5540-http-push.sh (which tests http clone

as case 2, incidentally):

-- snip --
*   ok 1: setup remote repository

* expecting success:
        cd "$ROOT_PATH" &&
        git clone $HTTPD_URL/test_repo.git test_repo_clone

*** glibc detected *** git: double free or corruption (!prev): 
0x000000000077e090 ***
======= Backtrace: =========
/lib/libc.so.6[0x2ba4f5ac8b0a]
/lib/libc.so.6(cfree+0x8c)[0x2ba4f5acc6fc]
git[0x4d7ef2]
git[0x4d9073]
git[0x4d90b7]
git[0x4d1626]
git[0x4d1b2f]
git[0x4c9abb]
git[0x4ca00d]
git[0x4cad1a]
git[0x41c5f3]
git[0x404ee8]
git[0x405095]
git[0x40517b]
git[0x4053b8]
/lib/libc.so.6(__libc_start_main+0xf4)[0x2ba4f5a74b44]
git[0x404589]
======= Memory map: ========
00400000-00528000 r-xp 00000000 08:01 45859192                  
         /home/schindelin/git/git
00727000-0072d000 rw-p 00127000 08:01 45859192                  
         /home/schindelin/git/git
0072d000-007b9000 rw-p 0072d000 00:00 0                         
         [heap]
2ba4f4e47000-2ba4f4e64000 r-xp 00000000 08:01 54051869          
         /lib/ld-2.6.1.so
2ba4f4e64000-2ba4f4e68000 rw-p 2ba4f4e64000 00:00 0
2ba4f5063000-2ba4f5065000 rw-p 0001c000 08:01 54051869          
         /lib/ld-2.6.1.so
2ba4f5065000-2ba4f50a2000 r-xp 00000000 08:01 49956909          
         /usr/lib/libcurl.so.4.0.0
2ba4f50a2000-2ba4f52a1000 ---p 0003d000 08:01 49956909          
         /usr/lib/libcurl.so.4.0.0
2ba4f52a1000-2ba4f52a3000 rw-p 0003c000 08:01 49956909          
         /usr/lib/libcurl.so.4.0.0
2ba4f52a3000-2ba4f52b9000 r-xp 00000000 08:01 49956837          
         /usr/lib/libz.so.1.2.3.3
2ba4f52b9000-2ba4f54b9000 ---p 00016000 08:01 49956837          
         /usr/lib/libz.so.1.2.3.3
2ba4f54b9000-2ba4f54ba000 rw-p 00016000 08:01 49956837          
         /usr/lib/libz.so.1.2.3.3
2ba4f54ba000-2ba4f5615000 r-xp 00000000 08:01 49957372          
         /usr/lib/libcrypto.so.0.9.8
2ba4f5615000-2ba4f5815000 ---p 0015b000 08:01 49957372          
         /usr/lib/libcrypto.so.0.9.8
2ba4f5815000-2ba4f5838000 rw-p 0015b000 08:01 49957372          
         /usr/lib/libcrypto.so.0.9.8
2ba4f5838000-2ba4f583b000 rw-p 2ba4f5838000 00:00 0
2ba4f583b000-2ba4f5851000 r-xp 00000000 08:01 54051886          
         /lib/libpthread-2.6.1.so
2ba4f5851000-2ba4f5a50000 ---p 00016000 08:01 54051886          
         /lib/libpthread-2.6.1.so
2ba4f5a50000-2ba4f5a52000 rw-p 00015000 08:01 54051886          
         /lib/libpthread-2.6.1.so
2ba4f5a52000-2ba4f5a57000 rw-p 2ba4f5a52000 00:00 0
2ba4f5a57000-2ba4f5ba9000 r-xp 00000000 08:01 54051872          
         /lib/libc-2.6.1.so
2ba4f5ba9000-2ba4f5da8000 ---p 00152000 08:01 54051872          
         /lib/libc-2.6.1.so
2ba4f5da8000-2ba4f5dab000 r--p 00151000 08:01 54051872          
         /lib/libc-2.6.1.so
2ba4f5dab000-2ba4f5dad000 rw-p 00154000 08:01 54051872          
         /lib/libc-2.6.1.so
2ba4f5dad000-2ba4f5db2000 rw-p 2ba4f5dad000 00:00 0
2ba4f5db2000-2ba4f5ddc000 r-xp 00000000 08:01 49957359          
         /usr/lib/libgssapi_krb5.so.2.2
2ba4f5ddc000-2ba4f5fdb000 ---p 0002a000 08:01 49957359          
         /usr/lib/libgssapi_krb5.so.2.2
2ba4f5fdb000-2ba4f5fdd000 rw-p 00029000 08:01 49957359          
         /usr/lib/libgssapi_krb5.so.2.2
2ba4f5fdd000-2ba4f600e000 r-xp 00000000 08:01 49957876          
         /usr/lib/libidn.so.11.5.29
2ba4f600e000-2ba4f620e000 ---p 00031000 08:01 49957876          
         /usr/lib/libidn.so.11.5.29
2ba4f620e000-2ba4f620f000 rw-p 00031000 08:01 49957876          
         /usr/lib/libidn.so.11.5.29
2ba4f620f000-2ba4f6210000 rw-p 2ba4f620f000 00:00 0
2ba4f6210000-2ba4f6212000 r-xp 00000000 08:01 54051875          
         /lib/libdl-2.6.1.so
2ba4f6212000-2ba4f6412000 ---p 00002000 08:01 54051875          
         /lib/libdl-2.6.1.so
2ba4f6412000-2ba4f6414000 rw-p 00002000 08:01 54051875          
         /lib/libdl-2.6.1.so
2ba4f6414000-2ba4f6458000 r-xp 00000000 08:01 49957373          
         /usr/lib/libssl.so.0.9.8
2ba4f6458000-2ba4f6657000 ---p 00044000 08:01 49957373          
         /usr/lib/libssl.so.0.9.8
2ba4f6657000-2ba4f665d000 rw-p 00043000 08:01 49957373          
         /usr/lib/libssl.so.0.9.8
2ba4f665d000-2ba4f66ed000 r-xp 00000000 08:01 49957362          
         /usr/lib/libkrb5.so.3.3
2ba4f66ed000-2ba4f68ed000 ---p 00090000 08:01 49957362          
         /usr/lib/libkrb5.so.3.3
2ba4f68ed000-2ba4f68f1000 rw-p 00090000 08:01 49957362          
         /usr/lib/libkrb5.so.3.3
2ba4f68f1000-2ba4f68f2000 rw-p 2ba4f68f1000 00:00 0
2ba4f68f2000-2ba4f6916000 r-xp 00000000 08:01 49957360          
         /usr/lib/libk5crypto.so.3.1
2ba4f6916000-2ba4f6b15000 ---p 00024000 08:01 49957360          
         /usr/lib/libk5crypto.so.3.1
2ba4f6b15000-2ba4f6b17000 rw-p 00023000 08:01 49957360          
         /usr/lib/libk5crypto.so.3.1
2ba4f6b17000-2ba4f6b19000 r-xp 00000000 08:01 54050832          
         /lib/libcom_err.so.2.1
2ba4f6b19000-2ba4f6d18000 ---p 00002000 08:01 54050832          
         /lib/libcom_err.so.2.1
2ba4f6d18000-2ba4f6d19000 rw-p 00001000 08:01 54050832          
         /lib/libcom_err.so.2.1
2ba4f6d19000-2ba4f6d20000 r-xp 00000000 08:01 49957363          
         /usr/lib/libkrb5support.so.0.1
2ba4f6d20000-2ba4f6f20000 ---p 00007000 08:01 49957363          
         /usr/lib/libkrb5support.so.0.1
2ba4f6f20000-2ba4f6f21000 rw-p 00007000 08:01 49957363          
         /usr/lib/libkrb5support.so.0.1
2ba4f6f21000-2ba4f6f22000 rw-p 2ba4f6f21000 00:00 0
2ba4f6f22000-2ba4f6f24000 r-xp 00000000 08:01 54050958          
         /lib/libkeyutils-1.2.so
2ba4f6f24000-2ba4f7123000 ---p 00002000 08:01 54050958          
         /lib/libkeyutils-1.2.so
2ba4f7123000-2ba4f7124000 rw-p 00001000 08:01 54050958          
         /lib/libkeyutils-1.2.so
2ba4f7124000-2ba4f7136error: Request for 
0c973ae9bd51902a28466f3850b543fa66a6aaf4 aborted
Initialized empty Git repository in 
/home/schindelin/git/t/trash 
directory.t5540-http-push/test_repo_clone/.git/
Aborted
* FAIL 2: clone remote repository

                cd "$ROOT_PATH" &&
                git clone $HTTPD_URL/test_repo.git test_repo_clone
-- snap --

It might be a strange interaction with my patches, though. valgrind pointed to the http_cleanup() call, and after removing that call, the message goes away.

Ciao, Dscho

Mike Hommey· Jan 18, 2009, 19:19 UTC · re: Johannes Schindelin · lore

Re: [WIP Patch 02/12] Some cleanup in get_refs_via_curl()

On Sun, Jan 18, 2009 at 08:06:17PM +0100, Johannes Schindelin wrote:

Show 19 quoted lines

> Hi,
> 
> On Sun, 18 Jan 2009, Mike Hommey wrote:
> 
> > diff --git a/transport.c b/transport.c
> > index 56831c5..6919ff1 100644
> > --- a/transport.c
> > +++ b/transport.c
> > @@ -508,6 +508,8 @@ static struct ref *get_refs_via_curl(struct transport *transport)
> >  		free(ref);
> >  	}
> >  
> > +	http_cleanup();
> > +	free(refs_url);
> >  	return refs;
> >  }
> 
> You cannot http_cleanup() here, as http-push calls that function, but 
> continues to want to use curl.

Are you really sure? It doesn't seem so, to me.

Mike

Johannes Schindelin· Jan 18, 2009, 21:10 UTC · re: Mike Hommey · lore

Re: [WIP Patch 02/12] Some cleanup in get_refs_via_curl()

Hi,

On Sun, 18 Jan 2009, Mike Hommey wrote:

Show 6 quoted lines

> On Sun, Jan 18, 2009 at 08:06:17PM +0100, Johannes Schindelin wrote:
> 
> > You cannot http_cleanup() here, as http-push calls that function, but 
> > continues to want to use curl.
> 
> Are you really sure? It doesn't seem so, to me.

Nope, sorry.  It was http clone.

Ciao, Dscho

Junio C Hamano· Jan 18, 2009, 08:30 UTC · re: Mike Hommey · lore

Re: [WIP Patch 00/12] Refactoring the http API

Mike Hommey <mh@glandium.org> writes:

Show 8 quoted lines

> As it is work in progress, some error handling might have regressions, but
> the original error handling is not necessarily much better.
>
> Also note I only rebased my one-year-old work on current master, and haven't
> actually tested it, though, as the code hasn't changed much, I guess it
> should be fine.
> ...
>  6 files changed, 162 insertions(+), 304 deletions(-)

Thanks.

This looks like a very nice code reduction, and the first few patches looked obviously correct, too ;-)

But I am puzzled by what you mean by "haven't actually tested it". Do you mean you do not use http transport very much yourself, or even when you do you do not use a version of git with these patches applied?

Mike Hommey· Jan 18, 2009, 09:12 UTC · re: Junio C Hamano · lore

Re: [WIP Patch 00/12] Refactoring the http API

On Sun, Jan 18, 2009 at 12:30:12AM -0800, Junio C Hamano wrote:

Show 19 quoted lines

> Mike Hommey <mh@glandium.org> writes:
> 
> > As it is work in progress, some error handling might have regressions, but
> > the original error handling is not necessarily much better.
> >
> > Also note I only rebased my one-year-old work on current master, and haven't
> > actually tested it, though, as the code hasn't changed much, I guess it
> > should be fine.
> > ...
> >  6 files changed, 162 insertions(+), 304 deletions(-)
> 
> Thanks.
> 
> This looks like a very nice code reduction, and the first few patches
> looked obviously correct, too ;-)
> 
> But I am puzzled by what you mean by "haven't actually tested it".  Do you
> mean you do not use http transport very much yourself, or even when you do
> you do not use a version of git with these patches applied?

I mean I haven't tested the rebased version. The original version was tested extensively a year ago. I don't use http transport that much now.

Mike

Boyd Stephen Smith Jr.· Jan 18, 2009, 11:29 UTC · re: Mike Hommey · lore

Re: [WIP Patch 00/12] Refactoring the http API

On Sunday 18 January 2009, Mike Hommey <mh@glandium.org> wrote about 'Re: [WIP Patch 00/12] Refactoring the http API':

Show 14 quoted lines

>On Sun, Jan 18, 2009 at 12:30:12AM -0800, Junio C Hamano wrote:
>> Mike Hommey <mh@glandium.org> writes:
>> > [I]
>> > haven't actually tested it, though, as the code hasn't changed much,
>> > I guess it should be fine.
>> >  6 files changed, 162 insertions(+), 304 deletions(-)
>>
>> Thanks.
>> But I am puzzled by what you mean by "haven't actually tested it".  Do
>> you mean you do not use http transport very much yourself, or even when
>> you do you do not use a version of git with these patches applied?
>I mean I haven't tested the rebased version. The original version was
>tested extensively a year ago. I don't use http transport that much
>now.

I know it's uncool of me to ask to do this, BUT... Do we have a good test-suite for this? If so, I can always run it against any number of different WEBDAV implementations. I have a glut of CPU time on many of my systems and enjoy breaking things.

-- 
Boyd Stephen Smith Jr.                     ,= ,-_-. =. 
bss@iguanasuicide.net                     ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy           `-'(. .)`-' 
http://iguanasuicide.net/                      \_/